| Message ID | 20200911195258.1048468-6-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: > > These stats are already reported in ethtool -S. > Hopefully they are equivalent to standard stats? Yes. > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > --- > .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > index d0928334bdc8..b5de242766e3 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > @@ -1778,6 +1778,24 @@ static void bnxt_get_pauseparam(struct net_device *dev, > epause->tx_pause = !!(link_info->req_flow_ctrl & BNXT_LINK_PAUSE_TX); > } > > +static void bnxt_get_pause_stats(struct net_device *dev, > + struct ethtool_pause_stats *epstat) > +{ > + struct bnxt *bp = netdev_priv(dev); > + struct rx_port_stats *rx_stats; > + struct tx_port_stats *tx_stats; > + > + if (BNXT_VF(bp) || !(bp->flags & BNXT_FLAG_PORT_STATS)) > + return; > + > + rx_stats = (void *)bp->port_stats.sw_stats; > + tx_stats = (void *)((unsigned long)bp->port_stats.sw_stats + > + BNXT_TX_PORT_STATS_BYTE_OFFSET); > + > + epstat->rx_pause_frames = rx_stats->rx_pause_frames; > + epstat->tx_pause_frames = tx_stats->tx_pause_frames; This will work, but the types on the 2 sides don't match. On the right hand side, since you are casting to the hardware struct rx_port_stats and tx_port_stats, the types are __le64. If rx_stats and tx_stats are *u64 and you use these macros: BNXT_GET_RX_PORT_STATS64(rx_stats, rx_pause_frames) BNXT_GET_TX_PORT_STATS64(tx_stats, tx_pause_frames) the results will be the same with native CPU u64 types. Thanks.
On Fri, 11 Sep 2020 15:34:24 -0700 Michael Chan wrote: > On Fri, Sep 11, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > These stats are already reported in ethtool -S. > > Hopefully they are equivalent to standard stats? > > Yes. > > > > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > > --- > > .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > index d0928334bdc8..b5de242766e3 100644 > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > @@ -1778,6 +1778,24 @@ static void bnxt_get_pauseparam(struct net_device *dev, > > epause->tx_pause = !!(link_info->req_flow_ctrl & BNXT_LINK_PAUSE_TX); > > } > > > > +static void bnxt_get_pause_stats(struct net_device *dev, > > + struct ethtool_pause_stats *epstat) > > +{ > > + struct bnxt *bp = netdev_priv(dev); > > + struct rx_port_stats *rx_stats; > > + struct tx_port_stats *tx_stats; > > + > > + if (BNXT_VF(bp) || !(bp->flags & BNXT_FLAG_PORT_STATS)) > > + return; > > + > > + rx_stats = (void *)bp->port_stats.sw_stats; > > + tx_stats = (void *)((unsigned long)bp->port_stats.sw_stats + > > + BNXT_TX_PORT_STATS_BYTE_OFFSET); > > + > > + epstat->rx_pause_frames = rx_stats->rx_pause_frames; > > + epstat->tx_pause_frames = tx_stats->tx_pause_frames; > > This will work, but the types on the 2 sides don't match. On the > right hand side, since you are casting to the hardware struct > rx_port_stats and tx_port_stats, the types are __le64. > > If rx_stats and tx_stats are *u64 and you use these macros: > > BNXT_GET_RX_PORT_STATS64(rx_stats, rx_pause_frames) > BNXT_GET_TX_PORT_STATS64(tx_stats, tx_pause_frames) > > the results will be the same with native CPU u64 types. Thanks! My build bot just poked me about this as well. I don't see any byte swaps in bnxt_get_ethtool_stats() - are they not needed there? I'm slightly confused.
On Fri, 11 Sep 2020 15:43:43 -0700 Jakub Kicinski wrote: > > > + struct bnxt *bp = netdev_priv(dev); > > > + struct rx_port_stats *rx_stats; > > > + struct tx_port_stats *tx_stats; > > > + > > > + if (BNXT_VF(bp) || !(bp->flags & BNXT_FLAG_PORT_STATS)) > > > + return; > > > + > > > + rx_stats = (void *)bp->port_stats.sw_stats; > > > + tx_stats = (void *)((unsigned long)bp->port_stats.sw_stats + > > > + BNXT_TX_PORT_STATS_BYTE_OFFSET); > > > + > > > + epstat->rx_pause_frames = rx_stats->rx_pause_frames; > > > + epstat->tx_pause_frames = tx_stats->tx_pause_frames; > > > > This will work, but the types on the 2 sides don't match. On the > > right hand side, since you are casting to the hardware struct > > rx_port_stats and tx_port_stats, the types are __le64. > > > > If rx_stats and tx_stats are *u64 and you use these macros: > > > > BNXT_GET_RX_PORT_STATS64(rx_stats, rx_pause_frames) > > BNXT_GET_TX_PORT_STATS64(tx_stats, tx_pause_frames) > > > > the results will be the same with native CPU u64 types. > > Thanks! My build bot just poked me about this as well. > > I don't see any byte swaps in bnxt_get_ethtool_stats() - > are they not needed there? I'm slightly confused. Oof those macros don't byte swap either, even more confused now :)
On Fri, Sep 11, 2020 at 3:43 PM Jakub Kicinski <kuba@kernel.org> wrote: > > On Fri, 11 Sep 2020 15:34:24 -0700 Michael Chan wrote: > > On Fri, Sep 11, 2020 at 12:53 PM Jakub Kicinski <kuba@kernel.org> wrote: > > > > > > These stats are already reported in ethtool -S. > > > Hopefully they are equivalent to standard stats? > > > > Yes. > > > > > > > > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > > > --- > > > .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 19 +++++++++++++++++++ > > > 1 file changed, 19 insertions(+) > > > > > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > > index d0928334bdc8..b5de242766e3 100644 > > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c > > > @@ -1778,6 +1778,24 @@ static void bnxt_get_pauseparam(struct net_device *dev, > > > epause->tx_pause = !!(link_info->req_flow_ctrl & BNXT_LINK_PAUSE_TX); > > > } > > > > > > +static void bnxt_get_pause_stats(struct net_device *dev, > > > + struct ethtool_pause_stats *epstat) > > > +{ > > > + struct bnxt *bp = netdev_priv(dev); > > > + struct rx_port_stats *rx_stats; > > > + struct tx_port_stats *tx_stats; > > > + > > > + if (BNXT_VF(bp) || !(bp->flags & BNXT_FLAG_PORT_STATS)) > > > + return; > > > + > > > + rx_stats = (void *)bp->port_stats.sw_stats; > > > + tx_stats = (void *)((unsigned long)bp->port_stats.sw_stats + > > > + BNXT_TX_PORT_STATS_BYTE_OFFSET); > > > + > > > + epstat->rx_pause_frames = rx_stats->rx_pause_frames; > > > + epstat->tx_pause_frames = tx_stats->tx_pause_frames; > > > > This will work, but the types on the 2 sides don't match. On the > > right hand side, since you are casting to the hardware struct > > rx_port_stats and tx_port_stats, the types are __le64. > > > > If rx_stats and tx_stats are *u64 and you use these macros: > > > > BNXT_GET_RX_PORT_STATS64(rx_stats, rx_pause_frames) > > BNXT_GET_TX_PORT_STATS64(tx_stats, tx_pause_frames) > > > > the results will be the same with native CPU u64 types. > > Thanks! My build bot just poked me about this as well. > > I don't see any byte swaps in bnxt_get_ethtool_stats() - > are they not needed there? I'm slightly confused. No, swapping is not needed since we are referencing the sw_stats. Every counter has already been swapped when we did the copy and overflow check from the hw struct to sw_stats. sw_stats is exactly the same as the hw struct except that every counter is already swapped into native CPU u64 and properly adjusted for overflow.
On Fri, 11 Sep 2020 15:53:24 -0700 Michael Chan wrote: > > > This will work, but the types on the 2 sides don't match. On the > > > right hand side, since you are casting to the hardware struct > > > rx_port_stats and tx_port_stats, the types are __le64. > > > > > > If rx_stats and tx_stats are *u64 and you use these macros: > > > > > > BNXT_GET_RX_PORT_STATS64(rx_stats, rx_pause_frames) > > > BNXT_GET_TX_PORT_STATS64(tx_stats, tx_pause_frames) > > > > > > the results will be the same with native CPU u64 types. > > > > Thanks! My build bot just poked me about this as well. > > > > I don't see any byte swaps in bnxt_get_ethtool_stats() - > > are they not needed there? I'm slightly confused. > > No, swapping is not needed since we are referencing the sw_stats. > Every counter has already been swapped when we did the copy and > overflow check from the hw struct to sw_stats. sw_stats is exactly > the same as the hw struct except that every counter is already swapped > into native CPU u64 and properly adjusted for overflow. I see, I'll change the pointer types to u64 * as well. Thanks!
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c index d0928334bdc8..b5de242766e3 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c @@ -1778,6 +1778,24 @@ static void bnxt_get_pauseparam(struct net_device *dev, epause->tx_pause = !!(link_info->req_flow_ctrl & BNXT_LINK_PAUSE_TX); } +static void bnxt_get_pause_stats(struct net_device *dev, + struct ethtool_pause_stats *epstat) +{ + struct bnxt *bp = netdev_priv(dev); + struct rx_port_stats *rx_stats; + struct tx_port_stats *tx_stats; + + if (BNXT_VF(bp) || !(bp->flags & BNXT_FLAG_PORT_STATS)) + return; + + rx_stats = (void *)bp->port_stats.sw_stats; + tx_stats = (void *)((unsigned long)bp->port_stats.sw_stats + + BNXT_TX_PORT_STATS_BYTE_OFFSET); + + epstat->rx_pause_frames = rx_stats->rx_pause_frames; + epstat->tx_pause_frames = tx_stats->tx_pause_frames; +} + static int bnxt_set_pauseparam(struct net_device *dev, struct ethtool_pauseparam *epause) { @@ -3645,6 +3663,7 @@ const struct ethtool_ops bnxt_ethtool_ops = { ETHTOOL_COALESCE_USE_ADAPTIVE_RX, .get_link_ksettings = bnxt_get_link_ksettings, .set_link_ksettings = bnxt_set_link_ksettings, + .get_pause_stats = bnxt_get_pause_stats, .get_pauseparam = bnxt_get_pauseparam, .set_pauseparam = bnxt_set_pauseparam, .get_drvinfo = bnxt_get_drvinfo,
These stats are already reported in ethtool -S. Hopefully they are equivalent to standard stats? Signed-off-by: Jakub Kicinski <kuba@kernel.org> --- .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)