| Message ID | 20241216075842.2394606-3-srasheed@marvell.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fix race conditions in ndo_get_stats64 | expand |
On Sun, Dec 15, 2024 at 11:58:40PM -0800, Shinas Rasheed wrote: > The per queue stats are available already and are retrieved > from register reads during ndo_get_stats64. The firmware stats > fetch call that happens in ndo_get_stats64() is currently not > required > > Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops") > Signed-off-by: Shinas Rasheed <srasheed@marvell.com> > --- > V2: > - No changes > > V1: https://lore.kernel.org/all/20241203072130.2316913-3-srasheed@marvell.com/ > > drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 10 ---------- > 1 file changed, 10 deletions(-) > > diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c > index 941bbaaa67b5..6400d6008097 100644 > --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c > +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c > @@ -996,12 +996,6 @@ static void octep_get_stats64(struct net_device *netdev, > struct octep_device *oct = netdev_priv(netdev); > int q; > > - if (netif_running(netdev)) > - octep_ctrl_net_get_if_stats(oct, > - OCTEP_CTRL_NET_INVALID_VFID, > - &oct->iface_rx_stats, > - &oct->iface_tx_stats); > - > tx_packets = 0; > tx_bytes = 0; > rx_packets = 0; > @@ -1019,10 +1013,6 @@ static void octep_get_stats64(struct net_device *netdev, > stats->tx_bytes = tx_bytes; > stats->rx_packets = rx_packets; > stats->rx_bytes = rx_bytes; > - stats->multicast = oct->iface_rx_stats.mcast_pkts; > - stats->rx_errors = oct->iface_rx_stats.err_pkts; > - stats->collisions = oct->iface_tx_stats.xscol; > - stats->tx_fifo_errors = oct->iface_tx_stats.undflw; I do not see, how it is a fix to remove some fields from stats. If this is a cleanup, it should not go to the stable tree. > } > > /** > -- > 2.25.1 > >
Hi Larysa, > On Sun, Dec 15, 2024 at 11:58:40PM -0800, Shinas Rasheed wrote: > > The per queue stats are available already and are retrieved > > from register reads during ndo_get_stats64. The firmware stats > > fetch call that happens in ndo_get_stats64() is currently not > > required > > > > Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops") > > Signed-off-by: Shinas Rasheed <srasheed@marvell.com> > > @@ -1019,10 +1013,6 @@ static void octep_get_stats64(struct net_device > *netdev, > > stats->tx_bytes = tx_bytes; > > stats->rx_packets = rx_packets; > > stats->rx_bytes = rx_bytes; > > - stats->multicast = oct->iface_rx_stats.mcast_pkts; > > - stats->rx_errors = oct->iface_rx_stats.err_pkts; > > - stats->collisions = oct->iface_tx_stats.xscol; > > - stats->tx_fifo_errors = oct->iface_tx_stats.undflw; > > I do not see, how it is a fix to remove some fields from stats. If this is a > cleanup, it should not go to the stable tree. > > > } > > The fix part of this patch is to remove the call to firmware to retrieve stats, which could block and cause rcu read lock warnings. The fields that are retrieved by this stats call can be neglected for use cases concerning the DPU, and the necessary stats are already read from per queue hardware stats registers. Hence, why the code is removed.
On Mon, Dec 16, 2024 at 06:31:05PM +0000, Shinas Rasheed wrote: > Hi Larysa, > > > > On Sun, Dec 15, 2024 at 11:58:40PM -0800, Shinas Rasheed wrote: > > > The per queue stats are available already and are retrieved > > > from register reads during ndo_get_stats64. The firmware stats > > > fetch call that happens in ndo_get_stats64() is currently not > > > required > > > > > > Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops") > > > Signed-off-by: Shinas Rasheed <srasheed@marvell.com> > > > @@ -1019,10 +1013,6 @@ static void octep_get_stats64(struct net_device > > *netdev, > > > stats->tx_bytes = tx_bytes; > > > stats->rx_packets = rx_packets; > > > stats->rx_bytes = rx_bytes; > > > - stats->multicast = oct->iface_rx_stats.mcast_pkts; > > > - stats->rx_errors = oct->iface_rx_stats.err_pkts; > > > - stats->collisions = oct->iface_tx_stats.xscol; > > > - stats->tx_fifo_errors = oct->iface_tx_stats.undflw; > > > > I do not see, how it is a fix to remove some fields from stats. If this is a > > cleanup, it should not go to the stable tree. > > > > > } > > > > > The fix part of this patch is to remove the call to firmware to retrieve stats, which could block and cause rcu read lock warnings. > The fields that are retrieved by this stats call can be neglected for use cases concerning the DPU, and the necessary stats are already > read from per queue hardware stats registers. Hence, why the code is removed. > Please, provide the warinings in question in the commit message, this is an important context for anyone that would need to look through code history.
diff --git a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c index 941bbaaa67b5..6400d6008097 100644 --- a/drivers/net/ethernet/marvell/octeon_ep/octep_main.c +++ b/drivers/net/ethernet/marvell/octeon_ep/octep_main.c @@ -996,12 +996,6 @@ static void octep_get_stats64(struct net_device *netdev, struct octep_device *oct = netdev_priv(netdev); int q; - if (netif_running(netdev)) - octep_ctrl_net_get_if_stats(oct, - OCTEP_CTRL_NET_INVALID_VFID, - &oct->iface_rx_stats, - &oct->iface_tx_stats); - tx_packets = 0; tx_bytes = 0; rx_packets = 0; @@ -1019,10 +1013,6 @@ static void octep_get_stats64(struct net_device *netdev, stats->tx_bytes = tx_bytes; stats->rx_packets = rx_packets; stats->rx_bytes = rx_bytes; - stats->multicast = oct->iface_rx_stats.mcast_pkts; - stats->rx_errors = oct->iface_rx_stats.err_pkts; - stats->collisions = oct->iface_tx_stats.xscol; - stats->tx_fifo_errors = oct->iface_tx_stats.undflw; } /**
The per queue stats are available already and are retrieved from register reads during ndo_get_stats64. The firmware stats fetch call that happens in ndo_get_stats64() is currently not required Fixes: 6a610a46bad1 ("octeon_ep: add support for ndo ops") Signed-off-by: Shinas Rasheed <srasheed@marvell.com> --- V2: - No changes V1: https://lore.kernel.org/all/20241203072130.2316913-3-srasheed@marvell.com/ drivers/net/ethernet/marvell/octeon_ep/octep_main.c | 10 ---------- 1 file changed, 10 deletions(-)