[dpdk-dev] [PATCH v3 1/1] net/pcap: imissed stats support
Ferruh Yigit
ferruh.yigit at intel.com
Thu Feb 4 11:27:22 CET 2021
On 2/4/2021 10:02 AM, Ido Goshen wrote:
>
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit at intel.com>
>> Sent: Thursday, 4 February 2021 11:26
>> To: Ido Goshen <Ido at cgstowernetworks.com>
>> Cc: dev at 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 at intel.com>
>>>> Sent: Thursday, 4 February 2021 2:13
>>>> To: Ido Goshen <Ido at cgstowernetworks.com>
>>>> Cc: dev at 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 at 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; }
>>>>> +
>>>>
>>>> <...>
>
More information about the dev
mailing list