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

[Verma, Shally]

>-----Original Message-----
>From: Trahe, Fiona <fiona.trahe at intel.com>
>Sent: 10 November 2018 06:24
>To: Jozwiak, TomaszX <tomaszx.jozwiak at intel.com>; Verma, Shally <Shally.Verma at cavium.com>; dev at dpdk.org;
>akhil.goyal at nxp.com
>Cc: Trahe, Fiona <fiona.trahe at intel.com>
>Subject: RE: [dpdk-dev] [PATCH 2/3] app/compress-perf: add performance measurement
>
>External Email
>
>Hi Shally, Tomasz,
>
>> > >> >> >> >+       /* 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?
>> > >> >> >
>> > >> >> >TJ: You probably mean cap->window_size.increment = 0 (because
>> > >> >> >cap->window_size is a structure). In that case we check if
>> > >> >> >test_data->window_sz >=min and test_data->window_sz <= max
>> > only,
>> > >> >> because increment = 0 means (base on compression API) we have only
>> > >> >> one value of windows_size (no range is supported).
>> > >> >> But PMD can set min and max too 0 for such case.
>> > >> >
>> > >> >TJ: I can't see any issue in that case too. Maybe I don't understand
>> > >> >what you
>> > >> mean but the logic is as follow:
>> > >> >1)  if you pass '--window-sz  ...' param. into command line your
>> > >> >intention is to force that value of window size during test. We
>> > >> >check is this
>> > >> value is allow (by param_range_check() function).
>> > >> >2) if you plan to use default value - just don't pass '--window-sz'
>> > >> >param. in command line at all. In that case we get windows size from
>> > >> >window_size.max field, so if window_size.min= window_size.max=0
>> > >> test_data->window_sz will be zero, as well.
>> > >> >If you mean that behavior is not good - I will be grateful for other
>> > >> suggestions.
>> > >>
>> > >> This is fine. but I am thinking of 3rd case here:
>> > >> c) user pass window sz but PMD window_sz.min = max = 0, then user
>> > >> requested windowsz is not applicable right?!
>> > >
>> > >In that case - true. There'll be fail :
>> > >"Compress device does not support this window size\n"); So what is your
>> > >proposal for  that case?
>> > >
>> > We can set to window size to implementation default and add in diagnostic
>> > of used window sz for test run.
>> > No need to fail here I believe.
>
>[Fiona] For Window size capability reported by the PMD in the info struct
>it is not valid to report min=0, max=0. The PMD must report the range it can
>handle - the API doesn't suggest otherwise.
>On the xform a specific window size is requested of the PMD, if it doesn't support
>this it's allowed to fall back to a lower size according to the API.
>However that doesn't mean the PMD can pick any size if it doesn't support the
>requested size, i.e. it can't pick a bigger size, just a smaller one.
>If an application requests a smaller window size
>than a PMD supports, it can be that the decompression engine
>will be unable to decompress if a larger window is used, so the PMD
>should only fall back to a smaller size.
>Based on above, I think the perf tool behaviour is ok.
>It should pass the user requested value to the PMD if the PMD capabilities support it.
Agree to this. However my point is what if PMD just leave these window sz values as 0, meaning implementation default i.e.
internally used fixed value used by PMD to lookup for both compression/decompression. But if we are not supporting  window sz = 0 on an API then its fine , no need to handle this special case. However given that, we need to add comment in capability field, PMD must set it to some non-zero value and 0 is not valid case to handle.

>If not it should fail. If the user wants to measure with a different window size they can
>pass in that parameter.
>The functional test suite can be used to validate the case where the PMD
>falls back - this is not what the perf tool is for.
>Does this make sense?
>
>@Shally, do you think we need an API change to support an unlimited set of window sizes?
>If so can you explain why?
No.I don't intend to add support something like unlimited window sz as that isn't a known use-case. Also, I didn't mean window sz = 0  to be interpreted as unlimited window sz. I just meant 0 = implementation default window sz , if that's supported on compression spec. 

Thanks
Shally
>