> -----Original Message----- > From: Ilya Maximets <[email protected]> > Sent: Wednesday, January 16, 2019 7:50 PM > 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 > > 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. >
Thanks for highlighting it. In v6 the 'attached' variable is assigned as required. > > > > I will add the mentioned changes in v6 > > > > Regards, > > Ophir > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
