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

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

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, January 21, 2025 4:48 PM
> To: Stefan Lässer <stefan.laesser@omicronenergy.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v2] net/af_packet: provide packet drop stats
> 
> On Tue, 21 Jan 2025 13:26:16 +0100
> Stefan Laesser <stefan.laesser@omicronenergy.com> wrote:
> 
> > +		/* 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;
> > +		}
> 
> Since reading the value clears it, maybe stats_reset should call as well?
> 
> Also, queues greater that the RTE_ETHDEV_STAT_CNTRS should still count
> against the total. And the two loops could be combined.
> 
> Maybe something like this?

That looks very reasonable - I will update my patch. 

> 
> diff --git a/drivers/net/af_packet/rte_eth_af_packet.c
> b/drivers/net/af_packet/rte_eth_af_packet.c
> index 7033910df8..af08e893f1 100644
> --- a/drivers/net/af_packet/rte_eth_af_packet.c
> +++ b/drivers/net/af_packet/rte_eth_af_packet.c
> @@ -417,50 +417,51 @@ eth_dev_info(struct rte_eth_dev *dev, struct
> rte_eth_dev_info *dev_info)
>  	return 0;
>  }
> 
> +/* query dropped packets counter from socket */ static unsigned int
> +packet_drop_count(int sockfd) {
> +	struct tpacket_stats pkt_stats;
> +	socklen_t pkt_stats_len = sizeof(struct tpacket_stats);
> +
> +	if (sockfd == -1)
> +		return 0;
> +
> +	if (getsockopt(sockfd, SOL_PACKET, PACKET_STATISTICS, &pkt_stats,
> &pkt_stats_len) < -1)
> +		return 0;
> +
> +	return pkt_stats.tp_drops;
> +}
> +
>  static int
>  eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)  {
> -	unsigned i, imax;
> +	unsigned int i;
>  	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;
> 
> -	imax = (internal->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS ?
> -	        internal->nb_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS);
> -	for (i = 0; i < imax; i++) {
> -		struct tpacket_stats iface_stats;
> -		socklen_t iface_stats_len = sizeof(struct tpacket_stats);
> -
> -		/* 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;
> -		}
> -
> -		stats->q_ipackets[i] = internal->rx_queue[i].rx_pkts;
> -		stats->q_ibytes[i] = internal->rx_queue[i].rx_bytes;
> +	for (i = 0; i < internal->nb_queues; i++) {
> +		/* reading drop count clears the value */
> +		internal->rx_queue[i].rx_dropped_pkts +=
> +			packet_drop_count(internal->rx_queue[i].sockfd);
> 
>  		rx_total += stats->q_ipackets[i];
>  		rx_bytes_total += stats->q_ibytes[i];
>  		rx_dropped_total += internal->rx_queue[i].rx_dropped_pkts;
>  		rx_nombuf_total += internal->rx_queue[i].rx_nombuf;
> -	}
> 
> -	imax = (internal->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS ?
> -	        internal->nb_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS);
> -	for (i = 0; i < imax; i++) {
> -		stats->q_opackets[i] = internal->tx_queue[i].tx_pkts;
> -		stats->q_obytes[i] = internal->tx_queue[i].tx_bytes;
>  		tx_total += stats->q_opackets[i];
>  		tx_err_total += internal->tx_queue[i].err_pkts;
>  		tx_bytes_total += stats->q_obytes[i];
> +
> +		if (i < RTE_ETHDEV_QUEUE_STAT_CNTRS) {
> +			stats->q_ipackets[i] = internal->rx_queue[i].rx_pkts;
> +			stats->q_ibytes[i] = internal->rx_queue[i].rx_bytes;
> +			stats->q_opackets[i] = internal->tx_queue[i].tx_pkts;
> +			stats->q_obytes[i] = internal->tx_queue[i].tx_bytes;
> +		}
>  	}
> 
>  	stats->ipackets = rx_total;
> @@ -480,6 +481,7 @@ eth_stats_reset(struct rte_eth_dev *dev)
>  	struct pmd_internals *internal = dev->data->dev_private;
> 
>  	for (i = 0; i < internal->nb_queues; i++) {
> +		packet_drop_count(internal->rx_queue[i].sockfd);
>  		internal->rx_queue[i].rx_pkts = 0;
>  		internal->rx_queue[i].rx_bytes = 0;
>  		internal->rx_queue[i].rx_nombuf = 0;