[2/3] net/af_xdp: Fix mbuf alloc failed statistic

  Failures to allocate mbufs in the receive path were not being
accounted for in the ethdev statistics. Fix this.

Bugzilla ID: 1429
Fixes: f1debd77efaf ("net/af_xdp: introduce AF_XDP PMD")
cc: stable@dpdk.og

Reported-by: Stephen Hemminger <stephen@networkplumber.org>
Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>
---
 drivers/net/af_xdp/rte_eth_af_xdp.c | 5 +++++
 1 file changed, 5 insertions(+)
  

On 10/05/2024 11:03, Ciara Loftus wrote:
> Failures to allocate mbufs in the receive path were not being
> accounted for in the ethdev statistics. Fix this.
>
> Bugzilla ID: 1429
> Fixes: f1debd77efaf ("net/af_xdp: introduce AF_XDP PMD")
> cc: stable@dpdk.og
>
> Reported-by: Stephen Hemminger <stephen@networkplumber.org>
> Signed-off-by: Ciara Loftus <ciara.loftus@intel.com>


Acked-by: Maryam Tahhan <mtahhan@redhat.com>


> ---
>   drivers/net/af_xdp/rte_eth_af_xdp.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
> index fee0d5d5f3..968bbf6d45 100644
> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> @@ -124,6 +124,7 @@ struct rx_stats {
>   	uint64_t rx_pkts;
>   	uint64_t rx_bytes;
>   	uint64_t rx_dropped;
> +	uint64_t alloc_failed;
>   };
>   
>   struct pkt_rx_queue {
> @@ -339,6 +340,8 @@ af_xdp_rx_zc(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>   		 * xsk_ring_cons__peek
>   		 */
>   		rx->cached_cons -= nb_pkts;
> +		rxq->stats.alloc_failed += nb_pkts;
> +
>   		return 0;
>   	}
>   
> @@ -408,6 +411,7 @@ af_xdp_rx_cp(void *queue, struct rte_mbuf **bufs, uint16_t nb_pkts)
>   		 * xsk_ring_cons__peek
>   		 */
>   		rx->cached_cons -= nb_pkts;
> +		rxq->stats.alloc_failed += nb_pkts;
>   		return 0;
>   	}
>   
> @@ -872,6 +876,7 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>   		stats->ibytes += stats->q_ibytes[i];
>   		stats->imissed += rxq->stats.rx_dropped;
>   		stats->oerrors += txq->stats.tx_dropped;
> +		dev->data->rx_mbuf_alloc_failed += rxq->stats.alloc_failed;
>   		fd = process_private->rxq_xsk_fds[i];
>   		ret = fd >= 0 ? getsockopt(fd, SOL_XDP, XDP_STATISTICS,
>   					   &xdp_stats, &optlen) : -1;
  

On Fri, 10 May 2024 10:03:57 +0000
Ciara Loftus <ciara.loftus@intel.com> wrote:

> diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c b/drivers/net/af_xdp/rte_eth_af_xdp.c
> index fee0d5d5f3..968bbf6d45 100644
> --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> @@ -124,6 +124,7 @@ struct rx_stats {
>  	uint64_t rx_pkts;
>  	uint64_t rx_bytes;
>  	uint64_t rx_dropped;
> +	uint64_t alloc_failed;
>  };

You don't have to use local statistic for this, there already is one in the dev struct
i.e dev->data->rx_mbuf_alloc_failed. The problem is you need the DPDK port number to find
what dev is.

And the code in ethdev for stats get will put it in the right place.


PS: what is the point of rxq->stats.rx_dropped? It is never incremented.

PPS: Looks like AF_XDP considers kernel full as an error (ie tx_dropped gets counted as error).
This is not what real hardware does.
  

On 10/05/2024 16:06, Stephen Hemminger wrote:
> You don't have to use local statistic for this, there already is one in the dev struct
> i.e dev->data->rx_mbuf_alloc_failed. The problem is you need the DPDK port number to find
> what dev is.

I think the diff id that dev->data->rx_mbuf_alloc_failed would reflect 
the pure HW(NIC) failed allocations where as the local alloc_failed 
would reflect the failed allocations on the xdp side. Both should be 
accounted for.
  

On Mon, 13 May 2024 09:23:08 +0100
Maryam Tahhan <mtahhan@redhat.com> wrote:

> On 10/05/2024 16:06, Stephen Hemminger wrote:
> > You don't have to use local statistic for this, there already is one in the dev struct
> > i.e dev->data->rx_mbuf_alloc_failed. The problem is you need the DPDK port number to find
> > what dev is.  
> 
> I think the diff id that dev->data->rx_mbuf_alloc_failed would reflect 
> the pure HW(NIC) failed allocations where as the local alloc_failed 
> would reflect the failed allocations on the xdp side. Both should be 
> accounted for.


Since this is not the fast path, why not just increment the global statistic.
This is a SW driver, but it can increment statistics just like a HW NIC driver.
  

> 
> On Fri, 10 May 2024 10:03:57 +0000
> Ciara Loftus <ciara.loftus@intel.com> wrote:
> 
> > diff --git a/drivers/net/af_xdp/rte_eth_af_xdp.c
> b/drivers/net/af_xdp/rte_eth_af_xdp.c
> > index fee0d5d5f3..968bbf6d45 100644
> > --- a/drivers/net/af_xdp/rte_eth_af_xdp.c
> > +++ b/drivers/net/af_xdp/rte_eth_af_xdp.c
> > @@ -124,6 +124,7 @@ struct rx_stats {
> >  	uint64_t rx_pkts;
> >  	uint64_t rx_bytes;
> >  	uint64_t rx_dropped;
> > +	uint64_t alloc_failed;
> >  };
> 
> You don't have to use local statistic for this, there already is one in the dev
> struct
> i.e dev->data->rx_mbuf_alloc_failed. The problem is you need the DPDK port
> number to find
> what dev is.

We now have the port number from the first patch in this series so that's no longer an issue.

> 
> And the code in ethdev for stats get will put it in the right place.
> 
> 
> PS: what is the point of rxq->stats.rx_dropped? It is never incremented.

Looks pointless indeed. Will add another patch to the series and remove it.

> 
> PPS: Looks like AF_XDP considers kernel full as an error (ie tx_dropped gets
> counted as error).
> This is not what real hardware does.