LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Alexander Lobakin <alexandr.lobakin@intel.com>
Cc: "David S. Miller" <davem@davemloft.net>,
	Jesse Brandeburg <jesse.brandeburg@intel.com>,
	Lukasz Czapnik <lukasz.czapnik@intel.com>,
	Marcin Kubiak <marcin.kubiak@intel.com>,
	Michal Kubiak <michal.kubiak@intel.com>,
	Michal Swiatkowski <michal.swiatkowski@intel.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Netanel Belgazal <netanel@amazon.com>,
	Arthur Kiyanovski <akiyano@amazon.com>,
	Saeed Bishara <saeedb@amazon.com>,
	Ioana Ciornei <ioana.ciornei@nxp.com>,
	Claudiu Manoil <claudiu.manoil@nxp.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Marcin Wojtas <mw@semihalf.com>,
	Russell King <linux@armlinux.org.uk>,
	Edward Cree <ecree.xilinx@gmail.com>,
	Martin Habets <habetsm.xilinx@gmail.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Jason Wang <jasowang@redhat.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	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>,
	Shay Agroskin <shayagr@amazon.com>,
	Alexander Duyck <alexanderduyck@fb.com>,
	Danielle Ratson <danieller@nvidia.com>,
	Ido Schimmel <idosch@nvidia.com>, Andrew Lunn <andrew@lunn.ch>,
	Arnd Bergmann <arnd@arndb.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	Jian Shen <shenjian15@huawei.com>,
	Petr Vorel <petr.vorel@gmail.com>, Yangbo Lu <yangbo.lu@nxp.com>,
	Michal Kubecek <mkubecek@suse.cz>,
	Zheng Yongjun <zhengyongjun3@huawei.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	YueHaibing <yuehaibing@huawei.com>,
	Johannes Berg <johannes@sipsolutions.net>,
	netdev@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	virtualization@lists.linux-foundation.org, bpf@vger.kernel.org
Subject: Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics
Date: Wed, 4 Aug 2021 09:57:16 -0700	[thread overview]
Message-ID: <20210804095716.35387fcd@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <20210804155327.337-1-alexandr.lobakin@intel.com>

On Wed,  4 Aug 2021 17:53:27 +0200 Alexander Lobakin wrote:
> From: Jakub Kicinski <kuba@kernel.org>
> Date: Wed, 4 Aug 2021 05:36:50 -0700
> 
> > On Tue, 03 Aug 2021 16:57:22 -0700 Saeed Mahameed wrote:  
> > > On Tue, 2021-08-03 at 13:49 -0700, Jakub Kicinski wrote:  
> > > > 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.  
> 
> Well, 20+ patches in the series with at least half of them is
> drivers conversion. Plus mlx5. Plus we'll about to land XDP
> statistics for all Intel drivers, just firstly need to get a
> common infra for them (the purpose of this series).

Great, do you have impact of the stats on Intel drivers?
(Preferably from realistic scenarios where CPU cache is actually 
under pressure, not { return XDP_PASS; }). Numbers win arguments.

> Also, introducing IEEE and rmon stats didn't make a statement that
> all drivers should really expose them, right?

That's not relevant. IEEE and RMON stats are read from HW, they have 
no impact on the SW fast path.

> > > There are stats "mainly errors*"  that are not even visible or reported
> > > to the user prog,   
> 
> Not really. Many drivers like to count the number of redirects,
> xdp_xmits and stuff (incl. mlx5). Nevertheless, these stats aren't
> the same as something you can get from inside an XDP prog, right.
> 
> > Fair point, exceptions should not be performance critical.
> >   
> > > for that i had an idea in the past to attach an
> > > exception_bpf_prog provided by the user, where driver/stack will report
> > > errors to this special exception_prog.  
> > 
> > Or maybe we should turn trace_xdp_exception() into a call which
> > unconditionally collects exception stats? I think we can reasonably
> > expect the exception_bpf_prog to always be attached, right?  
> 
> trace_xdp_exception() is again a error path, and would restrict us
> to have only "bad" statistics.
> 
> > > > > 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.  
> 
> I was trying to introduce as few functional changes as possible,
> including that all the current drivers expose XDP stats through
> Ethtool.

You know this, but for the benefit of others - ethtool -S does not 
dump standard stats from the netlink API, and ethtool -S --goups does
not dump "old" stats. So users will need to use different commands
to get to the two, anyway.

> I don't say it's a 100% optimal way, but lots of different scripts
> and monitoring tools are already based on this fact and there can
> be some negative impact. There'll be for sure due to that std stats
> is a bit different thing and different drivers count and name XDP
> stats differently (breh).

That's concerning. I'd much rather you didn't convert all the drivers
than convert them before someone makes 100% sure the meaning of the
stats is equivalent.

> BTW, I'm fine with rtnl xstats. A nice reminder, thanks. If there
> won't be much cons like "don't touch our Ethtool stats", I would
> prefer this one instead of Ethtool standard stats way.

You'll have to leave the ethtool -S ones in place anyway, right?
New drivers would not include them but I don't think there's much
we can (or should) do for the existing ones.

> > > XDP is going to always be eBPF based ! why not just report such stats
> > > to a special BPF_MAP ? BPF stack can collect the stats from the driver
> > > and report them to this special MAP upon user request.  
> > 
> > Do you mean replacing the ethtool-netlink / rtnetlink etc. with
> > a new BPF_MAP? I don't think adding another category of uAPI thru 
> > which netdevice stats are exposed would do much good :( Plus it 
> > doesn't address the "yet another cacheline" concern.  
> 
> + this makes obtaining/tracking the statistics much harder. For now,
> all you need is `ethtool -S devname` (mainline) or
> `ethtool -S devname --groups xdp` (this series), and obtaining rtnl
> xstats is just a different command to invoke. BPF_MAP-based stats
> are a completely different story then.
> 
> > To my understanding the need for stats recognizes the fact that (in
> > large organizations) fleet monitoring is done by different teams than
> > XDP development. So XDP team may have all the stats they need, but the
> > team doing fleet monitoring has no idea how to get to them.
> > 
> > To bridge the two worlds we need a way for the infra team to ask the
> > XDP for well-defined stats. Maybe we should take a page from the BPF
> > iterators book and create a program type for bridging the two worlds?
> > Called by networking core when duping stats to extract from the
> > existing BPF maps all the relevant stats and render them into a well
> > known struct? Users' XDP design can still use a single per-cpu map with
> > all the stats if they so choose, but there's a way to implement more
> > optimal designs and still expose well-defined stats.
> > 
> > Maybe that's too complex, IDK.  
> 
> Thanks,
> Al


  reply	other threads:[~2021-08-04 16:57 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-03 16:36 [PATCH net-next 00/21] ethtool, stats: introduce and use standard XDP stats Alexander Lobakin
2021-08-03 16:36 ` [PATCH net-next 01/21] ethtool, stats: use a shorthand pointer in stats_prepare_data() Alexander Lobakin
2021-08-03 16:36 ` [PATCH net-next 02/21] ethtool, stats: add compile-time checks for standard stats Alexander Lobakin
2021-08-03 16:36 ` [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics Alexander Lobakin
2021-08-03 20:49   ` Jakub Kicinski
2021-08-03 23:57     ` Saeed Mahameed
2021-08-04 12:36       ` Jakub Kicinski
2021-08-04 15:53         ` Alexander Lobakin
2021-08-04 16:57           ` Jakub Kicinski [this message]
2021-08-05 11:18             ` Alexander Lobakin
2021-08-05 13:31               ` Jakub Kicinski
2021-08-04 16:17         ` David Ahern
2021-08-04 16:44           ` Jakub Kicinski
2021-08-04 17:28             ` David Ahern
2021-08-04 18:27               ` Saeed Mahameed
2021-08-05  0:43                 ` David Ahern
2021-08-05  2:14                   ` Saeed Mahameed
2021-08-12 12:19             ` Jesper Dangaard Brouer
2021-10-26  9:23       ` Alexander Lobakin
2021-11-05 16:44         ` Alexander Lobakin
2021-11-08 11:37           ` Toke Høiland-Jørgensen
2021-11-08 13:21             ` Alexander Lobakin
2021-11-08 18:09               ` Toke Høiland-Jørgensen
2021-08-03 16:36 ` [PATCH net-next 04/21] ethernet, dpaa2: simplify per-channel Ethtool stats counting Alexander Lobakin
2021-08-03 16:36 ` [PATCH net-next 05/21] ethernet, dpaa2: convert to standard XDP stats Alexander Lobakin
2021-08-03 16:36 ` [PATCH net-next 06/21] ethernet, ena: constify src and syncp args of ena_safe_update_stat() Alexander Lobakin
2021-08-03 16:36 ` [PATCH net-next 07/21] ethernet, ena: convert to standard XDP stats Alexander Lobakin
2021-08-04 13:04   ` Shay Agroskin
2021-08-04 15:24     ` Alexander Lobakin
2021-08-03 16:36 ` [PATCH net-next 08/21] ethernet, enetc: " Alexander Lobakin
2021-08-03 16:36 ` [PATCH net-next 09/21] ethernet, mvneta: rename xdp_xmit_err to xdp_xmit_drops Alexander Lobakin
2021-08-03 16:36 ` [PATCH net-next 10/21] ethernet, mvneta: convert to standard XDP stats Alexander Lobakin
2021-08-03 16:36 ` [PATCH net-next 11/21] ethernet, mvpp2: rename xdp_xmit_err to xdp_xmit_drops Alexander Lobakin
2021-08-03 16:36 ` [PATCH net-next 12/21] ethernet, mvpp2: convert to standard XDP stats Alexander Lobakin
2021-08-03 16:36 ` [PATCH net-next 13/21] ethernet, sfc: " Alexander Lobakin
2021-08-03 17:59   ` Edward Cree
2021-08-03 16:36 ` [PATCH net-next 14/21] veth: rename rx_drops to xdp_errors Alexander Lobakin
2021-08-03 16:36 ` [PATCH net-next 15/21] veth: rename xdp_xmit_errors to xdp_xmit_drops Alexander Lobakin
2021-08-03 16:36 ` [PATCH net-next 16/21] veth: rename drop xdp_ suffix from packets and bytes stats Alexander Lobakin
2021-08-03 16:36 ` [PATCH net-next 17/21] veth: convert to standard XDP stats Alexander Lobakin
2021-08-03 16:36 ` [PATCH net-next 18/21] virtio-net: rename xdp_tx{,__drops} SQ stats to xdp_xmit{,__drops} Alexander Lobakin
2021-08-03 16:42 ` Alexander Lobakin
2021-08-03 16:42 ` [PATCH net-next 19/21] virtio-net: don't mix error-caused drops with XDP_DROP cases Alexander Lobakin
2021-08-03 16:42 ` [PATCH net-next 20/21] virtio-net: convert to standard XDP stats Alexander Lobakin
2021-08-03 16:42 ` [PATCH net-next 21/21] Documentation, ethtool-netlink: update standard statistics documentation Alexander Lobakin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210804095716.35387fcd@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com \
    --to=kuba@kernel.org \
    --cc=akiyano@amazon.com \
    --cc=akpm@linux-foundation.org \
    --cc=alexanderduyck@fb.com \
    --cc=alexandr.lobakin@intel.com \
    --cc=andrew@lunn.ch \
    --cc=andrii@kernel.org \
    --cc=arnd@arndb.de \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=claudiu.manoil@nxp.com \
    --cc=corbet@lwn.net \
    --cc=daniel@iogearbox.net \
    --cc=danieller@nvidia.com \
    --cc=davem@davemloft.net \
    --cc=ecree.xilinx@gmail.com \
    --cc=habetsm.xilinx@gmail.com \
    --cc=hawk@kernel.org \
    --cc=hkallweit1@gmail.com \
    --cc=idosch@nvidia.com \
    --cc=ioana.ciornei@nxp.com \
    --cc=jasowang@redhat.com \
    --cc=jesse.brandeburg@intel.com \
    --cc=johannes@sipsolutions.net \
    --cc=john.fastabend@gmail.com \
    --cc=kafai@fb.com \
    --cc=kpsingh@kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=lukasz.czapnik@intel.com \
    --cc=marcin.kubiak@intel.com \
    --cc=michal.kubiak@intel.com \
    --cc=michal.swiatkowski@intel.com \
    --cc=mkubecek@suse.cz \
    --cc=mst@redhat.com \
    --cc=mw@semihalf.com \
    --cc=netanel@amazon.com \
    --cc=netdev@vger.kernel.org \
    --cc=petr.vorel@gmail.com \
    --cc=saeedb@amazon.com \
    --cc=shayagr@amazon.com \
    --cc=shenjian15@huawei.com \
    --cc=songliubraving@fb.com \
    --cc=thomas.petazzoni@bootlin.com \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=yangbo.lu@nxp.com \
    --cc=yhs@fb.com \
    --cc=yuehaibing@huawei.com \
    --cc=zhengyongjun3@huawei.com \
    --subject='Re: [PATCH net-next 03/21] ethtool, stats: introduce standard XDP statistics' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).