[dpdk-dev] [PATCH 2/3] app/compress-perf: add performance measurement

[Verma, Shally]

>-----Original Message-----
>From: Daly, Lee <lee.daly at intel.com>
>Sent: 15 October 2018 20:40
>To: Verma, Shally <Shally.Verma at cavium.com>
>Cc: Jozwiak, TomaszX <tomaszx.jozwiak at intel.com>; dev at dpdk.org; Trahe, Fiona <fiona.trahe at intel.com>; akhil.goyal at nxp.com
>Subject: RE: [dpdk-dev] [PATCH 2/3] app/compress-perf: add performance measurement
>
>External Email
>
>Thanks for your input Shally see comments below.
>
>
>I will be reviewing these changes while Tomasz is out this week.
>
>> -----Original Message-----
>> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Verma, Shally
>> Sent: Friday, October 12, 2018 11:16 AM
>> To: Jozwiak, TomaszX <tomaszx.jozwiak at intel.com>; dev at dpdk.org; Trahe,
>> Fiona <fiona.trahe at intel.com>; akhil.goyal at nxp.com; De Lara Guarch, Pablo
>> <pablo.de.lara.guarch at intel.com>
>> Cc: De at dpdk.org; Lara at dpdk.org; Guarch at dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH 2/3] app/compress-perf: add performance
>> measurement
>>
>> HI TomaszX
>>
>> Sorry for delay in response. Comments inline.
>>
>
><...>
>> >+static int
>> >+comp_perf_check_capabilities(struct comp_test_data *test_data) {
>> >+       const struct rte_compressdev_capabilities *cap;
>> >+
>> >+       cap = rte_compressdev_capability_get(test_data->cdev_id,
>> >+                                            RTE_COMP_ALGO_DEFLATE);
>> >+
>> >+       if (cap == NULL) {
>> >+               RTE_LOG(ERR, USER1,
>> >+                       "Compress device does not support DEFLATE\n");
>> >+               return -1;
>> >+       }
>> >+
>> >+       uint64_t comp_flags = cap->comp_feature_flags;
>> >+
>> >+       /* Huffman enconding */
>> >+       if (test_data->huffman_enc == RTE_COMP_HUFFMAN_FIXED &&
>> >+                       (comp_flags & RTE_COMP_FF_HUFFMAN_FIXED) == 0) {
>> >+               RTE_LOG(ERR, USER1,
>> >+                       "Compress device does not supported Fixed Huffman\n");
>> >+               return -1;
>> >+       }
>> >+
>> >+       if (test_data->huffman_enc == RTE_COMP_HUFFMAN_DYNAMIC &&
>> >+                       (comp_flags & RTE_COMP_FF_HUFFMAN_DYNAMIC) == 0) {
>> >+               RTE_LOG(ERR, USER1,
>> >+                       "Compress device does not supported Dynamic Huffman\n");
>> >+               return -1;
>> >+       }
>> >+
>> >+       /* Window size */
>> >+       if (test_data->window_sz != -1) {
>> >+               if (param_range_check(test_data->window_sz,
>> >+ &cap->window_size)
>> What if cap->window_size is 0 i.e. implementation default?
>What do you mean when you say cap->window_size = 0?
>Cap->window_size is the range structure here, min, max and increment, which are filled out by the driver.
>Our implementation default in the perf tool will set the window size to max the driver can support.
If I recall and if I am not mixing my memories, I  believe, we added a condition in lib where driver can set window sz , min = 0 or max = 0 to just mark implementation default. If that's not the case supported yet on lib, then you can ignore this comment.
>
...

>> It looks like it will run 2nd time only if input file size < input data size in which
>> case it will just keep filling input buffer with repeated data.
>> Is that the intention here?
>From what I can see, yes, this will only enter this while loop a second time if the file is smaller than the data_size requested.
>Repeating the data from your input file as much as requested.
>If we were to pad with 0's or random data it would skew the ratio a lot.
>Even though I do understand the ratio may be better here in this case as well, due to the repetition of data.
>
Yea. So I think not to influence benchmark data here. we should stick to input filesz user is giving. As performance
at a particular level will vary by content type so lets app choose and find out performance for a given content type.

>>
...

>> >+       if (benchmarking) {
>> >+               tsc_end = rte_rdtsc();
>> >+               tsc_duration = tsc_end - tsc_start;
>> >+
>> >+               if (type == RTE_COMP_COMPRESS)
>> test looks for stateless operations only, so can we add perf test type like: test
>> type perf, op type:STATELESS/STATEFUL
>Are you asking for the tool to support stateful ops? Since no drivers support stateful yet
>We just wanted to ensure current driver functionality was covered with this first version.
Since it's an app so should be generic enough to be extensible for stateful benchmarking.
So, either we name app as test_comp_benchmark_statless or we make it generic to handling both, would be my suggestion.

Thanks
Shally
>
>>Also, why do we need --max-num-
>> sgl-segs as an input option from user? Shouldn't input_sz and seg_sz
>> internally decide on num-segs?
>> Or is it added to serve some other different purpose?
>Will have to get back to you on this one, seems illogical to get this input from user,
>But I will have to do further investigation to find if there was a different purpose.
>>
>> Thanks
>> Shally
>>
>Thanks for the feedback,
>We hope to get V2 sent asap.
>