mbox series

[net-next,0/7] ethtool: add pause frame stats

Message ID 20200911195258.1048468-1-kuba@kernel.org
Headers show
Series ethtool: add pause frame stats | expand

Message

Jakub Kicinski Sept. 11, 2020, 7:52 p.m. UTC 
Hi!

This is the first (small) series which exposes some stats via
the corresponding ethtool interface. Here (thanks to the
excitability of netlink) we expose pause frame stats via
the same interfaces as ethtool -a / -A.

In particular the following stats from the standard:
 - 30.3.4.2 aPAUSEMACCtrlFramesTransmitted
 - 30.3.4.3 aPAUSEMACCtrlFramesReceived

4 real drivers are converted, hopefully the semantics match
the standard.

Jakub Kicinski (8):
  ethtool: add standard pause stats
  docs: net: include the new ethtool pause stats in the stats doc
  netdevsim: add pause frame stats
  selftests: add a test for ethtool pause stats
  bnxt: add pause frame stats
  mlx5: add pause frame stats
  ixgbe: add pause frame stats
  mlx4: add pause frame stats

 Documentation/networking/ethtool-netlink.rst  |  11 ++
 Documentation/networking/statistics.rst       |  57 ++++++++-
 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  19 +++
 .../net/ethernet/intel/ixgbe/ixgbe_ethtool.c  |  11 ++
 .../net/ethernet/mellanox/mlx4/en_ethtool.c   |  19 +++
 .../net/ethernet/mellanox/mlx4/mlx4_stats.h   |  12 ++
 drivers/net/ethernet/mellanox/mlx5/core/en.h  |   2 +
 .../ethernet/mellanox/mlx5/core/en_ethtool.c  |  15 +++
 .../net/ethernet/mellanox/mlx5/core/en_rep.c  |   9 ++
 .../ethernet/mellanox/mlx5/core/en_stats.c    |  30 +++++
 .../ethernet/mellanox/mlx5/core/en_stats.h    |   3 +
 drivers/net/netdevsim/Makefile                |   2 +-
 drivers/net/netdevsim/ethtool.c               |  64 +++++++++++
 drivers/net/netdevsim/netdev.c                |   1 +
 drivers/net/netdevsim/netdevsim.h             |  11 ++
 include/linux/ethtool.h                       |  26 +++++
 include/uapi/linux/ethtool_netlink.h          |  18 ++-
 net/ethtool/pause.c                           |  57 ++++++++-
 .../drivers/net/netdevsim/ethtool-pause.sh    | 108 ++++++++++++++++++
 19 files changed, 467 insertions(+), 8 deletions(-)
 create mode 100644 drivers/net/netdevsim/ethtool.c
 create mode 100755 tools/testing/selftests/drivers/net/netdevsim/ethtool-pause.sh

Comments

Jakub Kicinski Sept. 11, 2020, 10:13 p.m. UTC | #1 
On Fri, 11 Sep 2020 14:12:50 -0700 Alexander Duyck wrote:
> On Fri, Sep 11, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > @@ -3546,6 +3556,7 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
> >         .set_eeprom             = ixgbe_set_eeprom,
> >         .get_ringparam          = ixgbe_get_ringparam,
> >         .set_ringparam          = ixgbe_set_ringparam,
> > +       .get_pause_stats        = ixgbe_get_pause_stats,
> >         .get_pauseparam         = ixgbe_get_pauseparam,
> >         .set_pauseparam         = ixgbe_set_pauseparam,
> >         .get_msglevel           = ixgbe_get_msglevel,  
> 
> So the count for this is simpler in igb than it is for ixgbe. I'm
> assuming you want just standard link flow control frames. If so then
> this patch is correct. Otherwise if you are wanting to capture
> priority flow control data then those are a seperate array of stats
> prefixed with a "p" instead of an "l". Otherwise this looks fine to
> me.

That's my interpretation, although I haven't found any place the
standard would address this directly. Non-PFC pause has a different
opcode, so I'm reasonably certain this makes sense.

BTW I'm not entirely clear on what "global PFC pause" is either.

Maybe someone can clarify? Mellanox folks?

> Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>

Thanks!
Jakub Kicinski Sept. 14, 2020, 4:20 p.m. UTC | #2 
On Sun, 13 Sep 2020 12:14:14 +0300 Ido Schimmel wrote:
> On Fri, Sep 11, 2020 at 03:13:43PM -0700, Jakub Kicinski wrote:
> > On Fri, 11 Sep 2020 14:12:50 -0700 Alexander Duyck wrote:  
> > > On Fri, Sep 11, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote:  
> > > > @@ -3546,6 +3556,7 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
> > > >         .set_eeprom             = ixgbe_set_eeprom,
> > > >         .get_ringparam          = ixgbe_get_ringparam,
> > > >         .set_ringparam          = ixgbe_set_ringparam,
> > > > +       .get_pause_stats        = ixgbe_get_pause_stats,
> > > >         .get_pauseparam         = ixgbe_get_pauseparam,
> > > >         .set_pauseparam         = ixgbe_set_pauseparam,
> > > >         .get_msglevel           = ixgbe_get_msglevel,    
> > > 
> > > So the count for this is simpler in igb than it is for ixgbe. I'm
> > > assuming you want just standard link flow control frames. If so then
> > > this patch is correct. Otherwise if you are wanting to capture
> > > priority flow control data then those are a seperate array of stats
> > > prefixed with a "p" instead of an "l". Otherwise this looks fine to
> > > me.  
> > 
> > That's my interpretation, although I haven't found any place the
> > standard would address this directly. Non-PFC pause has a different
> > opcode, so I'm reasonably certain this makes sense.
> > 
> > BTW I'm not entirely clear on what "global PFC pause" is either.
> > 
> > Maybe someone can clarify? Mellanox folks?  
> 
> I checked IEEE 802.1Qaz and could not find anything relevant. My only
> guess is that it might be a PFC frame with all the priorities set.
> 
> Where did you see it?

I think I saw it in MLX5 and I thought it's something
implementation-specific. But then I noticed 802.1-2018 
has a ieee8021PfcGlobalReqdGroup group.