Re: [PATCH v2 net-next 21/26] ice: add XDP and XSK generic per-channel statistics

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



From: Jakub Kicinski <kuba@xxxxxxxxxx>
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.

> 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.

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?

> > > > +  xdp-channel0-rx_xdp_redirect: 7
> > > > +  xdp-channel0-rx_xdp_redirect_errors: 8
> > > > +  xdp-channel0-rx_xdp_tx: 9
> > > > +  xdp-channel0-rx_xdp_tx_errors: 10
> > > > +  xdp-channel0-tx_xdp_xmit_packets: 11
> > > > +  xdp-channel0-tx_xdp_xmit_bytes: 12
> > > > +  xdp-channel0-tx_xdp_xmit_errors: 13
> > > > +  xdp-channel0-tx_xdp_xmit_full: 14
> 
> 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.

Al



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux