[dpdk-dev] [PATCH v6] sched: make RED scaling configurable

[Alan Dewar]

On Tue, Jan 16, 2018 at 3:50 PM, Alan Dewar <alangordondewar at gmail.com> wrote:
> On Mon, Jan 15, 2018 at 4:52 PM, Stephen Hemminger
> <stephen at networkplumber.org> wrote:
>>
>> On Mon, 15 Jan 2018 16:16:09 +0000
>> alangordondewar at gmail.com wrote:
>>
>> Looks like a good idea, minor editing feedback.
>>
>>
>> > -     red_cfg->min_th = ((uint32_t) min_th) << (wq_log2 + RTE_RED_SCALING);
>> > -     red_cfg->max_th = ((uint32_t) max_th) << (wq_log2 + RTE_RED_SCALING);
>> > -     red_cfg->pa_const = (2 * (max_th - min_th) * maxp_inv) << RTE_RED_SCALING;
>> > +     red_cfg->min_th = ((uint32_t) min_th) << (wq_log2 + rte_red_scaling);
>> > +     red_cfg->max_th = ((uint32_t) max_th) << (wq_log2 + rte_red_scaling);
>>
>> While you are at it remove unnecessary parenthesis here.
>>
>
> Okay will do.

Ah - the compiler doesn't like it if I remove all the unnecessary
parenthesis. it gives the following error:

/home/adewar/git-repos/dpdk/lib/librte_sched/rte_red.c:153:49: error:
suggest parentheses around ‘+’ inside ‘<<’ [-Werror=parentheses]
  red_cfg->min_th = (uint32_t) min_th << wq_log2 + rte_red_scaling;

I'll reinstate the ones around the addition.

>
>> > +     red_cfg->pa_const = (2 * (max_th - min_th) * maxp_inv) <<
>> > +             rte_red_scaling;
>>
>> It reads easier if the the shift operator on the next line
>>
>>         red_cfg->pa_const = (2 * (max_th - min_th) * maxp_inv)
>>                  << rte_red_scaling;
>>
>
> Okay will do.
>
>> Why do functional tests have to be in same file and clutter the code?
>
> Do you mean the same patch file or the same unit-test file?
>
> I received feedback previously (might not have been this patch) where
> I had split the functional changes and the new unit-tests into
> separate patches and was asked to combine them into a single patch.  I
> like the idea of if you have the fix you also have the new unit-tests.
>
> As for having the new tests in the same file as existing tests, the
> red tests are table driven so most of the changes to the unit-test
> code are just adding new table entries to exercise the RED code with a
> different scaling factor.