Re: [dpdk-dev] [PATCH] app/testpmd: fix statistics in multiple process

["Min Hu (Connor)" <humin29@huawei.com>]

在 2021/10/27 0:22, Ferruh Yigit 写道:
> On 9/17/2021 4:33 AM, Min Hu (Connor) wrote:
>> Hi, Xiaoyun,
>>
>> 在 2021/9/16 13:17, Li, Xiaoyun 写道:
>>> Hi
>>>
>>>> -----Original Message-----
>>>> From: Min Hu (Connor) <humin29@huawei.com>
>>>> Sent: Tuesday, September 14, 2021 11:13
>>>> To: dev@dpdk.org
>>>> Cc: Yigit, Ferruh <ferruh.yigit@intel.com>; Li, Xiaoyun 
>>>> <xiaoyun.li@intel.com>
>>>> Subject: [PATCH] app/testpmd: fix statistics in multiple process
>>>>
>>>> Currently, 'clear port stats all' in secondary will clear stats in 
>>>> PMD, and also
>>>> update stats which APP stores in 'ports[]' array in secondary 
>>>> process, note that,
>>>> that in primary process remains unchanged. So, when 'show fwd stats 
>>>> all' in
>>>> primary process, stats in PMD may be less than stats which APP 
>>>> stores in 'ports[]'
>>>> array(the stats is different with that in secondary). So forward 
>>>> statistics(stats in
>>>> PMD minus stats which APP stores in 'ports[]' array) will be wrong. 
>>>> Like this:
>>>> ---------------------- Forward statistics for port 0  --------------
>>>> RX-packets: 18446744073703120168 RX-dropped: 18446744073704076766
>>>> RX-total: 18446744073697645318
>>>> TX-packets: 18446744073703122216 TX-dropped: 0
>>>> TX-total: 18446744073703122216
>>>> --------------------------------------------------------------------
>>>> Stats in PMD are shared between multiple processes, but stats which 
>>>> APP stores
>>>> have their own copies in multiple processes. And this will result in 
>>>> bugs.
>>>>
>>>> This patch will fix it by creating shared memory to store last stats 
>>>> for multiple
>>>> and secondary process in testpmd.
>>>
>>> Why not just limit "clear port stats " behavior to only primary process?
>>> Is there any particular reason second process has to do clear stats?
>> While, I have no idea if some particular reason exists second process 
>> has to do clear stats. I just want to implement this function.
>>
>>>
>>> Stats is quite complicate struct. I feel like it will have race 
>>> condition issue if you allow two processes to clear it.
>>
>>> Only allow getting for multiple processes makes more sense.
>> Yes, I agree with you, maybe I can add constrains in testpmd guide doc.
>> BTW, what abot any others' opinion ?
>>
> 
> I agree with Xiaoyun on possible race conditions, and to restrict the 
> secondary
> to read the stats but not clear it.
> 
Got it, this patch can be abandoned, thanks.
>>>
>>> Also your patch has issues even if this idea is accepted by others. 
>>> Please see below.
>>>
>>>>
>>>> Fixes: 184de26c78d0 ("app/testpmd: support multi-process")
>>>>
>>>> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
>>>> ---
>>>>   app/test-pmd/config.c  |  4 +--
>>>>   app/test-pmd/testpmd.c | 63 
>>>> ++++++++++++++++++++++++++++++++++++------
>>>>   app/test-pmd/testpmd.h |  2 +-
>>>>   3 files changed, 58 insertions(+), 11 deletions(-)
>>>>
>>> <snip>
>>>> @@ -3572,6 +3614,7 @@ init_port_config(void)
>>>>               port->dev_conf.intr_conf.lsc = 1;
>>>>           if (rmv_interrupt && (*port->dev_info.dev_flags &
>>>> RTE_ETH_DEV_INTR_RMV))
>>>>               port->dev_conf.intr_conf.rmv = 1;
>>>> +        port_stats_init(pid);
>>>
>>> Init_port_config is called in several places. Why init stats as 0 for 
>>> port->stats here?
>>> You did so. Then if user use cmd like "port config 0 rxq 2" which 
>>> calls init_port_config, port->stats is cleared while device stats isn't.
>>> Then the fwd stats you got will be wrong.
>>>
>>> Please read code carefully. Port->stats will be reset as device stats 
>>> when start_fwd. Not reset as 0.
>>>
>>> Port->stats should be only cleared when you do clear_nic_stats. 
>>> Otherwise, it doesn't make sense as "last port stats".
>>>
>> I know your meaning.
>> Actually, My purpose for 'port_stats_init' is only called for all port
>>   at init stage. While 'init_port config' confusd me, May I can put the
>> 'port_stats_init' in 'init_config', this may can work out.
>>
>> Thanks.
>>
>>
>>>>       }
>>>>   }
>>>>
>>>> @@ -3882,6 +3925,10 @@ main(int argc, char** argv)
>>>>           rte_exit(EXIT_FAILURE, "Cannot init EAL: %s\n",
>>>>                rte_strerror(rte_errno));
>>>>
>>>> +    ret = eth_stats_zone_reserve();
>>>> +    if (ret != 0)
>>>> +        rte_exit(EXIT_FAILURE, "Allocate memzone ethdev stats 
>>>> failed");
>>>> +
>>>>       ret = register_eth_event_callback();
>>>>       if (ret != 0)
>>>>           rte_exit(EXIT_FAILURE, "Cannot register for ethdev events");
>>>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
>>>> 5863b2f43f..a48a302343 100644
>>>> --- a/app/test-pmd/testpmd.h
>>>> +++ b/app/test-pmd/testpmd.h
>>>> @@ -202,7 +202,7 @@ struct rte_port {
>>>>       struct rte_eth_dev_info dev_info;   /**< PCI info + driver 
>>>> name */
>>>>       struct rte_eth_conf     dev_conf;   /**< Port configuration. */
>>>>       struct rte_ether_addr       eth_addr;   /**< Port ethernet 
>>>> address */
>>>> -    struct rte_eth_stats    stats;      /**< Last port statistics */
>>>> +    struct rte_eth_stats    *stats;      /**< Last port statistics */
>>>>       unsigned int            socket_id;  /**< For NUMA support */
>>>>       uint16_t        parse_tunnel:1; /**< Parse internal headers */
>>>>       uint16_t                tso_segsz;  /**< Segmentation offload 
>>>> MSS for non-
>>>> tunneled packets. */
>>>> -- 
>>>> 2.33.0
>>>
>>> .
>>>
> 
> .