netdev - Re: [PATCH v2 net-next 21/26] ice: add XDP and XSK generic per-channel statistics

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]

Message-ID: <20211125094440.6c402d63@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Thu, 25 Nov 2021 09:44:40 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>
Cc:     Toke Høiland-Jørgensen <toke@...hat.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        "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>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        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 21/26] ice: add XDP and XSK generic
 per-channel statistics

On Thu, 25 Nov 2021 18:07:08 +0100 Alexander Lobakin wrote:
> > This I agree with, and while I can see the layering argument for putting
> > them into 'ip' and rtnetlink instead of ethtool, I also worry that these
> > counters will simply be lost in obscurity, so I do wonder if it wouldn't
> > be better to accept the "layering violation" and keeping them all in the
> > 'ethtool -S' output?  
> 
> I don't think we should harm the code and the logics in favor of
> 'some of the users can face something'. We don't control anything
> related to XDP using Ethtool at all, but there is some XDP-related
> stuff inside iproute2 code, so for me it's even more intuitive to
> have them there.
> Jakub, may be you'd like to add something at this point?

TBH I wasn't following this thread too closely since I saw Daniel
nacked it already. I do prefer rtnl xstats, I'd just report them 
in -s if they are non-zero. But doesn't sound like we have an agreement
whether they should exist or not.

Can we think of an approach which would make cloudflare and cilium
happy? Feels like we're trying to make the slightly hypothetical 
admin happy while ignoring objections of very real users.

> > > +  xdp-channel0-rx_xdp_redirect: 7
> > > +  xdp-channel0-rx_xdp_redirect_errors: 8
> > > +  xdp-channel0-rx_xdp_tx: 9
> > > +  xdp-channel0-rx_xdp_tx_errors: 10
> > > +  xdp-channel0-tx_xdp_xmit_packets: 11
> > > +  xdp-channel0-tx_xdp_xmit_bytes: 12
> > > +  xdp-channel0-tx_xdp_xmit_errors: 13
> > > +  xdp-channel0-tx_xdp_xmit_full: 14

Please leave the per-channel stats out. They make a precedent for
channel stats which should be an attribute of a channel. Working for 
a large XDP user for a couple of years now I can tell you from my own
experience I've not once found them useful. In fact per-queue stats are
a major PITA as they crowd the output.