>-----Original Message-----
>From: Daly, Lee <lee.d...@intel.com>
>Sent: 15 October 2018 20:40
>To: Verma, Shally <shally.ve...@cavium.com>
>Cc: Jozwiak, TomaszX <tomaszx.jozw...@intel.com>; dev@dpdk.org; Trahe, Fiona
><fiona.tr...@intel.com>; akhil.go...@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-boun...@dpdk.org] On Behalf Of Verma, Shally
>> Sent: Friday, October 12, 2018 11:16 AM
>> To: Jozwiak, TomaszX <tomaszx.jozw...@intel.com>; dev@dpdk.org; Trahe,
>> Fiona <fiona.tr...@intel.com>; akhil.go...@nxp.com; De Lara Guarch, Pablo
>> <pablo.de.lara.gua...@intel.com>
>> Cc: d...@dpdk.org; l...@dpdk.org; gua...@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.
>
