LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Saeed Mahameed <saeed@kernel.org>
To: David Ahern <dsahern@gmail.com>, Jakub Kicinski <kuba@kernel.org>,
	Tariq Toukan <ttoukan.linux@gmail.com>,
	Tariq Toukan <tariqt@nvidia.com>
Cc: Alexander Lobakin <alexandr.lobakin@intel.com>,
	"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>,
	Guy Tzalik <gtzalik@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>,
	Sameeh Jubran <sameehj@amazon.com>,
	Alexander Duyck <alexanderduyck@fb.com>,
	Danielle Ratson <danieller@nvidia.com>,
	Ido Schimmel <idosch@nvidia.com>, Andrew Lunn <andrew@lunn.ch>,
	Vladyslav Tarasiuk <vladyslavt@nvidia.com>,
	Arnd Bergmann <arnd@arndb.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	Jian Shen <shenjian15@huawei.com>,
	Petr Vorel <petr.vorel@gmail.com>, Dan Murphy <dmurphy@ti.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, 04 Aug 2021 11:27:09 -0700	[thread overview]
Message-ID: <11091d33ff7803257e38ee921e4ba9597acfccfc.camel@kernel.org> (raw)
In-Reply-To: <d21933cb-9d24-9bdd-cf18-e5077796ddf7@gmail.com>

On Wed, 2021-08-04 at 11:28 -0600, David Ahern wrote:
> On 8/4/21 10:44 AM, Jakub Kicinski wrote:
> > On Wed, 4 Aug 2021 10:17:56 -0600 David Ahern wrote:
> > > On 8/4/21 6:36 AM, Jakub Kicinski wrote:
> > > > > 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.
> > > > 
> > > > 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.  
> > > 

The main question here, do we want the prog to count or driver ? 
and the answer will lead to more questions :) :

1) will the prog/user need to access driver for driver only stats ? or
driver shall report to a special program and all the collection and
reporting is done in XDP/BPF internally .. 
2) stats per prog/queue/cpu/interface ? 
3) how to eventually report to user ethtool/ip -s/bpftool ?

too complex, IDK too .. :D


> > > I was just explaining to someone internally how to get stats at
> > > all of
> > > the different points in the stack to track down reasons for
> > > dropped packets:
> > > 
> > > ethtool -S for h/w and driver
> > > tc -s for drops by the qdisc
> > > /proc/net/softnet_stat for drops at the backlog layer
> > > netstat -s for network and transport layer
> > > 
> > > yet another command and API just adds to the nightmare of
> > > explaining and
> > > understanding these stats.
> > 
> > Are you referring to RTM_GETSTATS when you say "yet another
> > command"?
> > RTM_GETSTATS exists and is used by offloads today.
> > 
> > I'd expect ip -s (-s) to be extended to run GETSTATS and display
> > the xdp
> > stats. (Not sure why ip -s was left out of your list :))
> 
> It's on my diagram, and yes, forgot to add it here.
> 

i think ip -s is a good place for "standard" driver based xdp stats.
but as Jakub already explained, adding such driver mechanism is like
making a statement that drivers must implement this.

> > 
> > > There is real value in continuing to use ethtool API for XDP
> > > stats. Not
> > > saying this reorg of the XDP stats is the right thing to do, only
> > > that
> > > the existing API has real user benefits.
> > 
> > RTM_GETSTATS is an existing API. New ethtool stats are intended to
> > be HW
> > stats. I don't want to go back to ethtool being a dumping ground
> > for all
> > stats because that's what the old interface encouraged.
> 
> driver stats are important too. e.g., mlx5's cache stats and per-
> queue
> stats.
> 

one could claim that mlx5 cache stats should move to page_pool and
per_queue stats should move to the stack.

> > 
> > > Does anyone have data that shows bumping a properly implemented
> > > counter
> > > causes a noticeable performance degradation and if so by how
> > > much? You
> > > mention 'yet another cacheline' but collecting stats on stack and
> > > incrementing the driver structs at the end of the napi loop
> > > should not
> > > have a huge impact versus the value the stats provide.
> > 
> > Not sure, maybe Jesper has some numbers. Maybe Intel folks do?
> 

A properly implemented counter that doesn't introduce new cache misses,
will hardly show any measurable difference, the only way to measure is
via instructions per packet.

usually the way we implement counters in mlx5 is that if this is the
fastest flow that we expect then we only increment the good counters
"packet++/drop++/redirect++" any slower path should include counters to
indicate the slower path and the effect of the new "slower" counters
will still be negligible as we already are at a higher instructions per
packet hence the slower path .. 

the only time you measure a difference is when you introduce new
counting on a counter-free flow, e.g page_pool ;)

> I just ran some quick tests with my setup and measured about 1.2%
> worst

1.2% is a lot ! what was the test ? what is the change ?

> case. Certainly not exhaustive. Perhaps Intel or Mellanox can provide
> numbers for their high speed nics - e.g. ConnectX-6 and a saturated
> host.
> 

let's define what are we testing first, there are multiple places we
need to check, Tariq will be exploring transitioning mlx5 cache to
page_pool with all the counters, maybe it is a good place to measure.. 

> > 
> > I'm just allergic to situations when there is a decision made and 
> > then months later patches are posted disregarding the decision, 
> > without analysis on why that decision was wrong. And while the
> > maintainer who made the decision is on vacation.
> > 
> 
> stats is one of the many sensitive topics. I have been consistent in
> defending the need to use existing APIs and tooling and not relying
> on
> XDP program writers to add the relevant stats and then provide
> whatever
> tool is needed to extract and print them. Standardization for
> fundamental analysis tools.





  reply	other threads:[~2021-08-04 18:27 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
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 [this message]
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=11091d33ff7803257e38ee921e4ba9597acfccfc.camel@kernel.org \
    --to=saeed@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=dmurphy@ti.com \
    --cc=dsahern@gmail.com \
    --cc=ecree.xilinx@gmail.com \
    --cc=gtzalik@amazon.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=kuba@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=sameehj@amazon.com \
    --cc=shayagr@amazon.com \
    --cc=shenjian15@huawei.com \
    --cc=songliubraving@fb.com \
    --cc=tariqt@nvidia.com \
    --cc=thomas.petazzoni@bootlin.com \
    --cc=ttoukan.linux@gmail.com \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=vladyslavt@nvidia.com \
    --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).