RE: [PATCH] net/af_packet: provide packet drop stats

["Stefan Lässer" <stefan.laesser@omicronenergy.com>]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Thursday, January 16, 2025 5:25 PM
> To: Stefan Lässer <stefan.laesser@omicronenergy.com>
> Cc: John W. Linville <linville@tuxdriver.com>; dev@dpdk.org
> Subject: Re: [PATCH] net/af_packet: provide packet drop stats
> 
> On Thu, 16 Jan 2025 17:17:03 +0100
> Stefan Laesser <stefan.laesser@omicronenergy.com> wrote:
> 
> > The Linux kernel provides the ability to query the packet drop counter
> > of a socket. This information can be provided when the user requests
> > stats.
> >
> > It is important to note that each call to getsockopt with
> > PACKET_STATISTICS resets the internal counters. So the caller needs to
> > keep track of the total count on its own.
> >
> > Next, I have added a counter for the case when mbuf couldn't be
> > allocated.
> >
> > Signed-off-by: Stefan Laesser <stefan.laesser@omicronenergy.com>
> > ---
> >  drivers/net/af_packet/rte_eth_af_packet.c | 32
> > +++++++++++++++++++++--
> >  1 file changed, 30 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/af_packet/rte_eth_af_packet.c
> > b/drivers/net/af_packet/rte_eth_af_packet.c
> > index ceb8d9356a..a771dd854d 100644
> > --- a/drivers/net/af_packet/rte_eth_af_packet.c
> > +++ b/drivers/net/af_packet/rte_eth_af_packet.c
> > @@ -58,6 +58,8 @@ struct __rte_cache_aligned pkt_rx_queue {
> >
> >  	volatile unsigned long rx_pkts;
> >  	volatile unsigned long rx_bytes;
> > +	volatile unsigned long rx_nombuf;
> > +	volatile unsigned long rx_dropped_pkts;
> >  };
> >
> >  struct __rte_cache_aligned pkt_tx_queue { @@ -145,8 +147,10 @@
> > eth_af_packet_rx(void *queue, struct rte_mbuf **bufs, uint16_t
> > nb_pkts)
> >
> >  		/* allocate the next mbuf */
> >  		mbuf = rte_pktmbuf_alloc(pkt_q->mb_pool);
> > -		if (unlikely(mbuf == NULL))
> > +		if (unlikely(mbuf == NULL)) {
> > +			pkt_q->rx_nombuf++;
> >  			break;
> > +		}
> >
> >  		/* packet will fit in the mbuf, go ahead and receive it */
> >  		rte_pktmbuf_pkt_len(mbuf) = rte_pktmbuf_data_len(mbuf)
> =
> > ppd->tp_snaplen; @@ -417,17 +421,37 @@ static int
> > eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats
> > *igb_stats)  {
> >  	unsigned i, imax;
> > -	unsigned long rx_total = 0, tx_total = 0, tx_err_total = 0;
> > +	unsigned long rx_total = 0, rx_dropped_total = 0, rx_nombuf_total =
> 0;
> > +	unsigned long tx_total = 0, tx_err_total = 0;
> >  	unsigned long rx_bytes_total = 0, tx_bytes_total = 0;
> >  	const struct pmd_internals *internal = dev->data->dev_private;
> >
> > +	struct tpacket_stats iface_stats;
> > +	socklen_t iface_stats_len = sizeof(struct tpacket_stats);
> 
> This declaration could be moved inside the loop.

Yes, you are right - I will move it inside the loop.

> > +
> >  	imax = (internal->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS ?
> >  	        internal->nb_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS);
> >  	for (i = 0; i < imax; i++) {
> > +		/* query dropped packets counter from socket */
> > +		if (internal->rx_queue[i].sockfd != -1 &&
> > +			getsockopt(internal->rx_queue[i].sockfd,
> SOL_PACKET,
> > +						PACKET_STATISTICS,
> &iface_stats,
> > +						&iface_stats_len) > -1) {
> > +			/*
> > +			 * keep total because each call to getsocketopt with
> PACKET_STATISTICS
> > +			 * reset the counter of the socket
> > +			 */
> > +			internal->rx_queue[i].rx_dropped_pkts +=
> iface_stats.tp_drops;
> > +		}
> > +
> >  		igb_stats->q_ipackets[i] = internal->rx_queue[i].rx_pkts;
> >  		igb_stats->q_ibytes[i] = internal->rx_queue[i].rx_bytes;
> > +		igb_stats->q_errors[i] = internal-
> >rx_queue[i].rx_dropped_pkts;
> 
> Dropped packets are not errors; at least most other drivers do not report
> missed packets as errors. Should be imissed statistic.

The struct rte_eth_stats currently does not contain q_imissed. It only has
q_ipackets, q_opackets, q_ibytes, q_obytes and q_errors. The latter is described as
"Total number of queue packets received that are dropped.". This is why I have choosen
q_errors  because the field comment sounds like a good match to me.

As there is no q_imissed, I suggest removing this line from my patch and just adding the imissed total counter:

    igb_stats->q_ipackets[i] = internal->rx_queue[i].rx_pkts;
    igb_stats->q_ibytes[i] = internal->rx_queue[i].rx_bytes;
-   igb_stats->q_errors[i] = internal->rx_queue[i].rx_dropped_pkts;
 
    rx_total += igb_stats->q_ipackets[i];
    rx_bytes_total += igb_stats->q_ibytes[i];
-   rx_dropped_total += igb_stats->q_errors[i];
+  rx_dropped_total += internal->rx_queue[i].rx_dropped_pkts;
    rx_nombuf_total += internal->rx_queue[i].rx_nombuf;

Do you agree with that?

> > +
> >  		rx_total += igb_stats->q_ipackets[i];
> >  		rx_bytes_total += igb_stats->q_ibytes[i];
> > +		rx_dropped_total += igb_stats->q_errors[i];
> > +		rx_nombuf_total += internal->rx_queue[i].rx_nombuf;
> >  	}
> >
> >  	imax = (internal->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS ?
> > @@ -442,6 +466,8 @@ eth_stats_get(struct rte_eth_dev *dev, struct
> > rte_eth_stats *igb_stats)
> >
> >  	igb_stats->ipackets = rx_total;
> >  	igb_stats->ibytes = rx_bytes_total;
> > +	igb_stats->imissed = rx_dropped_total;
> > +	igb_stats->rx_nombuf = rx_nombuf_total;
> >  	igb_stats->opackets = tx_total;
> >  	igb_stats->oerrors = tx_err_total;
> >  	igb_stats->obytes = tx_bytes_total;
> > @@ -457,6 +483,8 @@ eth_stats_reset(struct rte_eth_dev *dev)
> >  	for (i = 0; i < internal->nb_queues; i++) {
> >  		internal->rx_queue[i].rx_pkts = 0;
> >  		internal->rx_queue[i].rx_bytes = 0;
> > +		internal->rx_queue[i].rx_nombuf = 0;
> > +		internal->rx_queue[i].rx_dropped_pkts = 0;
> >  	}
> >
> >  	for (i = 0; i < internal->nb_queues; i++) {
> 
> What about stats reset?

I'm sorry, I don't understand what you mean. The two newly added queue counters are reset in the eth_stats_reset() function.
Isn't that the right place to do it?

Thanks for your feedback!