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 7AEADA0A0E; Thu, 4 Feb 2021 01:13:33 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 042CF2404B3; Thu, 4 Feb 2021 01:13:33 +0100 (CET) Received: from mga18.intel.com (mga18.intel.com [134.134.136.126]) by mails.dpdk.org (Postfix) with ESMTP id 063842404AD for ; Thu, 4 Feb 2021 01:13:30 +0100 (CET) IronPort-SDR: h2xQF7kB4zaFDKaFejsHrYoIeru6e4kB2TaRAg9lA5LdZuoMPG0h3gmX/s0l++G82lBCzRSwv3 VDmtuUb0g6hA== X-IronPort-AV: E=McAfee;i="6000,8403,9884"; a="168823068" X-IronPort-AV: E=Sophos;i="5.79,399,1602572400"; d="scan'208";a="168823068" Received: from orsmga008.jf.intel.com ([10.7.209.65]) by orsmga106.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Feb 2021 16:13:26 -0800 IronPort-SDR: wamUaS6qJwcP28bB/41XWJUNob+UrVJgfyBhDGnbIvWQad425gfSJtvA5jDyyKNZOaMHtv+LtM OlhdTVEqrf+g== X-IronPort-AV: E=Sophos;i="5.79,399,1602572400"; d="scan'208";a="392791078" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.245.26]) ([10.213.245.26]) by orsmga008-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Feb 2021 16:13: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: Date: Thu, 4 Feb 2021 00:13:18 +0000 MIME-Version: 1.0 In-Reply-To: <20210203230750.4499-1-ido@cgstowernetworks.com> 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/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'? > + > 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. > + 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; > +} > + > +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;" > + 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; > +} > + <...>