On 16.01.2019 20:36, Ophir Munk wrote: > Hi Ilya and thanks for your comments. > Please see inline > >> -----Original Message----- >> From: Ilya Maximets <[email protected]> >> Sent: Tuesday, January 15, 2019 11:16 AM >> To: Ophir Munk <[email protected]>; [email protected] >> Cc: Asaf Penso <[email protected]>; Ian Stokes <[email protected]>; >> Shahaf Shuler <[email protected]>; Thomas Monjalon >> <[email protected]>; Olga Shern <[email protected]>; Kevin Traynor >> <[email protected]> >> Subject: Re: [PATCH v4] netdev-dpdk: support port representors >> >>>> >>>> But we have the same check on the upper level. If you will not check >>>> here, the new_port_id will go to netdev_dpdk_set_config(), where we >>>> have equal check for duplicated ports. Why this doesn't work for you? >>>> >>> >>> I will explain why this does not work. Please note the following code >>> snippet from >>> netdev_dpdk_set_config(): >>> >>> 1 dpdk_port_t new_port_id = netdev_dpdk_process_devargs(dev, >> new_devargs, errp) >>> 2 if (!rte_eth_dev_is_valid_port(new_port_id)) { >>> 3 err = EINVAL; >>> 4 } else if (new_port_id == dev->port_id) { >>> 5 /* Already configured, do not reconfigure again */ >>> 6 err = 0; >>> 7 } else { >>> 8 err = netdev_dpdk_check_dup_dev(new_port_id, netdev, >>> 9 new_devargs, errp); >>> 10 if (!err) { >>> 11 int sid = rte_eth_dev_socket_id(new_port_id); >>> 12 >>> 13 dev->requested_socket_id = sid < 0 ? SOCKET0 : sid; >>> 14 dev->devargs = xstrdup(new_devargs); >>> 15 dev->port_id = new_port_id; >>> 16 netdev_request_reconfigure(&dev->up); >>> 17 netdev_dpdk_clear_xstats(dev); >>> 18 } >>> 19 } >>> >>> Line 1: Before representors were introduced the check for a duplicated >> device was implicitly done inside netdev_dpdk_process_devarags which >> returned an error (reason: rte_dev_probe() was called twice on the same >> devargs and failed). >>> In such a case err is set to EINVAL in lines 2-3 and we do not reach lines >>> 4-19. >> With the introduction of representors netdev_dpdk_process_devarags() may >> return the same 'new_port_id' if called again with the same devargs, or may >> fail as before for PMDs which do not support representors. >>> In case netdev_dpdk_process_devarags() is successful for the same devargs >> we end with no errors (err = 0) in lines 4-6 .... hence we ignore the dup dev >> case which results in accepting the same device for 2 different ports .... >> which >> ends in seg fault. >> >> But 'dev->port_id' should not be initialized yet at this point. >> And the (new_port_id == dev->port_id) should not be true and we should >> execute code in 'else' branch (8-18). >> > > You are right. We can skip checking dup_dev inside > netdev_dpdk_process_devarags() and leave it only in netdev_dpdk_set_config(). > There is still a new changes needed: do not assign dev->attached=true inside > netdev_dpdk_process_devarags(). Delay this assignment only after a successful > call to check_dup_dev.
You should not move assignment to the upper layer because it should be set to 'true' only if device was probed by the 'rte_dev_probe'. In case the device was already probed before (by the whitelist) the value of 'dev->attached' must be 'false'. You need to move the probing logic out of 'netdev_dpdk_probe_port_by_devargs' instead. And make it 'netdev_dpdk_get_port_by_devargs'. It looks like the logic around 'attached' variable is messed up with this patch set. We need to recheck it carefully. > > I will add the mentioned changes in v6 > > Regards, > Ophir > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
