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:[email protected]] > Sent: Thursday, February 1, 2018 11:07 AM > To: Trahe, Fiona <[email protected]> > Cc: [email protected]; [email protected]; > [email protected]; > [email protected]; [email protected] > 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 <[email protected]> > --- > 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
