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

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 17 Jan 2025 07:20:05 +0000
Stefan Lässer <stefan.laesser@omicronenergy.com> wrote:

> > -----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.

The comment is open to interpretation.
My preference is that all drivers behave the same, and the reference is one of
the original Intel drivers (ixge, e1000, etc) since they were the first.

> 
> 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?

Yes, that would be good.

Probably worth renaming igb_stats variable since it is a left over copy/paste from somewhere.