[dpdk-dev] [PATCH 1/1] net/pcap: imissed stats support

[dpdk-dev] [PATCH 1/1] net/pcap: imissed stats support

[Ido Goshen]

Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
---
 drivers/net/pcap/rte_eth_pcap.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index a32b1f3f3..83e208514 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -58,6 +58,7 @@ struct queue_stat {
 	volatile unsigned long pkts;
 	volatile unsigned long bytes;
 	volatile unsigned long err_pkts;
+	volatile unsigned long missed_reset;
 };
 
 struct pcap_rx_queue {
@@ -680,11 +681,23 @@ eth_dev_info(struct rte_eth_dev *dev,
 	return 0;
 }
 
+static unsigned long
+eth_stats_get_pcap_missed(struct rte_eth_dev *dev, unsigned int qid)
+{
+	const struct pmd_process_private *pp = dev->process_private;
+	pcap_t *pcap = pp->rx_pcap[qid];
+	struct pcap_stat stat;
+	if (pcap_stats(pcap, &stat) != 0)
+		return 0;
+	return stat.ps_drop;
+}
+
 static int
 eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
 	unsigned int i;
 	unsigned long rx_packets_total = 0, rx_bytes_total = 0;
+	unsigned long rx_missed_total = 0;
 	unsigned long tx_packets_total = 0, tx_bytes_total = 0;
 	unsigned long tx_packets_err_total = 0;
 	const struct pmd_internals *internal = dev->data->dev_private;
@@ -695,6 +708,10 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 		stats->q_ibytes[i] = internal->rx_queue[i].rx_stat.bytes;
 		rx_packets_total += stats->q_ipackets[i];
 		rx_bytes_total += stats->q_ibytes[i];
+		unsigned long rx_missed = eth_stats_get_pcap_missed(dev, i);
+		if (rx_missed)
+			rx_missed_total = rx_missed -
+				internal->rx_queue[i].rx_stat.missed_reset;
 	}
 
 	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS &&
@@ -708,6 +725,7 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 
 	stats->ipackets = rx_packets_total;
 	stats->ibytes = rx_bytes_total;
+	stats->imissed = rx_missed_total;
 	stats->opackets = tx_packets_total;
 	stats->obytes = tx_bytes_total;
 	stats->oerrors = tx_packets_err_total;
@@ -724,6 +742,8 @@ eth_stats_reset(struct rte_eth_dev *dev)
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		internal->rx_queue[i].rx_stat.pkts = 0;
 		internal->rx_queue[i].rx_stat.bytes = 0;
+		internal->rx_queue[i].rx_stat.missed_reset =
+				eth_stats_get_pcap_missed(dev, i);
 	}
 
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
-- 
2.17.1

Re: [dpdk-dev] [PATCH 1/1] net/pcap: imissed stats support

[Ferruh Yigit]

On 1/25/2021 5:58 PM, Ido Goshen wrote:
> Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
> ---
>   drivers/net/pcap/rte_eth_pcap.c | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index a32b1f3f3..83e208514 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -58,6 +58,7 @@ struct queue_stat {
>   	volatile unsigned long pkts;
>   	volatile unsigned long bytes;
>   	volatile unsigned long err_pkts;
> +	volatile unsigned long missed_reset;
>   };
>   
>   struct pcap_rx_queue {
> @@ -680,11 +681,23 @@ eth_dev_info(struct rte_eth_dev *dev,
>   	return 0;
>   }
>   
> +static unsigned long
> +eth_stats_get_pcap_missed(struct rte_eth_dev *dev, unsigned int qid)
> +{
> +	const struct pmd_process_private *pp = dev->process_private;
> +	pcap_t *pcap = pp->rx_pcap[qid];
> +	struct pcap_stat stat;
> +	if (pcap_stats(pcap, &stat) != 0)

If the stats requested after "port stop" this will crash, since "port stop" will 
close the pcap.

Although unlikely that stats will be called after "port stop", still it may be 
and better to put a protection here.

> +		return 0;
> +	return stat.ps_drop;
> +}
> +
>   static int
>   eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>   {
>   	unsigned int i;
>   	unsigned long rx_packets_total = 0, rx_bytes_total = 0;
> +	unsigned long rx_missed_total = 0;
>   	unsigned long tx_packets_total = 0, tx_bytes_total = 0;
>   	unsigned long tx_packets_err_total = 0;
>   	const struct pmd_internals *internal = dev->data->dev_private;
> @@ -695,6 +708,10 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>   		stats->q_ibytes[i] = internal->rx_queue[i].rx_stat.bytes;
>   		rx_packets_total += stats->q_ipackets[i];
>   		rx_bytes_total += stats->q_ibytes[i];
> +		unsigned long rx_missed = eth_stats_get_pcap_missed(dev, i);
> +		if (rx_missed)
> +			rx_missed_total = rx_missed -
> +				internal->rx_queue[i].rx_stat.missed_reset;

rx_missed_total +=


After a "port stop" & "port start" the 'stat.ps_drop' will be reset, if stats 
cleared before, because of stored 'missed_reset', possible to see very big (and 
wrong) stat values.
Better to clear the 'missed_reset' on port stop.

>   	}
>   
>   	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS &&
> @@ -708,6 +725,7 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>   
>   	stats->ipackets = rx_packets_total;
>   	stats->ibytes = rx_bytes_total;
> +	stats->imissed = rx_missed_total;
>   	stats->opackets = tx_packets_total;
>   	stats->obytes = tx_bytes_total;
>   	stats->oerrors = tx_packets_err_total;
> @@ -724,6 +742,8 @@ eth_stats_reset(struct rte_eth_dev *dev)
>   	for (i = 0; i < dev->data->nb_rx_queues; i++) {
>   		internal->rx_queue[i].rx_stat.pkts = 0;
>   		internal->rx_queue[i].rx_stat.bytes = 0;
> +		internal->rx_queue[i].rx_stat.missed_reset =
> +				eth_stats_get_pcap_missed(dev, i);
>   	}
>   
>   	for (i = 0; i < dev->data->nb_tx_queues; i++) {
> 

[dpdk-dev] [PATCH v2] net/pcap: imissed stats support

[Ido Goshen]

Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
---
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 | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index a32b1f3f3..18c59d61c 100644
--- a/drivers/net/pcap/rte_eth_pcap.c
+++ b/drivers/net/pcap/rte_eth_pcap.c
@@ -58,6 +58,8 @@ struct queue_stat {
 	volatile unsigned long pkts;
 	volatile unsigned long bytes;
 	volatile unsigned long err_pkts;
+	volatile unsigned long missed_reset;
+	volatile unsigned long missed_mnemonic;
 };
 
 struct pcap_rx_queue {
@@ -144,6 +146,19 @@ RTE_LOG_REGISTER(eth_pcap_logtype, pmd.net.pcap, NOTICE);
 	rte_log(RTE_LOG_ ## level, eth_pcap_logtype, \
 		"%s(): " fmt "\n", __func__, ##args)
 
+static unsigned long
+eth_pcap_stats_missed_get(struct rte_eth_dev *dev, unsigned int qid)
+{
+	const struct pmd_process_private *pp = dev->process_private;
+	pcap_t *pcap = pp->rx_pcap[qid];
+	if (!pcap)
+		return 0;
+	struct pcap_stat stat;
+	if (pcap_stats(pcap, &stat) != 0)
+		return 0;
+	return stat.ps_drop;
+}
+
 static int
 eth_pcap_rx_jumbo(struct rte_mempool *mb_pool, struct rte_mbuf *mbuf,
 		const u_char *data, uint16_t data_len)
@@ -621,6 +636,8 @@ eth_dev_stop(struct rte_eth_dev *dev)
 
 	/* Special iface case. Single pcap is open and shared between tx/rx. */
 	if (internals->single_iface) {
+		internals->rx_queue[0].rx_stat.missed_mnemonic =
+				eth_pcap_stats_missed_get(dev, 0);
 		pcap_close(pp->tx_pcap[0]);
 		pp->tx_pcap[0] = NULL;
 		pp->rx_pcap[0] = NULL;
@@ -641,6 +658,8 @@ eth_dev_stop(struct rte_eth_dev *dev)
 
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		if (pp->rx_pcap[i] != NULL) {
+			internals->rx_queue[i].rx_stat.missed_mnemonic =
+					eth_pcap_stats_missed_get(dev, i);
 			pcap_close(pp->rx_pcap[i]);
 			pp->rx_pcap[i] = NULL;
 		}
@@ -685,6 +704,7 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
 	unsigned int i;
 	unsigned long rx_packets_total = 0, rx_bytes_total = 0;
+	unsigned long rx_missed_total = 0;
 	unsigned long tx_packets_total = 0, tx_bytes_total = 0;
 	unsigned long tx_packets_err_total = 0;
 	const struct pmd_internals *internal = dev->data->dev_private;
@@ -695,6 +715,10 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 		stats->q_ibytes[i] = internal->rx_queue[i].rx_stat.bytes;
 		rx_packets_total += stats->q_ipackets[i];
 		rx_bytes_total += stats->q_ibytes[i];
+		unsigned long rx_missed = eth_pcap_stats_missed_get(dev, i) +
+				internal->rx_queue[i].rx_stat.missed_mnemonic -
+				internal->rx_queue[i].rx_stat.missed_reset;
+		rx_missed_total += rx_missed;
 	}
 
 	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS &&
@@ -708,6 +732,7 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 
 	stats->ipackets = rx_packets_total;
 	stats->ibytes = rx_bytes_total;
+	stats->imissed = rx_missed_total;
 	stats->opackets = tx_packets_total;
 	stats->obytes = tx_bytes_total;
 	stats->oerrors = tx_packets_err_total;
@@ -724,6 +749,9 @@ eth_stats_reset(struct rte_eth_dev *dev)
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		internal->rx_queue[i].rx_stat.pkts = 0;
 		internal->rx_queue[i].rx_stat.bytes = 0;
+		internal->rx_queue[i].rx_stat.missed_reset =
+				eth_pcap_stats_missed_get(dev, i);
+		internal->rx_queue[i].rx_stat.missed_mnemonic = 0;
 	}
 
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
-- 
2.17.1

Re: [dpdk-dev] [PATCH v2] net/pcap: imissed stats support

[Ferruh Yigit]

On 2/1/2021 8:30 AM, Ido Goshen wrote:
> Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
> ---
> 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 | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
> index a32b1f3f3..18c59d61c 100644
> --- a/drivers/net/pcap/rte_eth_pcap.c
> +++ b/drivers/net/pcap/rte_eth_pcap.c
> @@ -58,6 +58,8 @@ struct queue_stat {
>   	volatile unsigned long pkts;
>   	volatile unsigned long bytes;
>   	volatile unsigned long err_pkts;
> +	volatile unsigned long missed_reset;
> +	volatile unsigned long missed_mnemonic;

Can you please put some comments why 'missed_mnemonic' is required?

<...>

> @@ -695,6 +715,10 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>   		stats->q_ibytes[i] = internal->rx_queue[i].rx_stat.bytes;
>   		rx_packets_total += stats->q_ipackets[i];
>   		rx_bytes_total += stats->q_ibytes[i];
> +		unsigned long rx_missed = eth_pcap_stats_missed_get(dev, i) +
> +				internal->rx_queue[i].rx_stat.missed_mnemonic -
> +				internal->rx_queue[i].rx_stat.missed_reset;


Instead of including the 'missed_mnemonic' to the regular calculation, what do 
you think to save the 'imissed' value to 'missed_mnemonic' in 'port_stop' and 
load it back in the 'eth_dev_start'?
This balanced usage can simplify the code I think.

Re: [dpdk-dev] [PATCH v2] net/pcap: imissed stats support

[Ido Goshen]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Monday, 1 February 2021 13:49
> To: Ido Goshen <Ido@cgstowernetworks.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v2] net/pcap: imissed stats support
> 
> On 2/1/2021 8:30 AM, Ido Goshen wrote:
> > Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
> > ---
> > 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 | 28
> ++++++++++++++++++++++++++++
> >   1 file changed, 28 insertions(+)
> >
> > diff --git a/drivers/net/pcap/rte_eth_pcap.c
> > b/drivers/net/pcap/rte_eth_pcap.c index a32b1f3f3..18c59d61c 100644
> > --- a/drivers/net/pcap/rte_eth_pcap.c
> > +++ b/drivers/net/pcap/rte_eth_pcap.c
> > @@ -58,6 +58,8 @@ struct queue_stat {
> >   	volatile unsigned long pkts;
> >   	volatile unsigned long bytes;
> >   	volatile unsigned long err_pkts;
> > +	volatile unsigned long missed_reset;
> > +	volatile unsigned long missed_mnemonic;
> 
> Can you please put some comments why 'missed_mnemonic' is required?
> 
ok

> <...>
> 
> > @@ -695,6 +715,10 @@ eth_stats_get(struct rte_eth_dev *dev, struct
> rte_eth_stats *stats)
> >   		stats->q_ibytes[i] = internal->rx_queue[i].rx_stat.bytes;
> >   		rx_packets_total += stats->q_ipackets[i];
> >   		rx_bytes_total += stats->q_ibytes[i];
> > +		unsigned long rx_missed = eth_pcap_stats_missed_get(dev,
> i) +
> > +				internal-
> >rx_queue[i].rx_stat.missed_mnemonic -
> > +				internal->rx_queue[i].rx_stat.missed_reset;
> 
> 
> Instead of including the 'missed_mnemonic' to the regular calculation, what
> do you think to save the 'imissed' value to 'missed_mnemonic' in 'port_stop'
> and load it back in the 'eth_dev_start'?
> This balanced usage can simplify the code I think.

Not sure I get the request, isn't it what the patch already doing?
Value is already stored in 'eth_dev_stop' and added back when needed.
What do you mean by load it back on  'eth_dev_start' - where to load it to?
Please explain further 

Re: [dpdk-dev] [PATCH v2] net/pcap: imissed stats support

[Ferruh Yigit]

On 2/1/2021 2:02 PM, Ido Goshen wrote:
> 
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Monday, 1 February 2021 13:49
>> To: Ido Goshen <Ido@cgstowernetworks.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH v2] net/pcap: imissed stats support
>>
>> On 2/1/2021 8:30 AM, Ido Goshen wrote:
>>> Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
>>> ---
>>> 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 | 28
>> ++++++++++++++++++++++++++++
>>>    1 file changed, 28 insertions(+)
>>>
>>> diff --git a/drivers/net/pcap/rte_eth_pcap.c
>>> b/drivers/net/pcap/rte_eth_pcap.c index a32b1f3f3..18c59d61c 100644
>>> --- a/drivers/net/pcap/rte_eth_pcap.c
>>> +++ b/drivers/net/pcap/rte_eth_pcap.c
>>> @@ -58,6 +58,8 @@ struct queue_stat {
>>>    	volatile unsigned long pkts;
>>>    	volatile unsigned long bytes;
>>>    	volatile unsigned long err_pkts;
>>> +	volatile unsigned long missed_reset;
>>> +	volatile unsigned long missed_mnemonic;
>>
>> Can you please put some comments why 'missed_mnemonic' is required?
>>
> ok
> 
>> <...>
>>
>>> @@ -695,6 +715,10 @@ eth_stats_get(struct rte_eth_dev *dev, struct
>> rte_eth_stats *stats)
>>>    		stats->q_ibytes[i] = internal->rx_queue[i].rx_stat.bytes;
>>>    		rx_packets_total += stats->q_ipackets[i];
>>>    		rx_bytes_total += stats->q_ibytes[i];
>>> +		unsigned long rx_missed = eth_pcap_stats_missed_get(dev,
>> i) +
>>> +				internal-
>>> rx_queue[i].rx_stat.missed_mnemonic -
>>> +				internal->rx_queue[i].rx_stat.missed_reset;
>>
>>
>> Instead of including the 'missed_mnemonic' to the regular calculation, what
>> do you think to save the 'imissed' value to 'missed_mnemonic' in 'port_stop'
>> and load it back in the 'eth_dev_start'?
>> This balanced usage can simplify the code I think.
> 
> Not sure I get the request, isn't it what the patch already doing?
> Value is already stored in 'eth_dev_stop' and added back when needed.
> What do you mean by load it back on  'eth_dev_start' - where to load it to?
> Please explain further
> 

Please ignore above comment, it was wrong, the intention was to not complicate 
the stats function with 'missed_mnemonic' & 'missed_reset' details,

for that what do you think to move the 'missed_mnemonic' & 'missed_reset' usage 
into the 'eth_pcap_stats_missed_get()'?
This also fixes imissed stats after multiple 'port_stop'.

Also a 'eth_pcap_stats_missed_reset()' can be added for same reason.

[dpdk-dev] [PATCH v3 1/1] net/pcap: imissed stats support

[Ido Goshen]

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 <ido@cgstowernetworks.com>
---
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;
+};
+
 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;
+	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;
+	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;
+}
+
+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;
+	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;
+}
+
 static int
 eth_pcap_rx_jumbo(struct rte_mempool *mb_pool, struct rte_mbuf *mbuf,
 		const u_char *data, uint16_t data_len)
@@ -621,6 +674,7 @@ eth_dev_stop(struct rte_eth_dev *dev)
 
 	/* Special iface case. Single pcap is open and shared between tx/rx. */
 	if (internals->single_iface) {
+		queue_missed_stat_on_stop_update(dev, 0);
 		pcap_close(pp->tx_pcap[0]);
 		pp->tx_pcap[0] = NULL;
 		pp->rx_pcap[0] = NULL;
@@ -641,6 +695,7 @@ eth_dev_stop(struct rte_eth_dev *dev)
 
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		if (pp->rx_pcap[i] != NULL) {
+			queue_missed_stat_on_stop_update(dev, i);
 			pcap_close(pp->rx_pcap[i]);
 			pp->rx_pcap[i] = NULL;
 		}
@@ -685,6 +740,7 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
 	unsigned int i;
 	unsigned long rx_packets_total = 0, rx_bytes_total = 0;
+	unsigned long rx_missed_total = 0;
 	unsigned long tx_packets_total = 0, tx_bytes_total = 0;
 	unsigned long tx_packets_err_total = 0;
 	const struct pmd_internals *internal = dev->data->dev_private;
@@ -695,6 +751,7 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 		stats->q_ibytes[i] = internal->rx_queue[i].rx_stat.bytes;
 		rx_packets_total += stats->q_ipackets[i];
 		rx_bytes_total += stats->q_ibytes[i];
+		rx_missed_total += queue_missed_stat_get(dev, i);
 	}
 
 	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS &&
@@ -708,6 +765,7 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 
 	stats->ipackets = rx_packets_total;
 	stats->ibytes = rx_bytes_total;
+	stats->imissed = rx_missed_total;
 	stats->opackets = tx_packets_total;
 	stats->obytes = tx_bytes_total;
 	stats->oerrors = tx_packets_err_total;
@@ -724,6 +782,7 @@ eth_stats_reset(struct rte_eth_dev *dev)
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		internal->rx_queue[i].rx_stat.pkts = 0;
 		internal->rx_queue[i].rx_stat.bytes = 0;
+		queue_missed_stat_reset(dev, i);
 	}
 
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
-- 
2.17.1

Re: [dpdk-dev] [PATCH v3 1/1] net/pcap: imissed stats support

[Ferruh Yigit]

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 <ido@cgstowernetworks.com>
> ---
> 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;
> +}
> +

<...>

Re: [dpdk-dev] [PATCH v3 1/1] net/pcap: imissed stats support

[Ido Goshen]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, 4 February 2021 2:13
> To: Ido Goshen <Ido@cgstowernetworks.com>
> 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 <ido@cgstowernetworks.com>
> > ---
> > 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?

> 
> > +}
> > +
> > +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
  ############################################################################

> > +	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; }
> > +
> 
> <...>

Re: [dpdk-dev] [PATCH v3 1/1] net/pcap: imissed stats support

[Ferruh Yigit]

On 2/4/2021 7:56 AM, Ido Goshen wrote:
> 
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Thursday, 4 February 2021 2:13
>> To: Ido Goshen <Ido@cgstowernetworks.com>
>> 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 <ido@cgstowernetworks.com>
>>> ---
>>> 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; }
>>> +
>>
>> <...>

Re: [dpdk-dev] [PATCH v3 1/1] net/pcap: imissed stats support

[Ido Goshen]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, 4 February 2021 11:26
> To: Ido Goshen <Ido@cgstowernetworks.com>
> 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 <ferruh.yigit@intel.com>
> >> Sent: Thursday, 4 February 2021 2:13
> >> To: Ido Goshen <Ido@cgstowernetworks.com>
> >> 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 <ido@cgstowernetworks.com>
> >>> ---
> >>> 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
  ############################################################################



 
> 
> 
> 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; }
> >>> +
> >>
> >> <...>

Re: [dpdk-dev] [PATCH v3 1/1] net/pcap: imissed stats support

[Ferruh Yigit]

On 2/4/2021 10:02 AM, Ido Goshen wrote:
> 
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Thursday, 4 February 2021 11:26
>> To: Ido Goshen <Ido@cgstowernetworks.com>
>> 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 <ferruh.yigit@intel.com>
>>>> Sent: Thursday, 4 February 2021 2:13
>>>> To: Ido Goshen <Ido@cgstowernetworks.com>
>>>> 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 <ido@cgstowernetworks.com>
>>>>> ---
>>>>> 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; }
>>>>> +
>>>>
>>>> <...>
> 

[dpdk-dev] [PATCH v4 1/1] net/pcap: imissed stats support

[Ido Goshen]

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 <ido@cgstowernetworks.com>
---
v4:
* remove volatile
* line spacing
* set missed_stat.pcap to 0 on stop

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 | 66 +++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)

diff --git a/drivers/net/pcap/rte_eth_pcap.c b/drivers/net/pcap/rte_eth_pcap.c
index a32b1f3f3..fe4c6cd12 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 */
+	unsigned int pcap;
+	/* stores values lost by pcap stop or rollover */
+	unsigned long mnemonic;
+	/* value on last reset */
+	unsigned long reset;
+};
+
 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,56 @@ 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;
+
+	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;
+
+	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;
+	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;
+	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;
+}
+
 static int
 eth_pcap_rx_jumbo(struct rte_mempool *mb_pool, struct rte_mbuf *mbuf,
 		const u_char *data, uint16_t data_len)
@@ -621,6 +681,7 @@ eth_dev_stop(struct rte_eth_dev *dev)
 
 	/* Special iface case. Single pcap is open and shared between tx/rx. */
 	if (internals->single_iface) {
+		queue_missed_stat_on_stop_update(dev, 0);
 		pcap_close(pp->tx_pcap[0]);
 		pp->tx_pcap[0] = NULL;
 		pp->rx_pcap[0] = NULL;
@@ -641,6 +702,7 @@ eth_dev_stop(struct rte_eth_dev *dev)
 
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		if (pp->rx_pcap[i] != NULL) {
+			queue_missed_stat_on_stop_update(dev, i);
 			pcap_close(pp->rx_pcap[i]);
 			pp->rx_pcap[i] = NULL;
 		}
@@ -685,6 +747,7 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 {
 	unsigned int i;
 	unsigned long rx_packets_total = 0, rx_bytes_total = 0;
+	unsigned long rx_missed_total = 0;
 	unsigned long tx_packets_total = 0, tx_bytes_total = 0;
 	unsigned long tx_packets_err_total = 0;
 	const struct pmd_internals *internal = dev->data->dev_private;
@@ -695,6 +758,7 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 		stats->q_ibytes[i] = internal->rx_queue[i].rx_stat.bytes;
 		rx_packets_total += stats->q_ipackets[i];
 		rx_bytes_total += stats->q_ibytes[i];
+		rx_missed_total += queue_missed_stat_get(dev, i);
 	}
 
 	for (i = 0; i < RTE_ETHDEV_QUEUE_STAT_CNTRS &&
@@ -708,6 +772,7 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
 
 	stats->ipackets = rx_packets_total;
 	stats->ibytes = rx_bytes_total;
+	stats->imissed = rx_missed_total;
 	stats->opackets = tx_packets_total;
 	stats->obytes = tx_bytes_total;
 	stats->oerrors = tx_packets_err_total;
@@ -724,6 +789,7 @@ eth_stats_reset(struct rte_eth_dev *dev)
 	for (i = 0; i < dev->data->nb_rx_queues; i++) {
 		internal->rx_queue[i].rx_stat.pkts = 0;
 		internal->rx_queue[i].rx_stat.bytes = 0;
+		queue_missed_stat_reset(dev, i);
 	}
 
 	for (i = 0; i < dev->data->nb_tx_queues; i++) {
-- 
2.17.1

Re: [dpdk-dev] [PATCH v4 1/1] net/pcap: imissed stats support

[Ferruh Yigit]

On 2/4/2021 10:33 AM, 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 <ido@cgstowernetworks.com>

Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [dpdk-dev] [PATCH v4 1/1] net/pcap: imissed stats support

[Ferruh Yigit]

On Thu, Feb 04, 2021 at 06:31:45PM +0000, Ferruh Yigit wrote:
> On 2/4/2021 10:33 AM, 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 <ido@cgstowernetworks.com>
> 
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 

Applied to dpdk-next-net/main, thanks.

Re: [dpdk-dev] [PATCH 1/1] net/pcap: imissed stats support

[Ferruh Yigit]

On 1/25/2021 5:58 PM, Ido Goshen wrote:
> Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>

<...>

> @@ -695,6 +708,10 @@ eth_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats *stats)
>   		stats->q_ibytes[i] = internal->rx_queue[i].rx_stat.bytes;
>   		rx_packets_total += stats->q_ipackets[i];
>   		rx_bytes_total += stats->q_ibytes[i];
> +		unsigned long rx_missed = eth_stats_get_pcap_missed(dev, i);
> +		if (rx_missed)
> +			rx_missed_total = rx_missed -
> +				internal->rx_queue[i].rx_stat.missed_reset;

'ps_drop' seems u_32 type, do you know how it behaves on overflow? Do you think 
do we need a check here for overflow?

Re: [dpdk-dev] [PATCH 1/1] net/pcap: imissed stats support

[Ido Goshen]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, 28 January 2021 20:21
> To: Ido Goshen <Ido@cgstowernetworks.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH 1/1] net/pcap: imissed stats support
> 
> On 1/25/2021 5:58 PM, Ido Goshen wrote:
> > Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
> 
> <...>
> 
> > @@ -695,6 +708,10 @@ eth_stats_get(struct rte_eth_dev *dev, struct
> rte_eth_stats *stats)
> >   		stats->q_ibytes[i] = internal->rx_queue[i].rx_stat.bytes;
> >   		rx_packets_total += stats->q_ipackets[i];
> >   		rx_bytes_total += stats->q_ibytes[i];
> > +		unsigned long rx_missed = eth_stats_get_pcap_missed(dev, i);
> > +		if (rx_missed)
> > +			rx_missed_total = rx_missed -
> > +				internal->rx_queue[i].rx_stat.missed_reset;
> 
> 'ps_drop' seems u_32 type, do you know how it behaves on overflow? Do you
> think do we need a check here for overflow?

Right, it may overflow after few hours. 
I don't see a way to fully solve it w/o periodic sampling which is quite an overhead 
To compensate and avoid getting weird high ("negative") values 
I can check if the last retrieved value is higher than the current, then either 
zero it (restart) which will reflect rollover, or 
add UINT_MAX hoping there was only one rollover since last sample
Please advice

Re: [dpdk-dev] [PATCH 1/1] net/pcap: imissed stats support

[Ferruh Yigit]

On 2/1/2021 8:53 AM, Ido Goshen wrote:
> 
> 
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Thursday, 28 January 2021 20:21
>> To: Ido Goshen <Ido@cgstowernetworks.com>
>> Cc: dev@dpdk.org
>> Subject: Re: [PATCH 1/1] net/pcap: imissed stats support
>>
>> On 1/25/2021 5:58 PM, Ido Goshen wrote:
>>> Signed-off-by: Ido Goshen <ido@cgstowernetworks.com>
>>
>> <...>
>>
>>> @@ -695,6 +708,10 @@ eth_stats_get(struct rte_eth_dev *dev, struct
>> rte_eth_stats *stats)
>>>    		stats->q_ibytes[i] = internal->rx_queue[i].rx_stat.bytes;
>>>    		rx_packets_total += stats->q_ipackets[i];
>>>    		rx_bytes_total += stats->q_ibytes[i];
>>> +		unsigned long rx_missed = eth_stats_get_pcap_missed(dev, i);
>>> +		if (rx_missed)
>>> +			rx_missed_total = rx_missed -
>>> +				internal->rx_queue[i].rx_stat.missed_reset;
>>
>> 'ps_drop' seems u_32 type, do you know how it behaves on overflow? Do you
>> think do we need a check here for overflow?
> 
> Right, it may overflow after few hours.
> I don't see a way to fully solve it w/o periodic sampling which is quite an overhead

Agree

> To compensate and avoid getting weird high ("negative") values
> I can check if the last retrieved value is higher than the current, then either
> zero it (restart) which will reflect rollover, or
> add UINT_MAX hoping there was only one rollover since last sample
> Please advice
> 

I would go with single rollover assumption, but comment this in the code.