> -----Original Message-----
> From: Stephen Hemminger <step...@networkplumber.org>
> Sent: Thursday, January 16, 2025 5:25 PM
> To: Stefan Lässer <stefan.laes...@omicronenergy.com>
> Cc: John W. Linville <linvi...@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.laes...@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.laes...@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!
