Re: [PATCH bpf] samples: bpf: fix summary per-sec stats in xdp_sample_user

[Kumar Kartikeya Dwivedi]

On Fri, Nov 12, 2021 at 03:27:03AM IST, Alexander Lobakin wrote:
> sample_summary_print() uses accumulated period to calculate and
> display per-sec averages. This period gets incremented by sampling
> interval each time a new sample is formed, and thus equals to the
> number of samples collected multiplied by this interval.
> However, the totals are being calculated differently, they receive
> current sample statistics already divided by the interval gotten as
> a difference between sample timestamps for better precision -- in
> other words, they are being incremented by the per-sec values each
> sample.
> This leads to the excessive division of summary per-secs when
> interval != 1 sec. It is obvious pps couldn't become two times
> lower just from picking a different sampling interval value:
>
> $ samples/bpf/xdp_redirect_cpu -p xdp_prognum_n1_inverse_qnum -c all
>   -s -d 6 -i 1
> < snip >
>   Packets received    : 2,197,230,321
>   Average packets/s   : 22,887,816
>   Packets redirected  : 2,197,230,472
>   Average redir/s     : 22,887,817
> $ samples/bpf/xdp_redirect_cpu -p xdp_prognum_n1_inverse_qnum -c all
>   -s -d 6 -i 2
> < snip >
>   Packets received    : 159,566,498
>   Average packets/s   : 11,397,607
>   Packets redirected  : 159,566,995
>   Average redir/s     : 11,397,642
>
> This can be easily fixed by treating the divisor not as a period,
> but rather as a total number of samples, and thus incrementing it
> by 1 instead of interval. As a nice side effect, we can now remove
> so-named argument from a couple of functions. Let us also create
> an "alias" for sample_output::rx_cnt::pps named 'num' using a union
> since this field is used to store this number (period previously)
> as well, and the resulting counter-intuitive code might've been
> a reason for this bug.
>
> Fixes: 156f886cf697 ("samples: bpf: Add basic infrastructure for XDP samples")
> Signed-off-by: Alexander Lobakin <alexandr.lobakin@xxxxxxxxx>
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@xxxxxxxxx>
> ---

Ouch. Thank you for the fix.

Reviewed-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>

> [...]

--
Kartikeya