linux-kernel - Re: [PATCH v2 net-next 00/26] net: introduce and use generic XDP stats

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 30 Nov 2021 17:34:54 +0100
From:   Alexander Lobakin <alexandr.lobakin@...el.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Alexander Lobakin <alexandr.lobakin@...el.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Michal Swiatkowski <michal.swiatkowski@...ux.intel.com>,
        Maciej Fijalkowski <maciej.fijalkowski@...el.com>,
        Jonathan Corbet <corbet@....net>,
        Shay Agroskin <shayagr@...zon.com>,
        Arthur Kiyanovski <akiyano@...zon.com>,
        David Arinzon <darinzon@...zon.com>,
        Noam Dagan <ndagan@...zon.com>,
        Saeed Bishara <saeedb@...zon.com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Tony Nguyen <anthony.l.nguyen@...el.com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Marcin Wojtas <mw@...ihalf.com>,
        Russell King <linux@...linux.org.uk>,
        Saeed Mahameed <saeedm@...dia.com>,
        Leon Romanovsky <leon@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        John Fastabend <john.fastabend@...il.com>,
        Edward Cree <ecree.xilinx@...il.com>,
        Martin Habets <habetsm.xilinx@...il.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        KP Singh <kpsingh@...nel.org>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        Yajun Deng <yajun.deng@...ux.dev>,
        Sergey Ryazanov <ryazanov.s.a@...il.com>,
        David Ahern <dsahern@...nel.org>,
        Andrei Vagin <avagin@...il.com>,
        Johannes Berg <johannes.berg@...el.com>,
        Vladimir Oltean <vladimir.oltean@....com>,
        Cong Wang <cong.wang@...edance.com>, netdev@...r.kernel.org,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-rdma@...r.kernel.org, bpf@...r.kernel.org,
        virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH v2 net-next 00/26] net: introduce and use generic XDP stats

From: Jakub Kicinski <kuba@...nel.org>
Date: Tue, 30 Nov 2021 08:12:07 -0800

> On Tue, 30 Nov 2021 16:56:12 +0100 Alexander Lobakin wrote:
> > 3. XDP and XSK ctrs separately or not.
> > 
> > My PoV is that those are two quite different worlds.
> > However, stats for actions on XSK really make a little sense since
> > 99% of time we have xskmap redirect. So I think it'd be fine to just
> > expand stats structure with xsk_{rx,tx}_{packets,bytes} and count
> > the rest (actions, errors) together with XDP.
> > 
> > 
> > Rest:
> >  - don't create a separate `ip` command and report under `-s`;
> >  - save some RTNL skb space by skipping zeroed counters.
> 
> Let me ruin this point of clarity for you. I think that stats should 
> be skipped when they are not collected (see ETHTOOL_STAT_NOT_SET).
> If messages get large user should use the GETSTATS call and avoid 
> the problem more effectively.

Well, it was Dave's thought here: [0]

> Another thought on this patch: with individual attributes you could save
> some overhead by not sending 0 counters to userspace. e.g., define a
> helper that does:

I know about ETHTOOL_STAT_NOT_SET, but RTNL xstats doesn't use this,
does it?
GETSTATS is another thing, and I'll use it, thanks.

> 
> > Also, regarding that I count all on the stack and then add to the
> > storage once in a polling cycle -- most drivers don't do that and
> > just increment the values in the storage directly, but this can be
> > less performant for frequently updated stats (or it's just my
> > embedded past).
> > Re u64 vs u64_stats_t -- the latter is more universal and
> > architecture-friendly, the former is used directly in most of the
> > drivers primarily because those drivers and the corresponding HW
> > are being run on 64-bit systems in the vast majority of cases, and
> > Ethtools stats themselves are not so critical to guard them with
> > anti-tearing. Anyways, local64_t is cheap on ARM64/x86_64 I guess?

[0] https://lore.kernel.org/netdev/a4602b15-25b1-c388-73b4-1f97f6f0e555@gmail.com/

Al