Re: [dpdk-dev] [PATCH 1/3] examples/flow_classify: fix check of port and core

["Iremonger, Bernard" <bernard.iremonger@intel.com>]

Hi Min,

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, April 20, 2021 10:41 AM
> To: Min Hu (Connor) <humin29@huawei.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Iremonger,
> Bernard <bernard.iremonger@intel.com>; Kantecki, Tomasz
> <tomasz.kantecki@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Burakov, Anatoly
> <anatoly.burakov@intel.com>; david.marchand@redhat.com
> Subject: Re: [dpdk-dev] [PATCH 1/3] examples/flow_classify: fix check of
> port and core
> 
> 20/04/2021 04:26, Min Hu (Connor):
> > 2021/4/20 9:08, Thomas Monjalon:
> > > 27/03/2021 08:40, Min Hu (Connor):
> > >> fix check of port and core in flow_classify example.
> > >>
> > >> Fixes: bab16ddaf2c1 ("examples/flow_classify: add sample
> > >> application")
> > >> Cc: stable@dpdk.org
> > >>
> > >> Signed-off-by: Min Hu (Connor) <humin29@huawei.com>
> > >> ---
> > >>   	RTE_ETH_FOREACH_DEV(port)
> > >> -		if (rte_eth_dev_socket_id(port) > 0 &&
> > >> +		if (rte_eth_dev_socket_id(port) >= 0 &&

This fix works (I have tested it on my system)
However a cleaner fix would be to drop the above line and add the if on the next line instead (also tested on my system).

If (rte_eth_dev_socket_id(port) != (int)rte_socket_id())

> > >>   			rte_eth_dev_socket_id(port) != (int)rte_socket_id())
> {
> > >>   			printf("\n\n");
> > >>   			printf("WARNING: port %u is on remote NUMA
> node\n",
> > >
> > > Please explain which case is broken and why.
> > > If I understand well, we don't detect remote NUMA if not running on first
> socket.
> > >
> > Hi, the code is this:
> >
> **********************************************************
> ***************
> > 	/*
> > 	 * Check that the port is on the same NUMA node as the polling
> thread
> > 	 * for best performance.
> > 	 */
> > 	RTE_ETH_FOREACH_DEV(port)
> > 		if (rte_eth_dev_socket_id(port) > 0 &&
> > 			rte_eth_dev_socket_id(port) != (int)rte_socket_id())
> {
> > 			printf("\n\n");
> > 			printf("WARNING: port %u is on remote NUMA
> node\n",
> > 			       port);
> > 			printf("to polling thread.\n");
> > 			printf("Performance will not be optimal.\n");
> > 		}
> > 	printf("\nCore %u forwarding packets. ", rte_lcore_id());
> > 	printf("[Ctrl+C to quit]\n");
> >
> **********************************************************
> ************
> > ***
> >
> > According to the comments and logging, the author just hope user to
> > use the core and device which are in the same numa node for optimal
> > performance. If not, A warning gives out.
> >
> > For example in flow_classify:
> > ./build/flow_classify -w 0000:7d:00.1  -l 93
> > Here:
> > 0000:7d:00.1 is on numa node 0.
> > core 93  is on numa node 3.
> >
> > the two are not in same numa node, but no warning gives out in old codes.
> >
> > Well, using this patch, we can get the waring.
> 
> You need to explain which case was broken in the commit log.
> Thanks
> 
> 
Regards,

Bernard.