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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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.