Date:   Fri, 12 Nov 2021 07:31:42 +0530
From:   Kumar Kartikeya Dwivedi <memxor@gmail.com>
To:     Alexander Lobakin <alexandr.lobakin@intel.com>
Cc:     Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Jesse Brandeburg <jesse.brandeburg@intel.com>,
        Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
        Michal Swiatkowski <michal.swiatkowski@intel.com>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        Jesper Dangaard Brouer <hawk@kernel.org>,
        John Fastabend <john.fastabend@gmail.com>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        KP Singh <kpsingh@kernel.org>, netdev@vger.kernel.org,
        bpf@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH bpf] samples: bpf: fix summary per-sec stats in
 xdp_sample_user
Message-ID: <20211112020142.q656zeu35qjtmvy5@apollo.localdomain>
References: <20211111215703.690-1-alexandr.lobakin@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20211111215703.690-1-alexandr.lobakin@intel.com>
Precedence: bulk

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@intel.com>
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---

Ouch. Thank you for the fix.

Reviewed-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>

> [...]

--
Kartikeya