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 A39D7A0524; Thu, 4 Feb 2021 11:27:29 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7FCB22406B0; Thu, 4 Feb 2021 11:27:29 +0100 (CET) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by mails.dpdk.org (Postfix) with ESMTP id 062B92406AF for ; Thu, 4 Feb 2021 11:27:27 +0100 (CET) IronPort-SDR: 0b/JpKo0CRulhx0dpBpno8T+rCnZZQewz+dVNcCkihXDcF/0XQulEBRlGHo0Uvhpbm+2wTaSSG uADUN3SmK3/A== X-IronPort-AV: E=McAfee;i="6000,8403,9884"; a="245285678" X-IronPort-AV: E=Sophos;i="5.79,400,1602572400"; d="scan'208";a="245285678" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga105.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 04 Feb 2021 02:27:26 -0800 IronPort-SDR: j+M/N9yEyO+xtPFBUi+gJsjrEBfkreD76g8HofFArqAbqgTmwYARsVWjVsJiL0OArzrQaQr0oU 06s1Q3lTFpaw== X-IronPort-AV: E=Sophos;i="5.79,400,1602572400"; d="scan'208";a="393098621" 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 02:27:26 -0800 To: Ido Goshen Cc: "dev@dpdk.org" References: <20210201083012.28544-1-ido@cgstowernetworks.com> <20210203230750.4499-1-ido@cgstowernetworks.com> <7b31af98-5214-087d-f45f-bc6df0357419@intel.com> From: Ferruh Yigit Message-ID: <122ad50a-3604-f41b-f3c6-6e84c08de570@intel.com> Date: Thu, 4 Feb 2021 10:27:22 +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 10:02 AM, Ido Goshen wrote: > > >> -----Original Message----- >> From: Ferruh Yigit >> Sent: Thursday, 4 February 2021 11:26 >> To: Ido Goshen >> Cc: dev@dpdk.org >> Subject: Re: [PATCH v3 1/1] net/pcap: imissed stats support >> >> 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. >> > > ok > >>>> >>>>> +} >>>>> + >>>>> +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? >> > > Yes it is **with** the suggested change (missed_stat->reset = missed_stat->pcap + missed_stat->mnemonic;) > Assume this sequence > port stop -> mnemonic = N, pcap = 0 > reset stats -> reset = 0 + N, mnemonic = 0 > show -> imissed = 0 + 0 - N > > > it works good w/o the suggested change (missed_stat->reset = missed_stat->pcap;) > testpmd> show port stats 0 > > ######################## NIC statistics for port 0 ######################## > RX-packets: 0 RX-missed: 3932 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: 0 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 > ############################################################################ > > I missed 'mnemonic' get zeroed out in the "clear stats", existing implementation looks good, thanks for clarification. > > >> >> >> 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; } >>>>> + >>>> >>>> <...> >