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: Min Hu (Connor) <humin29@huawei.com>
> Sent: Tuesday, April 27, 2021 1:23 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@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
> 
> 
> 
> 在 2021/4/27 19:51, Iremonger, Bernard 写道:
> > 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).
> >
> Hi, 'rte_eth_dev_socket_id(port)' may return -1,  we should avoid this
> situation.

Agreed.
Original fix is good.

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

Regards,

Bernard.