Re: [dpdk-dev] [PATCH v3 1/1] net/pcap: imissed stats support
From: Ferruh Yigit <hidden>
Date: 2021-02-04 00:13:34
On 2/3/2021 11:07 PM, Ido Goshen wrote:
quoted hunk ↗ jump to hunk
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 <redacted> --- 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'?
quoted hunk ↗ jump to hunk
+ 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;
+}
+<...>