| Message ID | 20240502212628.381069-2-jdamato@fastly.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | mlx4: Add support for netdev-genl API | expand |
On Thu, May 02, 2024 at 09:26:25PM +0000, Joe Damato wrote: > mlx4_en_alloc_frags currently returns -ENOMEM when mlx4_alloc_page > fails but does not increment a stat field when this occurs. > > struct mlx4_en_rx_ring has a dropped field which is tabulated in > mlx4_en_DUMP_ETH_STATS, but never incremented by the driver. > > This change modifies mlx4_en_alloc_frags to increment mlx4_en_rx_ring's > dropped field for the -ENOMEM case. > > Signed-off-by: Joe Damato <jdamato@fastly.com> > Tested-by: Martin Karsten <mkarsten@uwaterloo.ca> > --- > drivers/net/ethernet/mellanox/mlx4/en_rx.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c > index 8328df8645d5..573ae10300c7 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c > +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c > @@ -82,8 +82,10 @@ static int mlx4_en_alloc_frags(struct mlx4_en_priv *priv, > > for (i = 0; i < priv->num_frags; i++, frags++) { > if (!frags->page) { > - if (mlx4_alloc_page(priv, frags, gfp)) > + if (mlx4_alloc_page(priv, frags, gfp)) { > + ring->dropped++; > return -ENOMEM; > + } Correct me if I'm wrong, but ring->dropped is added to rx_dropped stats in mlx4_en_DUMP_ETH_STATS(). You have already established with Jakub that allocation error does not mean dropped packet, but the counter contributes to dropped packets stats. Also, I do not think that using a `dropped` counter for something that does not neccessarily result in a dropped packet is plain confusing. > ring->rx_alloc_pages++; > } > rx_desc->data[i].addr = cpu_to_be64(frags->dma + > -- > 2.25.1 > >
On Tue, May 07, 2024 at 05:44:27PM +0200, Larysa Zaremba wrote: > On Thu, May 02, 2024 at 09:26:25PM +0000, Joe Damato wrote: > > mlx4_en_alloc_frags currently returns -ENOMEM when mlx4_alloc_page > > fails but does not increment a stat field when this occurs. > > > > struct mlx4_en_rx_ring has a dropped field which is tabulated in > > mlx4_en_DUMP_ETH_STATS, but never incremented by the driver. > > > > This change modifies mlx4_en_alloc_frags to increment mlx4_en_rx_ring's > > dropped field for the -ENOMEM case. > > > > Signed-off-by: Joe Damato <jdamato@fastly.com> > > Tested-by: Martin Karsten <mkarsten@uwaterloo.ca> > > --- > > drivers/net/ethernet/mellanox/mlx4/en_rx.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c > > index 8328df8645d5..573ae10300c7 100644 > > --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c > > +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c > > @@ -82,8 +82,10 @@ static int mlx4_en_alloc_frags(struct mlx4_en_priv *priv, > > > > for (i = 0; i < priv->num_frags; i++, frags++) { > > if (!frags->page) { > > - if (mlx4_alloc_page(priv, frags, gfp)) > > + if (mlx4_alloc_page(priv, frags, gfp)) { > > + ring->dropped++; > > return -ENOMEM; > > + } > > Correct me if I'm wrong, but ring->dropped is added to rx_dropped stats in > mlx4_en_DUMP_ETH_STATS(). You have already established with Jakub that > allocation error does not mean dropped packet, but the counter contributes to > dropped packets stats. > > Also, I do not think that using a `dropped` counter for something that does not > neccessarily result in a dropped packet is plain confusing. Fair enough; I could add a new field called "alloc_fail" to mlx4_en_rx_ring and increment that instead as it is, according to the earlier thread, an alloc_fail as far as netdev-genl is concerned, I think. FWIW: I had spoken to Mellanox about this off list many weeks ago and they had agreed at that time to this general approach. I haven't heard from them in some time, but I am open to something else if that's the blocker here.
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c index 8328df8645d5..573ae10300c7 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c @@ -82,8 +82,10 @@ static int mlx4_en_alloc_frags(struct mlx4_en_priv *priv, for (i = 0; i < priv->num_frags; i++, frags++) { if (!frags->page) { - if (mlx4_alloc_page(priv, frags, gfp)) + if (mlx4_alloc_page(priv, frags, gfp)) { + ring->dropped++; return -ENOMEM; + } ring->rx_alloc_pages++; } rx_desc->data[i].addr = cpu_to_be64(frags->dma +