Message ID | 20200911195258.1048468-8-kuba@kernel.org |
---|---|
State | Superseded |
Delegated to: | David Miller |
Headers | show |
Series | ethtool: add pause frame stats | expand |
On Fri, Sep 11, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote: > > Report standard pause frame stats. They are already aggregated > in struct ixgbe_hw_stats. > > The combination of the registers is suggested as equivalent to > PAUSEMACCtrlFramesTransmitted / PAUSEMACCtrlFramesReceived > by the Intel 82576EB datasheet, I could not find any information > in the HW actually supported by ixgbe. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c > index 71ec908266a6..a280aa34ca1d 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c > @@ -531,6 +531,16 @@ static int ixgbe_set_link_ksettings(struct net_device *netdev, > return err; > } > > +static void ixgbe_get_pause_stats(struct net_device *netdev, > + struct ethtool_pause_stats *stats) > +{ > + struct ixgbe_adapter *adapter = netdev_priv(netdev); > + struct ixgbe_hw_stats *hwstats = &adapter->stats; > + > + stats->tx_pause_frames = hwstats->lxontxc + hwstats->lxofftxc; > + stats->rx_pause_frames = hwstats->lxonrxc + hwstats->lxoffrxc; > +} > + > static void ixgbe_get_pauseparam(struct net_device *netdev, > struct ethtool_pauseparam *pause) > { > @@ -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. Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com>
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!
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? > > > Reviewed-by: Alexander Duyck <alexander.h.duyck@linux.intel.com> > > Thanks! >
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.
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c index 71ec908266a6..a280aa34ca1d 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c @@ -531,6 +531,16 @@ static int ixgbe_set_link_ksettings(struct net_device *netdev, return err; } +static void ixgbe_get_pause_stats(struct net_device *netdev, + struct ethtool_pause_stats *stats) +{ + struct ixgbe_adapter *adapter = netdev_priv(netdev); + struct ixgbe_hw_stats *hwstats = &adapter->stats; + + stats->tx_pause_frames = hwstats->lxontxc + hwstats->lxofftxc; + stats->rx_pause_frames = hwstats->lxonrxc + hwstats->lxoffrxc; +} + static void ixgbe_get_pauseparam(struct net_device *netdev, struct ethtool_pauseparam *pause) { @@ -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,
Report standard pause frame stats. They are already aggregated in struct ixgbe_hw_stats. The combination of the registers is suggested as equivalent to PAUSEMACCtrlFramesTransmitted / PAUSEMACCtrlFramesReceived by the Intel 82576EB datasheet, I could not find any information in the HW actually supported by ixgbe. Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c | 11 +++++++++++ 1 file changed, 11 insertions(+)