RE: [EXTERNAL] [PATCH 1/3] compressdev: support for dictionaries and PDCP checksum

[Akhil Goyal <gakhil@marvell.com>]

Hi Sameer,
> Adds definitions for PDCP checksums and apis to pass in
> dictionaries
I think you need to define new algo UDC as well.

> 
> Signed-off-by: Sameer Vaze <svaze@qti.qualcomm.com>

Please update version in the patch title while sending the new version.
It took me a lot of time to figure out which version I should review and reply to.
I can see there are 10s of series spammed without updating the version.
Please take care of this while sending the next version.
Also add in-reply-to tag of previous version while sending next version.
Please check https://doc.dpdk.org/guides/contributing/patches.html#sending-patches
https://doc.dpdk.org/guides/contributing/patches.html#the-review-process

> ---
And you should also specify change log here after ---

>  lib/compressdev/rte_comp.h | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/lib/compressdev/rte_comp.h b/lib/compressdev/rte_comp.h
> index 96d9b276dd..169d3d960e 100644
> --- a/lib/compressdev/rte_comp.h
> +++ b/lib/compressdev/rte_comp.h
> @@ -101,6 +101,10 @@ enum rte_comp_op_status {
>  	 * is not an error case. Output data up to op.produced can be used and
>  	 * next op in the stream should continue on from op.consumed+1.
>  	 */
> +	RTE_COMP_OP_STATUS_CHECK_SUM_VALIDATION_FAILED,
> +	/**< Checksum validation failed. Either calculated does checksum not
> match
> +	 * the one provided or there was an error calculating the checksum
> +	 */
>  };
> 
>  /** Compression Algorithms */
> @@ -166,6 +170,10 @@ enum rte_comp_checksum_type {
>  	/**< Generates a xxHash-32 checksum, as used by LZ4.
>  	 * https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__github.com_Cyan4973_xxHash_blob_dev_doc_xxhash-
> 5Fspec.md&d=DwIDAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=DnL7Si2wl_PRwpZ9TW
> ey3eu68gBzn7DkPwuqhd6WNyo&m=YLWffSHu_HeEi63ZTpMIS7uCsbTGlu1gZDDe
> Pxv8nL7v3JAvIdKcNrrUq3BR5Y22&s=b5bCLrPbUjjxy1ItxfYQOUoHAnDjqjVwVfh6il
> TDCIw&e=
>  	 */
> +	RTE_COMP_CHECKSUM_3GPP_PDCP_UDC,
> +	/**< Generates checksum as defined under Uplink Data Compression
> +	 * checksum as defined in the 3GPP PDCP specification
> +	 */
>  };
> 
>  /** Compression Huffman Type - used by DEFLATE algorithm */
> @@ -201,6 +209,11 @@ enum rte_comp_flush_flag {
>  	 */
>  };
> 
> +#define	DEFLATE_MAX_WINDOW_SIZE	(1ULL << 15)
Wrong namespace used.
Are these really required to be defined here.
I do see any comments where these macros will be used and what is the use case.

> +
Remove extra line here

> +#define	DEFLATE_MIN_WINDOW_SIZE		(1ULL << 8)
> +
Remove extra line here
> +
>  /** Compression transform types */
>  enum rte_comp_xform_type {
>  	RTE_COMP_COMPRESS,
> @@ -305,6 +318,15 @@ struct rte_comp_compress_xform {
>  	/**< Hash algorithm to be used with compress operation. Hash is always
>  	 * done on plaintext.
>  	 */
> +	uint8_t *dictionary;
> +	/**<
> +	 * Pointer to memory containing dictionary to be used for inflate
> +	 * and deflate operations
> +	 */
> +	uint16_t dictionary_len;
> +	/**<
> +	 * Length of dictionary to be used
> +	 */

Don’t you need to define UDC in rte_comp_algorithm?
And move these algorithm specific fields into a separate struct and add that into the union defined above in this struct?
Seems patch is incomplete.

There is no update to documentation as well.
doc/guides/prog_guide/compressdev.rst
doc/guides/compressdevs/features/default.ini

>  };
> 
>  /**
> @@ -328,6 +350,15 @@ struct rte_comp_decompress_xform {
>  	/**< Hash algorithm to be used with decompress operation. Hash is
> always
>  	 * done on plaintext.
>  	 */
> +	uint8_t *dictionary;
> +	/**<
> +	 * Pointer to memory containing dictionary to be used for inflate
> +	 * and deflate operations
> +	 */
Please make this in a single line if it is under 100 characters.

> +	uint16_t dictionary_len;
> +	/**<
> +	 * Length of dictionary to be used
> +	 */
Make this also in a single line instead of 3 lines as being followed for other fields.


>  };
> 
>  /**
> --
> 2.31.1