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

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

Hi Shally,

//snip//;
> >> +	/**< 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?
> [Shally] We will need to add an API, say, rte_comp_set_dictionary() to load pre-set dictionary.
> If a particular algorithm implementation supports dictionary load, then app can call this API before starting compression.
[Fiona] ok. so maybe submit in a patch separate to hash on github.
With any needed APIs.
 
> 
> >
> >> +	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.
> 
> [Shally] Ok. I miss to set background here.
> Though practically hash can be performed standalone on data but we are proposing it as a feature that can
> be performed along with compression and so
> is the assumption for crc/adler. Thus , we proposed it as part of compression algo capability. Ex.
> Implementation may support Deflate+sha1 but not lzs+sha1.
> Do you want to propose hash/checksum as standalone capability on compression API? shouldn't
> standalone hash a part of  crypto spec? similar is question
> for checksum because app can use processor optimized crc/adler library func, if required. So, I see
> standalone hash/checksums as duplicated feature on compression.
> 
> I will issue next patch on github, once we this requirement is resolved.
[Fiona] No, I wasn't suggesting as standalone op - just that I thought it a device had the capability
to do hash it could probably do it regardless of which algo is used. 
But if you think there's a case where a device has deflate+hash capability and not lzs+sha1
then the hash capability can be per-algo. Based on above can it just be added to the 
existing feature-flags bitmask? Probably no need for the separate hash and algo capability
structs you were proposing since all capabilities are per-algo.

> 
> >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 */
> >>  };
> >> +
//snip