From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 6B4DB46054; Fri, 17 Jan 2025 17:43:57 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 45CB140294; Fri, 17 Jan 2025 17:43:57 +0100 (CET) Received: from mail-pl1-f177.google.com (mail-pl1-f177.google.com [209.85.214.177]) by mails.dpdk.org (Postfix) with ESMTP id BCC614028F for ; Fri, 17 Jan 2025 17:43:55 +0100 (CET) Received: by mail-pl1-f177.google.com with SMTP id d9443c01a7336-2164b662090so45630115ad.1 for ; Fri, 17 Jan 2025 08:43:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=networkplumber-org.20230601.gappssmtp.com; s=20230601; t=1737132235; x=1737737035; darn=dpdk.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=3s8ZY6aFT94jXDx9catECKOPMsc+pMfsbYaIL4G3jCI=; b=zjf2fK66u+7J8nr65mla08ityUHmAg9HDB9QW/9ST1tMWdja1Z7VDA36eTc6vROrLQ YbnQENUNUJMtB8N/cgyP1CK+o7AqOYYSi0R3x8VpvadEyU6a0FsZg6aJYBYS5zpsf8Gf NvNw8xQwH8LGwGREyVUjAuYTZW9yBiBwr4Ypv4dWAZ2XL/a6GybfPP0SJQxPvvr8MOm4 cypKbeXy5y01yOu5c8wTmrP2Yv27YH34IkZpZKO+QZzXhKNrpqSR3YOLEDQlDj90XE4g 2tdwjmSO/i0bMDyD7COUIxlwB7zYTjof7dBZDUtc0s+VdfjrsBlhiDyETv9geYEUUDeQ xzeQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1737132235; x=1737737035; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=3s8ZY6aFT94jXDx9catECKOPMsc+pMfsbYaIL4G3jCI=; b=JYq4+eQwpK5Q9bHrv+fAAhbOqFJxuhswFJ68j9pcOu17Qqz/b+N2OXvn3OvlPh/5Ht sxCQnt9Rg+VMK6B8PcUvJkf9TLooIMwzXEffbLhWhfKlu6UpkeyU1cn5q6XZRMrP0FDr /hFRnVHcD9a7wfmXJr+pc6S7VCCmcLux2mzuozqliQUSE+gdYKOT1pVk7Hv2aK+tByeG 5tlxivJN83toNL5pv+W2KK+p9xMbEQnbwdrJwNIgUAoNZGXm/dvQrNtvX5Gyr/24inbW r/+SgD4Wol8d63sMe0QAGW6nKf98jMCvAmTTgI5mKCXwLS6ZxWFIIGMB+qaV2KRM2qHH PSMw== X-Forwarded-Encrypted: i=1; AJvYcCXVhFXRxIFN3bqHbSkMj/guwq1Cpa9nWzbjjIBUgm4TI6XFssbqE+wexd0ZPzSn0g6W+fY=@dpdk.org X-Gm-Message-State: AOJu0YwQFfTp73gffV77NLSp+hkxH19IemnZCzhWYYxxgk0t2ISVzgSC yUcedoGAHgTRO5lHMX/cF2MFs/OImp4Hb0qZ++D6qHVrJZRAYcy4M/ISbKK00rAjpdjSKg2v+bM A X-Gm-Gg: ASbGncur5qEPSrgNdt4yIgKbBSiXYYBxHKBhyMbOQhhOuE6amjhlrJp8oKmlHwM4oWW mwvunMKEI3H2nzHr5LZaNuEPE0pa5toEqZtNZmVgS60jYRn/u0VlR7kpTJiiH02SGpd0dl2mU46 2bqNcTCDFFe1dfiMz5Uekjs+D+POk2GAZW1n1Tk+VAfYmcClykB7GlG7E4ROOZK6mLnU6FtkdhW dBtkLOs6DvgHNMFpURpecs6Yg9PNL32cBoKeswizxxsWpsfZRsdIguxbtDgB+7kdV9g9FIfKQvb PhYJJzygElzR8P7LhGDkMDqfOCA3GKuv/Q== X-Google-Smtp-Source: AGHT+IG2PBgOpPRvNKU7WRHaFC+CgL9gfRzEShwdItGpywFkkxI6W33F3DJphjj7FWlTDGNh0wZqvg== X-Received: by 2002:a05:6a20:a10c:b0:1e1:94ee:c37a with SMTP id adf61e73a8af0-1eb2147ea81mr5652471637.15.1737132234699; Fri, 17 Jan 2025 08:43:54 -0800 (PST) Received: from hermes.local (204-195-96-226.wavecable.com. [204.195.96.226]) by smtp.gmail.com with ESMTPSA id d2e1a72fcca58-72daba44370sm2110338b3a.122.2025.01.17.08.43.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 17 Jan 2025 08:43:54 -0800 (PST) Date: Fri, 17 Jan 2025 08:43:52 -0800 From: Stephen Hemminger To: Stefan =?UTF-8?B?TMOkc3Nlcg==?= Cc: "John W. Linville" , "dev@dpdk.org" Subject: Re: [PATCH] net/af_packet: provide packet drop stats Message-ID: <20250117084352.43c1b00a@hermes.local> In-Reply-To: References: <20250116161703.917279-1-stefan.laesser@omicronenergy.com> <20250116082443.341e5d89@hermes.local> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Fri, 17 Jan 2025 07:20:05 +0000 Stefan L=C3=A4sser wrote: > > -----Original Message----- > > From: Stephen Hemminger > > Sent: Thursday, January 16, 2025 5:25 PM > > To: Stefan L=C3=A4sser > > Cc: John W. Linville ; dev@dpdk.org > > Subject: Re: [PATCH] net/af_packet: provide packet drop stats > >=20 > > On Thu, 16 Jan 2025 17:17:03 +0100 > > Stefan Laesser wrote: > > =20 > > > 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 > > > --- > > > 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 =3D rte_pktmbuf_alloc(pkt_q->mb_pool); > > > - if (unlikely(mbuf =3D=3D NULL)) > > > + if (unlikely(mbuf =3D=3D NULL)) { > > > + pkt_q->rx_nombuf++; > > > break; > > > + } > > > > > > /* packet will fit in the mbuf, go ahead and receive it */ > > > rte_pktmbuf_pkt_len(mbuf) =3D rte_pktmbuf_data_len(mbuf) =20 > > =3D =20 > > > 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 =3D 0, tx_total =3D 0, tx_err_total =3D 0; > > > + unsigned long rx_total =3D 0, rx_dropped_total =3D 0, rx_nombuf_tot= al =3D =20 > > 0; =20 > > > + unsigned long tx_total =3D 0, tx_err_total =3D 0; > > > unsigned long rx_bytes_total =3D 0, tx_bytes_total =3D 0; > > > const struct pmd_internals *internal =3D dev->data->dev_private; > > > > > > + struct tpacket_stats iface_stats; > > > + socklen_t iface_stats_len =3D sizeof(struct tpacket_stats); =20 > >=20 > > This declaration could be moved inside the loop. =20 >=20 > Yes, you are right - I will move it inside the loop. >=20 > > > + > > > imax =3D (internal->nb_queues < RTE_ETHDEV_QUEUE_STAT_CNTRS ? > > > internal->nb_queues : RTE_ETHDEV_QUEUE_STAT_CNTRS); > > > for (i =3D 0; i < imax; i++) { > > > + /* query dropped packets counter from socket */ > > > + if (internal->rx_queue[i].sockfd !=3D -1 && > > > + getsockopt(internal->rx_queue[i].sockfd, =20 > > SOL_PACKET, =20 > > > + PACKET_STATISTICS, =20 > > &iface_stats, =20 > > > + &iface_stats_len) > -1) { > > > + /* > > > + * keep total because each call to getsocketopt with =20 > > PACKET_STATISTICS =20 > > > + * reset the counter of the socket > > > + */ > > > + internal->rx_queue[i].rx_dropped_pkts +=3D =20 > > iface_stats.tp_drops; =20 > > > + } > > > + > > > igb_stats->q_ipackets[i] =3D internal->rx_queue[i].rx_pkts; > > > igb_stats->q_ibytes[i] =3D internal->rx_queue[i].rx_bytes; > > > + igb_stats->q_errors[i] =3D internal- > > >rx_queue[i].rx_dropped_pkts; =20 > >=20 > > Dropped packets are not errors; at least most other drivers do not repo= rt > > missed packets as errors. Should be imissed statistic. =20 >=20 > 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 de= scribed 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. >=20 > As there is no q_imissed, I suggest removing this line from my patch and = just adding the imissed total counter: >=20 > igb_stats->q_ipackets[i] =3D internal->rx_queue[i].rx_pkts; > igb_stats->q_ibytes[i] =3D internal->rx_queue[i].rx_bytes; > - igb_stats->q_errors[i] =3D internal->rx_queue[i].rx_dropped_pkts; > =20 > rx_total +=3D igb_stats->q_ipackets[i]; > rx_bytes_total +=3D igb_stats->q_ibytes[i]; > - rx_dropped_total +=3D igb_stats->q_errors[i]; > + rx_dropped_total +=3D internal->rx_queue[i].rx_dropped_pkts; > rx_nombuf_total +=3D internal->rx_queue[i].rx_nombuf; >=20 > Do you agree with that? Yes, that would be good. Probably worth renaming igb_stats variable since it is a left over copy/pas= te from somewhere.