Re: [dpdk-dev] [PATCH v2 3/5] compress/zlib: add xform and stream create support

["Verma, Shally" <Shally.Verma@cavium.com>]

>-----Original Message-----
>From: De Lara Guarch, Pablo [mailto:pablo.de.lara.guarch@intel.com]
>Sent: 11 July 2018 17:57
>To: Verma, Shally <Shally.Verma@cavium.com>
>Cc: dev@dpdk.org; Athreya, Narayana Prasad <NarayanaPrasad.Athreya@cavium.com>; Challa, Mahipal
><Mahipal.Challa@cavium.com>; Sahu, Sunila <Sunila.Sahu@cavium.com>; Sahu, Sunila <Sunila.Sahu@cavium.com>; Gupta, Ashish
><Ashish.Gupta@cavium.com>
>Subject: RE: [PATCH v2 3/5] compress/zlib: add xform and stream create support
>
>External Email
>
>> -----Original Message-----
>> From: Shally Verma [mailto:shally.verma@caviumnetworks.com]
>> Sent: Monday, July 2, 2018 5:57 PM
>> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
>> Cc: dev@dpdk.org; pathreya@caviumnetworks.com;
>> mchalla@caviumnetworks.com; Sunila Sahu <ssahu@caviumnetworks.com>;
>> Sunila Sahu <sunila.sahu@caviumnetworks.com>; Ashish Gupta
>> <ashish.gupta@caviumnetworks.com>
>> Subject: [PATCH v2 3/5] compress/zlib: add xform and stream create support
>
>Retitle to "compress/zlib: support xform and stream creation"
>
>About stateful, since it looks like it is not supported for the moment,
>I think stream_create/stream_free functions should fail or should not be implemented.
>
>More comments inline.
>
>>
>> From: Sunila Sahu <ssahu@caviumnetworks.com>
>>
>> Implement private xform and stream create ops
>>
>> Signed-off-by: Sunila Sahu <sunila.sahu@caviumnetworks.com>
>> Signed-off-by: Shally Verma <shally.verma@caviumnetworks.com>
>> Signed-off-by: Ashish Gupta <ashish.gupta@caviumnetworks.com>
>> ---
>>  drivers/compress/zlib/zlib_pmd.c         | 93
>> ++++++++++++++++++++++++++++++++
>>  drivers/compress/zlib/zlib_pmd_ops.c     | 83 ++++++++++++++++++++++++++--
>>  drivers/compress/zlib/zlib_pmd_private.h |  4 ++
>>  3 files changed, 176 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/compress/zlib/zlib_pmd.c b/drivers/compress/zlib/zlib_pmd.c
>> index c4f67bb..7c2614e 100644
>> --- a/drivers/compress/zlib/zlib_pmd.c
>> +++ b/drivers/compress/zlib/zlib_pmd.c
>
>...
>
>> +             case RTE_COMP_HUFFMAN_DYNAMIC:
>> +                     strategy = Z_HUFFMAN_ONLY;
>
>I don't think this is the suitable value for dynamic compression.
>Z_HUFFMAN_ONLY does not do LZ77. I think the strategy for this case is Z_DEFAULT_STRATEGY.
Ya. you right. It will be changed to DEFAULT_STRATEGY.

Thanks
Shally

>
>> +                     break;
>> +             default:
>> +                     ZLIB_PMD_ERR("Compression strategy not
>> supported\n");
>> +                     return -1;
>> +             }
>
>...
>
>> +++ b/drivers/compress/zlib/zlib_pmd_ops.c
>
>...
>
>> +     struct rte_mempool *mp = rte_mempool_lookup(internals->mp_name);
>
>As said in other email, you can directly get the mempool pointer from internals.
>
>> +     if (rte_mempool_get(mp, zstream)) {
>> +             ZLIB_PMD_ERR(
>> +                             "Couldn't get object from session mempool");
>
>This can be in the same line.
>
>...
>
>> +/** Configure private xform */
>> +static int
>> +zlib_pmd_private_xform_create(struct rte_compressdev *dev,
>> +             const struct rte_comp_xform *xform,
>> +             void **private_xform)
>> +{
>> +     int ret = 0;
>> +
>> +     ret = zlib_pmd_stream_create(dev, xform, private_xform);
>> +     return ret;
>
>This can be written like "return zlib_pmd_stream_create(...);
>
>...
>
>> +             .private_xform_create   = zlib_pmd_private_xform_create,
>> +             .private_xform_free             =
>> zlib_pmd_private_xform_free,
>
>Fix indentation here.
>
>>
>> -             .stream_create  = NULL,
>> -             .stream_free    = NULL
>> +             .stream_create  = zlib_pmd_stream_create,
>> +             .stream_free    = zlib_pmd_stream_free
>>  };