netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Toke Høiland-Jørgensen" <toke@redhat.com>
To: Alexander Lobakin <alexandr.lobakin@intel.com>,
	Jakub Kicinski <kuba@kernel.org>
Cc: Alexander Lobakin <alexandr.lobakin@intel.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	"David S. Miller" <davem@davemloft.net>,
	Jesse Brandeburg <jesse.brandeburg@intel.com>,
	Michal Swiatkowski <michal.swiatkowski@linux.intel.com>,
	Maciej Fijalkowski <maciej.fijalkowski@intel.com>,
	Jonathan Corbet <corbet@lwn.net>,
	Shay Agroskin <shayagr@amazon.com>,
	Arthur Kiyanovski <akiyano@amazon.com>,
	David Arinzon <darinzon@amazon.com>,
	Noam Dagan <ndagan@amazon.com>, Saeed Bishara <saeedb@amazon.com>,
	Ioana Ciornei <ioana.ciornei@nxp.com>,
	Claudiu Manoil <claudiu.manoil@nxp.com>,
	Tony Nguyen <anthony.l.nguyen@intel.com>,
	Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
	Marcin Wojtas <mw@semihalf.com>,
	Russell King <linux@armlinux.org.uk>,
	Saeed Mahameed <saeedm@nvidia.com>,
	Leon Romanovsky <leon@kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Jesper Dangaard Brouer <hawk@kernel.org>,
	John Fastabend <john.fastabend@gmail.com>,
	Edward Cree <ecree.xilinx@gmail.com>,
	Martin Habets <habetsm.xilinx@gmail.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Jason Wang <jasowang@redhat.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>,
	Lorenzo Bianconi <lorenzo@kernel.org>,
	Yajun Deng <yajun.deng@linux.dev>,
	Sergey Ryazanov <ryazanov.s.a@gmail.com>,
	David Ahern <dsahern@kernel.org>, Andrei Vagin <avagin@gmail.com>,
	Johannes Berg <johannes.berg@intel.com>,
	Vladimir Oltean <vladimir.oltean@nxp.com>,
	Cong Wang <cong.wang@bytedance.com>,
	netdev@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org,
	bpf@vger.kernel.org, virtualization@lists.linux-foundation.org
Subject: Re: [PATCH v2 net-next 21/26] ice: add XDP and XSK generic per-channel statistics
Date: Fri, 26 Nov 2021 13:30:16 +0100	[thread overview]
Message-ID: <87sfvj9k13.fsf@toke.dk> (raw)
In-Reply-To: <20211125204007.133064-1-alexandr.lobakin@intel.com>

Alexander Lobakin <alexandr.lobakin@intel.com> writes:

> From: Jakub Kicinski <kuba@kernel.org>
> Date: Thu, 25 Nov 2021 09:44:40 -0800
>
>> 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.
>
> Right, just -s is fine, if we drop the per-channel approach.

I agree that adding them to -s is fine (and that resolves my "no one
will find them" complain as well). If it crowds the output we could also
default to only output'ing a subset, and have the more detailed
statistics hidden behind a verbose switch (or even just in the JSON
output)?

>> 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.
>
> The initial idea was to only uniform the drivers. But in general
> you are right, 10 drivers having something doesn't mean it's
> something good.

I don't think it's accurate to call the admin use case "hypothetical".
We're expending a significant effort explaining to people that XDP can
"eat" your packets, and not having any standard statistics makes this
way harder. We should absolutely cater to our "early adopters", but if
we want XDP to see wider adoption, making it "less weird" is critical!

> Maciej, I think you were talking about Cilium asking for those stats
> in Intel drivers? Could you maybe provide their exact usecases/needs
> so I'll orient myself? I certainly remember about XSK Tx packets and
> bytes.
> And speaking of XSK Tx, we have per-socket stats, isn't that enough?

IMO, as long as the packets are accounted for in the regular XDP stats,
having a whole separate set of stats only for XSK is less important.

>> 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.
>
> Oh okay. My very first iterations were without this, but then I
> found most of the drivers expose their XDP stats per-channel. Since
> I didn't plan to degrade the functionality, they went that way.

I personally find the per-channel stats quite useful. One of the primary
reasons for not achieving full performance with XDP is broken
configuration of packet steering to CPUs, and having per-channel stats
is a nice way of seeing this. I can see the point about them being way
too verbose in the default output, though, and I do generally filter the
output as well when viewing them. But see my point above about only
printing a subset of the stats by default; per-channel stats could be
JSON-only, for instance?

-Toke


  reply	other threads:[~2021-11-26 12:32 UTC|newest]

Thread overview: 76+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-11-23 16:39 [PATCH v2 net-next 00/26] net: introduce and use generic XDP stats Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 01/26] rtnetlink: introduce generic XDP statistics Alexander Lobakin
2021-11-30  2:36   ` David Ahern
2021-11-23 16:39 ` [PATCH v2 net-next 02/26] xdp: provide common driver helpers for implementing XDP stats Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 03/26] ena: implement generic XDP statistics callbacks Alexander Lobakin
2021-11-29 13:34   ` Shay Agroskin
2021-11-30 19:14     ` Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 04/26] dpaa2: implement generic XDP stats callbacks Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 05/26] enetc: " Alexander Lobakin
2021-11-23 17:09   ` Vladimir Oltean
2021-11-24 11:37     ` Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 06/26] mvneta: reformat mvneta_netdev_ops Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 07/26] mvneta: add .ndo_get_xdp_stats() callback Alexander Lobakin
2021-11-24 11:39   ` Russell King (Oracle)
2021-11-25 17:16     ` Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 08/26] mvpp2: provide " Alexander Lobakin
2021-11-24 11:33   ` Russell King (Oracle)
2021-11-24 11:36   ` Russell King (Oracle)
2021-11-23 16:39 ` [PATCH v2 net-next 09/26] mlx5: don't mix XDP_DROP and Rx XDP error cases Alexander Lobakin
2021-11-24 18:15   ` kernel test robot
2021-11-25 16:40     ` Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 10/26] mlx5: provide generic XDP stats callbacks Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 11/26] sf100, sfx: implement " Alexander Lobakin
2021-11-24  9:59   ` Edward Cree
2021-11-23 16:39 ` [PATCH v2 net-next 12/26] veth: don't mix XDP_DROP counter with Rx XDP errors Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 13/26] veth: drop 'xdp_' suffix from packets and bytes stats Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 14/26] veth: reformat veth_netdev_ops Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 15/26] veth: add generic XDP stats callbacks Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 16/26] virtio_net: don't mix XDP_DROP counter with Rx XDP errors Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 17/26] virtio_net: rename xdp_tx{,_drops} SQ stats to xdp_xmit{,_errors} Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 18/26] virtio_net: reformat virtnet_netdev Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 19/26] virtio_net: add callbacks for generic XDP stats Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 20/26] i40e: add XDP and XSK generic per-channel statistics Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 21/26] ice: " Alexander Lobakin
2021-11-24  0:52   ` Daniel Borkmann
2021-11-24 16:34     ` Lorenz Bauer
2021-11-25 11:56     ` Toke Høiland-Jørgensen
2021-11-25 17:07       ` Alexander Lobakin
2021-11-25 17:44         ` Jakub Kicinski
2021-11-25 20:40           ` Alexander Lobakin
2021-11-26 12:30             ` Toke Høiland-Jørgensen [this message]
2021-11-26 18:06               ` Jakub Kicinski
2021-11-26 18:47                 ` Toke Høiland-Jørgensen
2021-11-26 19:14                   ` Jakub Kicinski
2021-11-28 17:54                     ` Ido Schimmel
2021-11-29 14:47                       ` Jakub Kicinski
2021-11-29 15:51                         ` Petr Machata
2021-11-29 15:54                           ` Petr Machata
2021-11-29 16:05                           ` Jakub Kicinski
2021-11-29 17:08                             ` Petr Machata
2021-11-29 17:17                               ` Jakub Kicinski
2021-11-30 11:55                                 ` Petr Machata
2021-11-30 15:07                                   ` Jakub Kicinski
2021-11-26 22:27                 ` Daniel Borkmann
2021-11-26 23:01                   ` Daniel Borkmann
2021-11-29 13:59                     ` Jesper Dangaard Brouer
2021-11-29 15:03                       ` Jakub Kicinski
2021-11-29 11:51                   ` Toke Høiland-Jørgensen
2021-11-23 16:39 ` [PATCH v2 net-next 22/26] igb: add XDP " Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 23/26] igc: bail out early on XSK xmit if no descs are available Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 24/26] igc: add XDP and XSK generic per-channel statistics Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 25/26] ixgbe: " Alexander Lobakin
2021-11-23 16:39 ` [PATCH v2 net-next 26/26] Documentation: reflect generic XDP statistics Alexander Lobakin
2021-11-28 22:23 ` [PATCH v2 net-next 00/26] net: introduce and use generic XDP stats David Ahern
2021-11-30 15:56 ` Alexander Lobakin
2021-11-30 16:12   ` Jakub Kicinski
2021-11-30 16:34     ` Alexander Lobakin
2021-11-30 17:04       ` Jakub Kicinski
2021-11-30 17:38         ` David Ahern
2021-11-30 19:46           ` Jakub Kicinski
2021-12-01 15:21           ` Jamal Hadi Salim
2021-11-30 16:17   ` Toke Høiland-Jørgensen
2021-11-30 17:07     ` Jakub Kicinski
2021-11-30 17:56       ` David Ahern
2021-11-30 19:53         ` Jakub Kicinski
2021-11-30 17:45   ` David Ahern

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=87sfvj9k13.fsf@toke.dk \
    --to=toke@redhat.com \
    --cc=akiyano@amazon.com \
    --cc=alexandr.lobakin@intel.com \
    --cc=andrii@kernel.org \
    --cc=anthony.l.nguyen@intel.com \
    --cc=ast@kernel.org \
    --cc=avagin@gmail.com \
    --cc=bpf@vger.kernel.org \
    --cc=claudiu.manoil@nxp.com \
    --cc=cong.wang@bytedance.com \
    --cc=corbet@lwn.net \
    --cc=daniel@iogearbox.net \
    --cc=darinzon@amazon.com \
    --cc=davem@davemloft.net \
    --cc=dsahern@kernel.org \
    --cc=ecree.xilinx@gmail.com \
    --cc=habetsm.xilinx@gmail.com \
    --cc=hawk@kernel.org \
    --cc=ioana.ciornei@nxp.com \
    --cc=jasowang@redhat.com \
    --cc=jesse.brandeburg@intel.com \
    --cc=johannes.berg@intel.com \
    --cc=john.fastabend@gmail.com \
    --cc=kafai@fb.com \
    --cc=kpsingh@kernel.org \
    --cc=kuba@kernel.org \
    --cc=leon@kernel.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=lorenzo@kernel.org \
    --cc=maciej.fijalkowski@intel.com \
    --cc=michal.swiatkowski@linux.intel.com \
    --cc=mst@redhat.com \
    --cc=mw@semihalf.com \
    --cc=ndagan@amazon.com \
    --cc=netdev@vger.kernel.org \
    --cc=ryazanov.s.a@gmail.com \
    --cc=saeedb@amazon.com \
    --cc=saeedm@nvidia.com \
    --cc=shayagr@amazon.com \
    --cc=songliubraving@fb.com \
    --cc=thomas.petazzoni@bootlin.com \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=vladimir.oltean@nxp.com \
    --cc=yajun.deng@linux.dev \
    --cc=yhs@fb.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).