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

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

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 ?

> 
> 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
> 
> .
>