From: "Toke Høiland-Jørgensen" <toke@redhat•com>
To: 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 19:47:17 +0100 [thread overview]
Message-ID: <87ee72ah56.fsf@toke.dk> (raw)
In-Reply-To: <20211126100611.514df099@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Jakub Kicinski <kuba@kernel•org> writes:
> On Fri, 26 Nov 2021 13:30:16 +0100 Toke Høiland-Jørgensen wrote:
>> >> 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!
>
> Fair. In all honesty I said that hoping to push for a more flexible
> approach hidden entirely in BPF, and not involving driver changes.
> Assuming the XDP program has more fine grained stats we should be able
> to extract those instead of double-counting. Hence my vague "let's work
> with apps" comment.
>
> For example to a person familiar with the workload it'd be useful to
> know if program returned XDP_DROP because of configured policy or
> failure to parse a packet. I don't think that sort distinction is
> achievable at the level of standard stats.
>
> The information required by the admin is higher level. As you say the
> primary concern there is "how many packets did XDP eat".
Right, sure, I am also totally fine with having only a somewhat
restricted subset of stats available at the interface level and make
everything else be BPF-based. I'm hoping we can converge of a common
understanding of what this "minimal set" should be :)
> Speaking of which, one thing that badly needs clarification is our
> expectation around XDP packets getting counted towards the interface
> stats.
Agreed. My immediate thought is that "XDP packets are interface packets"
but that is certainly not what we do today, so not sure if changing it
at this point would break things?
>> > 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.
>
> Right, that's about the only thing I use it for as well. "Is the load
> evenly distributed?" But that's not XDP specific and not worth
> standardizing for, yet, IMO, because..
>
>> 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?
>
> we don't even know what constitutes a channel today. And that will
> become increasingly problematic as importance of application specific
> queues increases (zctap etc). IMO until the ontological gaps around
> queues are filled we should leave per-queue stats in ethtool -S.
Hmm, right, I see. I suppose that as long as the XDP packets show up in
one of the interface counters in ethtool -S, it's possible to answer the
load distribution issue, and any further debugging (say, XDP drops on a
certain queue due to CPU-based queue indexing on TX) can be delegated to
BPF-based tools...
-Toke
next prev parent reply other threads:[~2021-11-26 18:49 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
2021-11-26 18:06 ` Jakub Kicinski
2021-11-26 18:47 ` Toke Høiland-Jørgensen [this message]
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=87ee72ah56.fsf@toke.dk \
--to=toke@redhat$(echo .)com \
--cc=akiyano@amazon$(echo .)com \
--cc=alexandr.lobakin@intel$(echo .)com \
--cc=andrii@kernel$(echo .)org \
--cc=anthony.l.nguyen@intel$(echo .)com \
--cc=ast@kernel$(echo .)org \
--cc=avagin@gmail$(echo .)com \
--cc=bpf@vger$(echo .)kernel.org \
--cc=claudiu.manoil@nxp$(echo .)com \
--cc=cong.wang@bytedance$(echo .)com \
--cc=corbet@lwn$(echo .)net \
--cc=daniel@iogearbox$(echo .)net \
--cc=darinzon@amazon$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=dsahern@kernel$(echo .)org \
--cc=ecree.xilinx@gmail$(echo .)com \
--cc=habetsm.xilinx@gmail$(echo .)com \
--cc=hawk@kernel$(echo .)org \
--cc=ioana.ciornei@nxp$(echo .)com \
--cc=jasowang@redhat$(echo .)com \
--cc=jesse.brandeburg@intel$(echo .)com \
--cc=johannes.berg@intel$(echo .)com \
--cc=john.fastabend@gmail$(echo .)com \
--cc=kafai@fb$(echo .)com \
--cc=kpsingh@kernel$(echo .)org \
--cc=kuba@kernel$(echo .)org \
--cc=leon@kernel$(echo .)org \
--cc=linux-doc@vger$(echo .)kernel.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-rdma@vger$(echo .)kernel.org \
--cc=linux@armlinux$(echo .)org.uk \
--cc=lorenzo@kernel$(echo .)org \
--cc=maciej.fijalkowski@intel$(echo .)com \
--cc=michal.swiatkowski@linux$(echo .)intel.com \
--cc=mst@redhat$(echo .)com \
--cc=mw@semihalf$(echo .)com \
--cc=ndagan@amazon$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=ryazanov.s.a@gmail$(echo .)com \
--cc=saeedb@amazon$(echo .)com \
--cc=saeedm@nvidia$(echo .)com \
--cc=shayagr@amazon$(echo .)com \
--cc=songliubraving@fb$(echo .)com \
--cc=thomas.petazzoni@bootlin$(echo .)com \
--cc=virtualization@lists$(echo .)linux-foundation.org \
--cc=vladimir.oltean@nxp$(echo .)com \
--cc=yajun.deng@linux$(echo .)dev \
--cc=yhs@fb$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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