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 985F8A0524; Thu, 4 Feb 2021 10:26:29 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 75F5A240618; Thu, 4 Feb 2021 10:26:29 +0100 (CET) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by mails.dpdk.org (Postfix) with ESMTP id 35CBB240611 for ; Thu, 4 Feb 2021 10:26:27 +0100 (CET) IronPort-SDR: Ih7qosVShfnua93545a8Uxv5CIneNk1bfxc/lJuohu2FyEpilIz/LjVcFDBIR0eTPhCe4UOOB8 1UM3QLYDNUiQ== X-IronPort-AV: E=McAfee;i="6000,8403,9884"; a="181276283" X-IronPort-AV: E=Sophos;i="5.79,400,1602572400"; d="scan'208";a="181276283" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Feb 2021 01:26:25 -0800 IronPort-SDR: Fliqh6Z0+BrTFqfxivEA2MSgvLCWtOcpyTkZMeFXxtODtY3Bolwd4mQYMUi+mOeEGuQ18vJuJe 8deAkVNPQFxQ== X-IronPort-AV: E=Sophos;i="5.79,400,1602572400"; d="scan'208";a="393076594" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.211.210]) ([10.213.211.210]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Feb 2021 01:26:25 -0800 To: Ido Goshen Cc: "dev@dpdk.org" References: <20210201083012.28544-1-ido@cgstowernetworks.com> <20210203230750.4499-1-ido@cgstowernetworks.com> From: Ferruh Yigit Message-ID: <7b31af98-5214-087d-f45f-bc6df0357419@intel.com> Date: Thu, 4 Feb 2021 09:26:21 +0000 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v3 1/1] net/pcap: imissed stats support 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 Sender: "dev" On 2/4/2021 7:56 AM, Ido Goshen wrote: > > >> -----Original Message----- >> From: Ferruh Yigit >> Sent: Thursday, 4 February 2021 2:13 >> To: Ido Goshen >> Cc: dev@dpdk.org >> Subject: Re: [PATCH v3 1/1] net/pcap: imissed stats support >> >> On 2/3/2021 11:07 PM, Ido Goshen wrote: >>> get value from pcap_stats.ps_drop (see man pcap_stats) the value is >>> adjusted in this cases: >>> - port stop - pcap is closed and will lose count >>> - stats reset - pcap doesn't provide reset api >>> - rollover - pcap counter size is u_32 only >>> >>> Signed-off-by: Ido Goshen >>> --- >>> v3: >>> * code cleanup by dedicated struct and functions extraction >>> * multi stop support by menmonic+= accumulation >>> * rollover fixup >>> >>> v2: >>> * sum all queues (rx_missed_total += fix) >>> * null pcap protection >>> * inter stop/start persistancy (counter won't reset on stop) >>> >>> drivers/net/pcap/rte_eth_pcap.c | 59 >> +++++++++++++++++++++++++++++++++ >>> 1 file changed, 59 insertions(+) >>> >>> diff --git a/drivers/net/pcap/rte_eth_pcap.c >>> b/drivers/net/pcap/rte_eth_pcap.c index a32b1f3f3..16e8752f3 100644 >>> --- a/drivers/net/pcap/rte_eth_pcap.c >>> +++ b/drivers/net/pcap/rte_eth_pcap.c >>> @@ -60,11 +60,21 @@ struct queue_stat { >>> volatile unsigned long err_pkts; >>> }; >>> >>> +struct queue_missed_stat { >>> + /* last value retrieved from pcap */ >>> + volatile unsigned int pcap; >>> + /* stores values lost by pcap stop or rollover */ >>> + volatile unsigned long mnemonic; >>> + /* value on last reset */ >>> + volatile unsigned long reset; >>> +}; >> >> I am aware other stats has 'volatile' keyword, but as far as I can see it is not >> needed, since these are new ones can you please drop the 'volatile'? > > ok > >> >>> + >>> struct pcap_rx_queue { >>> uint16_t port_id; >>> uint16_t queue_id; >>> struct rte_mempool *mb_pool; >>> struct queue_stat rx_stat; >>> + struct queue_missed_stat missed_stat; >>> char name[PATH_MAX]; >>> char type[ETH_PCAP_ARG_MAXLEN]; >>> >>> @@ -144,6 +154,49 @@ RTE_LOG_REGISTER(eth_pcap_logtype, >> pmd.net.pcap, NOTICE); >>> rte_log(RTE_LOG_ ## level, eth_pcap_logtype, \ >>> "%s(): " fmt "\n", __func__, ##args) >>> >>> +static struct queue_missed_stat* >>> +queue_missed_stat_update(struct rte_eth_dev *dev, unsigned int qid) { >>> + struct pmd_internals *internals = dev->data->dev_private; >>> + struct queue_missed_stat *missed_stat = >>> + &internals->rx_queue[qid].missed_stat; >>> + const struct pmd_process_private *pp = dev->process_private; >>> + pcap_t *pcap = pp->rx_pcap[qid]; >>> + struct pcap_stat stat; >> >> Can you please put an empty line after variable declarations, and before return. > > ok > >> >>> + if (!pcap || (pcap_stats(pcap, &stat) != 0)) >>> + return missed_stat; >>> + /* rollover check - best effort fixup assuming single rollover */ >>> + if (stat.ps_drop < missed_stat->pcap) >>> + missed_stat->mnemonic += UINT_MAX; >>> + missed_stat->pcap = stat.ps_drop; >> >> here. >> >>> + return missed_stat; >>> +} >>> + >>> +static void >>> +queue_missed_stat_on_stop_update(struct rte_eth_dev *dev, unsigned >>> +int qid) { >>> + struct queue_missed_stat *missed_stat = >>> + queue_missed_stat_update(dev, qid); >>> + missed_stat->mnemonic += missed_stat->pcap; >> >> Better to reset 'missed_stat->pcap' afterwards, in case stats requested before >> port started again: >> missed_stat->pcap = 0; > > right, should be careful not to double count it > but maybe better to set it to 0 in queue_missed_stat_update in the stop return case > if (!pcap || (pcap_stats(pcap, &stat) != 0)) > { > missed_stat->pcap = 0; > return missed_stat; > } > this way the missed_stat->pcap will always represent the current value from pcap and not hold old value > specifically in case port is stopped it will be 0 and not re-added > agree? > This also works, but there is other condition in that if block, I don't know when 'pcap_stats()' can fail, it has a risk of unexpected side affect to clear the stats randomly, I think safer to do it in 'queue_missed_stat_on_stop_update()' and I believe logic is more clear that way. >> >>> +} >>> + >>> +static void >>> +queue_missed_stat_reset(struct rte_eth_dev *dev, unsigned int qid) { >>> + struct queue_missed_stat *missed_stat = >>> + queue_missed_stat_update(dev, qid); >>> + missed_stat->reset = missed_stat->pcap; >> >> I guess this should be: >> "missed_stat->reset = missed_stat->pcap + missed_stat->mnemonic;" > > I don't think so > reset should only remember where pcap was at the reset point and not store old values > trying it immediately results in > > testpmd> show port stats 0 > > ######################## NIC statistics for port 0 ######################## > RX-packets: 0 RX-missed: 1940 RX-bytes: 0 > RX-errors: 0 > RX-nombuf: 0 > TX-packets: 0 TX-errors: 0 TX-bytes: 0 > > Throughput (since last show) > Rx-pps: 0 Rx-bps: 0 > Tx-pps: 0 Tx-bps: 0 > ############################################################################ > testpmd> clear port stats 0 > > NIC statistics for port 0 cleared > testpmd> show port stats 0 > > ######################## NIC statistics for port 0 ######################## > RX-packets: 0 RX-missed: 18446744073709550646 RX-bytes: 0 > RX-errors: 0 > RX-nombuf: 0 > TX-packets: 0 TX-errors: 0 TX-bytes: 0 > > Throughput (since last show) > Rx-pps: 0 Rx-bps: 0 > Tx-pps: 0 Tx-bps: 0 > ############################################################################ > What above shows, is it output after suggested change? missed = "(pcap + mnemonic) - reset" Lets assume overflow limit is 16, with original code: reset: 0, pcap: 2, mnemonic: 16, missed: 18 clear stats: reset: 2, pcap: 2, mnemonic: 16, missed: 16 (wrong, it should be 0) OR reset: 0, pcap: 2, mnemonic: 0, missed: 2 port stop: reset: 0, pcap: 0, mnemonic: 2, missed: 2 port start, some traffic: reset: 0, pcap: 3, mnemonic: 2, missed: 5 clear stats: reset: 3, pcap: 3, mnemonic: 2, missed: 2 (wrong, it should be 0) 'mnemonic' becomes part of the stats after overflow or "port stop", so I think it should be stored in the reset. As far as I can see suggested code change is required if I am not missing anything. >>> + missed_stat->mnemonic = 0; >>> +} >>> + >>> +static unsigned long >>> +queue_missed_stat_get(struct rte_eth_dev *dev, unsigned int qid) { >>> + const struct queue_missed_stat *missed_stat = >>> + queue_missed_stat_update(dev, qid); >>> + return missed_stat->pcap + missed_stat->mnemonic - >>> +missed_stat->reset; } >>> + >> >> <...>