On 7/6/2020 8:11 PM, Honnappa Nagarahalli wrote:
> Hi Ferruh,
> Thanks for the review comments
>
> <snip>
>
>> Subject: Re: [PATCH v2 5/5] app/testpmd: enable empty polls in burst stats
>>
>> On 6/26/2020 11:09 PM, Honnappa Nagarahalli wrote:
>>> The number of empty polls provides information about available CPU
>>> head room in the presence of continuous polling.
>>
>> +1 to have it, it can be useful.
>>
>>>
>>> Signed-off-by: Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>
>>> Reviewed-by: Phil Yang <phil.y...@arm.com>
>>> Reviewed-by: Ruifeng Wang <ruifeng.w...@arm.com>
>>> Tested-by: Phil Yang <phil.y...@arm.com>
>>> Tested-by: Ali Alnubani <alia...@mellanox.com>
>>
>> <...>
>>
>>> /*
>>> * First compute the total number of packet bursts and the
>>> * two highest numbers of bursts of the same number of packets.
>>> */
>>> total_burst = 0;
>>> - burst_stats[0] = burst_stats[1] = burst_stats[2] = 0;
>>> - pktnb_stats[0] = pktnb_stats[1] = pktnb_stats[2] = 0;
>>> + memset(&burst_stats, 0x0, sizeof(burst_stats));
>>> + memset(&pktnb_stats, 0x0, sizeof(pktnb_stats));
>>> +
>>> for (nb_pkt = 0; nb_pkt < MAX_PKT_BURST; nb_pkt++) {
>>> nb_burst = pbs->pkt_burst_spread[nb_pkt];
>>> - if (nb_burst == 0)
>>> - continue;
>>> total_burst += nb_burst;
>>> - if (nb_burst > burst_stats[0]) {
>>> - burst_stats[1] = burst_stats[0];
>>> - pktnb_stats[1] = pktnb_stats[0];
>>> +
>>> + /* Show empty poll stats always */
>>> + if (nb_pkt == 0) {
>>> burst_stats[0] = nb_burst;
>>> pktnb_stats[0] = nb_pkt;
>>
>> just a minor issue but this assignment is not needed.
> The empty poll stats are being shown always (even if there were no empty
> polls). The check 'nb_pkts == 0' indicates that the burst stats are for empty
> polls. Hence this assignment is required. But, this can be moved out of the
> loop to provide clarity. I will do that.
It is not required because of 'memset' above, we know 'nb_pkt == 0' and we know
'pktnb_stats[0] == 0', so "pktnb_stats[0] = nb_pkt;" is redundant.
>
>>
>>> - } else if (nb_burst > burst_stats[1]) {
>>> + continue;
>>> + }
>>> +
>>> + if (nb_burst == 0)
>>> + continue;
>>
>> again another minor issue, can have this check above 'nb_pkt == 0', to
>> prevent
>> unnecssary "burst_stats[0] = nb_burst;" assignment if "nb_burst == 0".
> The empty polls are always shown, even if there were 0% empty polls.
I got that, again because of memset, "burst_stats[0] == 0", you can skip
"burst_stats[0] = nb_burst;" in case "nb_burst == 0", that is why you can move
it up.
Anyway, both are trivial issues ...
>
>>
>> <...>
>>
>> Reviewed-by: Ferruh Yigit <ferruh.yi...@intel.com>
