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

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

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