netdev - Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP 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: <20210803134900.578b4c37@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Tue, 3 Aug 2021 13:49:00 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Alexander Lobakin <alexandr.lobakin@...el.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jesse Brandeburg <jesse.brandeburg@...el.com>,
        Lukasz Czapnik <lukasz.czapnik@...el.com>,
        Marcin Kubiak <marcin.kubiak@...el.com>,
        Michal Kubiak <michal.kubiak@...el.com>,
        Michal Swiatkowski <michal.swiatkowski@...el.com>,
        Jonathan Corbet <corbet@....net>,
        Netanel Belgazal <netanel@...zon.com>,
        Arthur Kiyanovski <akiyano@...zon.com>,
        Guy Tzalik <gtzalik@...zon.com>,
        Saeed Bishara <saeedb@...zon.com>,
        Ioana Ciornei <ioana.ciornei@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
        Marcin Wojtas <mw@...ihalf.com>,
        Russell King <linux@...linux.org.uk>,
        Edward Cree <ecree.xilinx@...il.com>,
        Martin Habets <habetsm.xilinx@...il.com>,
        "Michael S. Tsirkin" <mst@...hat.com>,
        Jason Wang <jasowang@...hat.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        John Fastabend <john.fastabend@...il.com>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        KP Singh <kpsingh@...nel.org>,
        Shay Agroskin <shayagr@...zon.com>,
        Sameeh Jubran <sameehj@...zon.com>,
        Alexander Duyck <alexanderduyck@...com>,
        Danielle Ratson <danieller@...dia.com>,
        Ido Schimmel <idosch@...dia.com>, Andrew Lunn <andrew@...n.ch>,
        Vladyslav Tarasiuk <vladyslavt@...dia.com>,
        Arnd Bergmann <arnd@...db.de>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Jian Shen <shenjian15@...wei.com>,
        Petr Vorel <petr.vorel@...il.com>, Dan Murphy <dmurphy@...com>,
        Yangbo Lu <yangbo.lu@....com>,
        Michal Kubecek <mkubecek@...e.cz>,
        Zheng Yongjun <zhengyongjun3@...wei.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        YueHaibing <yuehaibing@...wei.com>,
        Johannes Berg <johannes@...solutions.net>,
        netdev@...r.kernel.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        virtualization@...ts.linux-foundation.org, bpf@...r.kernel.org
Subject: Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP
 statistics

On Tue,  3 Aug 2021 18:36:23 +0200 Alexander Lobakin wrote:
> Most of the driver-side XDP enabled drivers provide some statistics
> on XDP programs runs and different actions taken (number of passes,
> drops, redirects etc.).

Could you please share the statistics to back that statement up?
Having uAPI for XDP stats is pretty much making the recommendation 
that drivers should implement such stats. The recommendation from
Alexei and others back in the day (IIRC) was that XDP programs should
implement stats, not the drivers, to avoid duplication.

> Regarding that it's almost pretty the same across all the drivers
> (which is obvious), we can implement some sort of "standardized"
> statistics using Ethtool standard stats infra to eliminate a lot
> of code and stringsets duplication, different approaches to count
> these stats and so on.

I'm not 100% sold on the fact that these should be ethtool stats. 
Why not rtnl_fill_statsinfo() stats? Current ethtool std stats are 
all pretty Ethernet specific, and all HW stats. Mixing HW and SW stats
is what we're trying to get away from.

> These new 12 fields provided by the standard XDP stats should cover
> most, if not all, stats that might be interesting for collecting and
> tracking.
> Note that most NIC drivers keep XDP statistics on a per-channel
> basis, so this also introduces a new callback for getting a number
> of channels which a driver will provide stats for. If it's not
> implemented or returns 0, it means stats are global/device-wide.

Per-channel stats via std ethtool stats are not a good idea. Per queue
stats must be via the queue netlink interface we keep talking about for
ever but which doesn't seem to materialize. When stats are reported via
a different interface than objects they pertain to matching stats,
objects and their lifetime becomes very murky.