[2/3] net/af_xdp: Fix mbuf alloc failed statistic
Checks
Commit Message
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(+)
Comments
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.
@@ -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;