Re: [dpdk-dev] [RFC v4 1/1] lib/compressdev: Adding hash support

["Trahe, Fiona" <fiona.trahe@intel.com>]

Hi Shally,

Sorry about the delay in responding. Comments below.
If you want to push next update to github we can continue dev there.
Regards,
Fiona


> -----Original Message-----
> From: Shally Verma [mailto:shally.verma@caviumnetworks.com]
> Sent: Thursday, February 1, 2018 11:07 AM
> To: Trahe, Fiona <fiona.trahe@intel.com>
> Cc: pathreya@caviumnetworks.com; mchalla@caviumnetworks.com; agupta@caviumnetworks.com;
> dev@dpdk.org; ahmed.mansour@nxp.com
> Subject: [RFC v4 1/1] lib/compressdev: Adding hash support
> 
> Added hash support in lib compressdev. It's an incremental patch to
> compression lib RFC v3 https://dpdk.org/dev/patchwork/patch/32331/
> 
> Changes from RFC v3:
> - Added hash algo enumeration and associated capability stucture
>   and params in xform and rte_comp_op
> - Rearranged rte_compresdev_capability structure to have
>   separate rte_comp_algo_capability and moved
>   algo specific capabilities: window_size, dictionary support,
>   and hash as part of it
> - Added RTE_COMP_UNSPECIFIED=0 in enum rte_comp_algorithm
> - Redefined RTE_COMP_END_OF_CAPABILITIES_LIST to use RTE_COMP_UNSPECIFIED
>   to resolve missing-field-initializer compiler warning
> - Updated compress/decompress xform to input hash algorithm
>   during session init
> - Updated struct rte_comp_op to input hash buffer
> - Fixed checkpatch reported errors on RFCv3
> 
> Every compression algorithm can indicate its capability to
> perform alongside hash in its associate rte_comp_algo_capa structure.
> If none is supported, can terminate array with
> hash_algo = RTE_COMP_HASH_ALGO_UNSPECIFIED.
> if supported, application can initialize session with desired
> algorithm enumeration in xform structure and pass valid hash buffer
> pointer during enqueue_burst().
> 
> Signed-off-by: Shally Verma <shally.verma@caviumnetworks.com>
> ---
>  lib/librte_compressdev/rte_comp.h                  | 83 +++++++++++++++++-----
>  lib/librte_compressdev/rte_compressdev.c           | 19 +++++
>  lib/librte_compressdev/rte_compressdev.h           | 59 ++++++++++++---
>  lib/librte_compressdev/rte_compressdev_version.map |  1 +
>  4 files changed, 137 insertions(+), 25 deletions(-)
> 
> diff --git a/lib/librte_compressdev/rte_comp.h b/lib/librte_compressdev/rte_comp.h
> index ca8cbb4..341f59f 100644
> --- a/lib/librte_compressdev/rte_comp.h
> +++ b/lib/librte_compressdev/rte_comp.h
> @@ -75,10 +75,11 @@ enum rte_comp_op_status {
>  	 */
>  };
> 
> -
>  /** Compression Algorithms */
>  enum rte_comp_algorithm {
> -	RTE_COMP_NULL = 0,
> +	RTE_COMP_UNSPECIFIED = 0,
> +	/** No Compression algo */
> +	RTE_COMP_NULL,
>  	/**< No compression.
>  	 * Pass-through, data is copied unchanged from source buffer to
>  	 * destination buffer.
> @@ -94,6 +95,18 @@ enum rte_comp_algorithm {
>  	RTE_COMP_ALGO_LIST_END
>  };
> 
> +
> +/** Compression hash algorithms */
> +enum rte_comp_hash_algorithm {
> +	RTE_COMP_HASH_ALGO_UNSPECIFIED = 0,
> +	/**< No hash */
> +	RTE_COMP_HASH_ALGO_SHA1,
> +	/**< SHA1 hash algorithm */
> +	RTE_COMP_HASH_ALGO_SHA256,
> +	/**< SHA256 hash algorithm */
> +	RTE_COMP_HASH_ALGO_LIST_END,
> +};
> +
>  /**< Compression Level.
>   * The number is interpreted by each PMD differently. However, lower numbers
>   * give fastest compression, at the expense of compression ratio while
> @@ -154,21 +167,24 @@ enum rte_comp_flush_flag {
>  	RTE_COMP_FLUSH_SYNC,
>  	/**< All data should be flushed to output buffer. Output data can be
>  	 * decompressed. However state and history is not cleared, so future
> -	 * ops may use history from this op */
> +	 * ops may use history from this op
> +	 */
>  	RTE_COMP_FLUSH_FULL,
>  	/**< All data should be flushed to output buffer. Output data can be
>  	 * decompressed. State and history data is cleared, so future
>  	 * ops will be independent of ops processed before this.
>  	 */
>  	RTE_COMP_FLUSH_FINAL
> -	/**< Same as RTE_COMP_FLUSH_FULL but also bfinal bit is set in last block
> +	/**< Same as RTE_COMP_FLUSH_FULL but also bfinal bit is set in
> +	 * last block
>  	 */
>  	 /* TODO:
>  	  * describe flag meanings for decompression.
>  	  * describe behavous in OUT_OF_SPACE case.
>  	  * At least the last flag is specific to deflate algo. Should this be
>  	  * called rte_comp_deflate_flush_flag? And should there be
> -	  * comp_op_deflate_params in the op? */
> +	  * comp_op_deflate_params in the op?
> +	  */
>  };
> 
>  /** Compression transform types */
> @@ -180,17 +196,17 @@ enum rte_comp_xform_type {
>  };
> 
>  enum rte_comp_op_type {
> -    RTE_COMP_OP_STATELESS,
> -    /**< All data to be processed is submitted in the op, no state or history
> -     * from previous ops is used and none will be stored for future ops.
> -     * flush must be set to either FLUSH_FULL or FLUSH_FINAL
> -     */
> -    RTE_COMP_OP_STATEFUL
> -    /**< There may be more data to be processed after this op, it's part of a
> -     * stream of data. State and history from previous ops can be used
> -     * and resulting state and history can be stored for future ops,
> -     * depending on flush_flag.
> -     */
> +	RTE_COMP_OP_STATELESS,
> +	/**< All data to be processed is submitted in the op, no state or
> +	 * history from previous ops is used and none will be stored for
> +	 * future ops.flush must be set to either FLUSH_FULL or FLUSH_FINAL
> +	 */
> +	RTE_COMP_OP_STATEFUL
> +	/**< There may be more data to be processed after this op, it's
> +	 * part of a stream of data. State and history from previous ops
> +	 * can be usedand resulting state and history can be stored for
> +	 * future ops, depending on flush_flag.
> +	 */
>  };
> 
> 
> @@ -207,6 +223,13 @@ struct rte_comp_deflate_params {
>  struct rte_comp_compress_common_params {
>  	enum rte_comp_algorithm algo;
>  	/**< Algorithm to use for compress operation */
> +	enum rte_comp_hash_algorithm hash_algo;
> +	/**< Hash algorithm to be used with compress operation. Hash is always
> +	 * done on plaintext. application can query
> +	 * rte_compressdev_capability_get() and parse hash_capa array per each
> +	 * algorithm to know supported hash algos and associated params until
> +	 * it find an entry with RTE_COMP_HASH_ALGO_UNSPECIFIED
> +	 */
>  	union {
>  		struct rte_comp_deflate_params deflate;
>  		/**< Parameters specific to the deflate algorithm */
> @@ -240,6 +263,13 @@ struct rte_comp_compress_xform {
>  struct rte_comp_decompress_common_params {
>  	enum rte_comp_algorithm algo;
>  	/**< Algorithm to use for decompression */
> +	enum rte_comp_hash_algorithm hash_algo;
> +	/**< Hash algorithm to be used with decompress operation. Hash is always
> +	 * done on plaintext. application can query
> +	 * rte_compressdev_capability_get() and parse hash_capa array per each
> +	 * algorithm to know supported hash algos and associated params until
> +	 * it find an entry with RTE_COMP_HASH_ALGO_UNSPECIFIED
> +	 */
>  	enum rte_comp_checksum_type chksum;
>  	/**< Type of checksum to generate on the decompressed data. */
>  	uint16_t window_size;
> @@ -301,7 +331,7 @@ struct rte_comp_xform {
>  struct rte_comp_op {
> 
>  	enum rte_comp_op_type op_type;
> -	void * stream_private;
> +	void *stream_private;
>  	/* location where PMD maintains stream state
>  	 * only required if op_type is STATEFUL, else should be NULL
>  	 */
> @@ -344,6 +374,25 @@ struct rte_comp_op {
>  		 * decompress direction.
>  		 */
>  	} dst;
> +	struct {
> +		uint8_t *hash_buf;
> +		/**< Pointer to output hash calculated on plaintext if session
> +		 * is initialized with Hash algorithm RTE_COMP_HASH_ALGO_SHA1 /
> +		 * RTE_COMP_HASH_ALGO_SHA256. Buffer would contain valid value
> +		 * only after an op with
> +		 * flush flag = RTE_COMP_FLUSH_FULL/FLUSH_FINAL is processed
> +		 * successfully.
> +		 *
> +		 * Length of buffer should be large enough to accommodate digest
> +		 * produced by specific hash algo. Application can know
> +		 * digest_size via parsing hash capability array in struct
> +		 * rte_compressdev_capabilities
> +		 */
> +		uint32_t offset;
> +		/**< Starting offset for writing hash bytes, specified as
> +		 * number of bytes from start of hash buffer pointer.
> +		 */
[Fiona] Is offset really necessary?
If it is then need to update description of length of hash buffer above to be digest_size + offset.
Also I expect an iova address is necessary for a hw accelerator to produce this hash.

> +	} hash;
>  	enum rte_comp_flush_flag flush_flag;
>  	/**< defines flush characteristics for the output data.
>  	 * Only applicable in compress direction
> diff --git a/lib/librte_compressdev/rte_compressdev.c b/lib/librte_compressdev/rte_compressdev.c
> index 6186ce5..b62b1ce 100644
> --- a/lib/librte_compressdev/rte_compressdev.c
> +++ b/lib/librte_compressdev/rte_compressdev.c
> @@ -113,6 +113,25 @@ struct rte_compressdev_callback {
> 
>  };
> 
> +const struct rte_compressdev_capabilities *
> +rte_compressdev_capability_get(uint8_t dev_id,
> +		const struct rte_compressdev_capability_idx *idx)
> +{
> +	const struct rte_compressdev_capabilities *capability;
> +	struct rte_compressdev_info dev_info;
> +	int i = 0;
> +
> +	memset(&dev_info, 0, sizeof(struct rte_compressdev_info));
> +	rte_compressdev_info_get(dev_id, &dev_info);
> +
> +	while ((capability = &dev_info.capabilities[i++])->algo !=
> +			RTE_COMP_UNSPECIFIED){
> +		if (capability->algo == idx->algo)
> +			return capability;
> +	}
> +
> +	return NULL;
> +}
> 
>  #define param_range_check(x, y) \
>  	(((x < y.min) || (x > y.max)) || \
> diff --git a/lib/librte_compressdev/rte_compressdev.h b/lib/librte_compressdev/rte_compressdev.h
> index 9a86603..7d1b9b1 100644
> --- a/lib/librte_compressdev/rte_compressdev.h
> +++ b/lib/librte_compressdev/rte_compressdev.h
> @@ -125,22 +125,65 @@ struct rte_comp_param_range {
>  	 */
>  };
> 
> +/**
> + * Compression hash algo Capability
> + */
> +struct rte_comp_hash_algo_capability {
> +	enum rte_comp_hash_algorithm hash_alg;
> +	/**< hash algo enumeration */
> +	uint32_t digest_size;
> +	/**< length of digest produced by hash */
[Fiona] op doesn't give an option to specify digest_length.
So if it's a fixed size per hash algo then no need for 
length in capabilities. And so can maybe use a bitmask for hash capabilities.
If a range is needed then this should be a range and add digest_length to op or xform

> +};
> +
> +/**
> + * Compression algo Capability
> + */
> +struct rte_comp_algo_capability {
> +	struct rte_comp_param_range window_size;
> +	/**< window size range in bytes */
> +	int support_dict;
> +	/** Indicate algo support for dictionary load */
[Fiona] I need more info on this - What else is needed on the API to support it?

> +	struct rte_comp_hash_algo_capability *hash_capa;
> +	/** pointer to an array of hash capability struct */
[Fiona] Does the hash capability depend on the compression algorithm?
I'd have thought it's completely independent as it's computed 
on the uncompressed data?
I suppose the same can be said of checksum.
This all seems a bit awkward.
Would something like this be better:

enum rte_comp_capability_type {
  RTE_COMP_CAPA_TYPE_ALGO,
  RTE_COMP_CAPA_TYPE_CHKSUM,
  RTE_COMP_CAPA_TYPE_HASH
}

struct rte_comp_algo_capability {
               enum rte_comp_algorithm algo;
	/** Compression Algorithm */
	uint64_t comp_feature_flags;
	/**< bitmap of flags for compression service features supported with this algo */
	struct rte_comp_param_range window_size;
	/**< window size range in bytes supported with this algo */
               /* int support_dict; */
              /** Indicate algo support for dictionary load */
};

struct rte_compressdev_capabilities {
{
	rte_comp_capability_type capa_type;
	union {
	    struct rte_comp_algo_capability algo_capa,
	    int checksum_capa, /* bitmask of supported checksums  */
	    int hash_capa, /* bitmask of supported hashes - this could be a struct if more data needed */
	}
};

> 
>  /** Structure used to capture a capability of a comp device */
>  struct rte_compressdev_capabilities {
> -	/* TODO */
>  	enum rte_comp_algorithm algo;
> +	/** Compression Algorithm */
> +	struct rte_comp_algo_capability alg_capa;
> +	/**< Compression algo capabilities */
>  	uint64_t comp_feature_flags;
> -	/**< bitmap of flags for compression service features*/
> -	struct rte_comp_param_range window_size;
> -	/**< window size range in bytes */
> +	/**< bitmap of flags for compression service features */
>  };
> 
> +/**  Structure used to index compression device capability array */
> +struct rte_compressdev_capability_idx {
> +	enum rte_comp_algorithm algo;
> +};
> +
> +/**
> + *  Provide device capability for requested algorithm
> + *
> + * @param       dev_id          The identifier of the device.
> + * @param       idx             Index to capability array
> + *
> + * @return
> + *   - Return pointer to capability structure, if exist.
> + *   - Return NULL, if does not exist.
> + */
> +const struct rte_compressdev_capabilities *
> +rte_compressdev_capability_get(uint8_t dev_id,
> +		const struct rte_compressdev_capability_idx *idx);
> +
> 
>  /** Macro used at end of comp PMD list */
>  #define RTE_COMP_END_OF_CAPABILITIES_LIST() \
> -	{ RTE_COMP_ALGO_LIST_END }
> +	{ RTE_COMP_UNSPECIFIED }
> 
> +/** Macro used at end of comp hash capability list */
> +#define RTE_COMP_HASH_END_OF_CAPABILITIES_LIST() \
> +	{ RTE_COMP_HASH_ALG_UNSPECIFIED }
> 
>  /**
>   * compression device supported feature flags
> @@ -833,7 +876,7 @@ struct rte_comp_session *
>  rte_compressdev_queue_pair_detach_session(uint8_t dev_id, uint16_t qp_id,
>  		struct rte_comp_session *session);
>  /**
> - * This should alloc a stream from the device's mempool and initialise it.
> + * This should alloc a stream from the device's mempool and initialise it.
>   * This handle will be passed to the PMD with every op in the stream.
>   *
>   * @param	dev_id		The identifier of the device.
> @@ -850,7 +893,7 @@ struct rte_comp_session *
>   */
>  int
>  rte_comp_stream_create(uint8_t dev_id, struct rte_comp_session *sess,
> -					void ** stream);
> +					void **stream);
> 
>  /**
>   * This should clear the stream and return it to the device's mempool.
> @@ -863,7 +906,7 @@ struct rte_comp_session *
>   * @return
>   */
>  int
> -rte_comp_stream_free(uint8_t dev_id, void * stream);
> +rte_comp_stream_free(uint8_t dev_id, void *stream);
> 
>  /**
>   * Provide driver identifier.
> diff --git a/lib/librte_compressdev/rte_compressdev_version.map
> b/lib/librte_compressdev/rte_compressdev_version.map
> index 665f0bf..3114949 100644
> --- a/lib/librte_compressdev/rte_compressdev_version.map
> +++ b/lib/librte_compressdev/rte_compressdev_version.map
> @@ -5,6 +5,7 @@ EXPERIMENTAL {
>  	rte_compressdev_allocate_driver;
>  	rte_compressdev_callback_register;
>  	rte_compressdev_callback_unregister;
> +	rte_compressdev_capability_get;
>  	rte_compressdev_close;
>  	rte_compressdev_configure;
>  	rte_compressdev_count;
> --
> 1.9.1