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. I will add the mentioned changes in v6 Regards, Ophir _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
