Hi Thomas and Ilya. Thank you for your reviews. Please find comments inline.
Regards, Ophir > -----Original Message----- > From: Thomas Monjalon <[email protected]> > Sent: Thursday, January 10, 2019 4:50 PM > To: Ilya Maximets <[email protected]> > Cc: Ophir Munk <[email protected]>; [email protected]; Asaf > Penso <[email protected]>; Ian Stokes <[email protected]>; Shahaf > Shuler <[email protected]>; Olga Shern <[email protected]>; Kevin > Traynor <[email protected]> > Subject: Re: [PATCH v4] netdev-dpdk: support port representors > > 10/01/2019 15:23, Ilya Maximets: > > On 10.01.2019 16:42, Thomas Monjalon wrote: > > > Hi, > > > > > > 10/01/2019 12:32, Ilya Maximets: > > >> On 08.01.2019 12:31, Ophir Munk wrote: > > >>> if (dev->attached) { > > >>> + /* Remove the port eth device. */ > > >>> rte_eth_dev_close(dev->port_id); > > >>> - rte_eth_dev_info_get(dev->port_id, &dev_info); > > >>> - if (dev_info.device && !rte_dev_remove(dev_info.device)) { > > >>> - VLOG_INFO("Device '%s' has been detached", dev->devargs); > > >> > > >> Please keep this log message for the successful case. > > >> > > >>> - } else { > > >>> - VLOG_ERR("Device '%s' can not be detached", dev->devargs); > > >>> + VLOG_INFO("Device '%s' has been removed", dev->devargs); > > >>> + /* If this is the last port_id using this rte device > > >>> + * remove this rte device and all its eth devices. > > >>> + * FIXME: avoid direct access to DPDK internal array > rte_eth_devices. > > >>> + */ > > >>> + rte_dev = rte_eth_devices[dev->port_id].device; > > >>> + if (netdev_dpdk_get_num_ports(rte_dev) == 1) { > > >> > > >> rte_eth_dev_close() sets the RTE_ETH_DEV_UNUSED state for PMDs > with > > >> RTE_ETH_DEV_CLOSE_REMOVE flag. > > >> The function netdev_dpdk_get_num_ports() skips unused ports. > > >> The result should not be equal to 1. We need the other way to > > >> detect the siblings or close the port after calculating them. > > >> In this case we'll probably do not need to have check for UNUSED > > >> state inside the _get_num_ports(). > > > > > > Maybe I don't understand correctly. > > > Why do you want to count closed sibling ports? > > > > Proposed version of 'netdev_dpdk_get_num_ports' iterates over > > 'dpdk_list' which contains only ports that currently opened by OVS. > > So, the only port that could have UNUSED state in that list is the > > port that we're just closed here. If we'll count number of siblings > > before closing the port where should be no UNUSED ports in that list. > > > > The issue I tried to raise here is that rte_eth_dev_close() changed > > the state only for PMDs with RTE_ETH_DEV_CLOSE_REMOVE. So, the result > > of 'netdev_dpdk_get_num_ports' will be different for different PMDs. > > Yes, that's true. > You can manage it two ways: > - you rely on OVS list of open ports > - you remove the device completely (as before) > for PMDs not supporting RTE_ETH_DEV_CLOSE_REMOVE > In v5 I will implemented this last option: when RTE_ETH_DEV_CLOSE_REMOVE is not supported the code will be as before representors. > > >>> + if (rte_dev_remove(rte_dev) < 0) { > > >>> + VLOG_ERR("Device '%s' can not be detached", dev- > >devargs); > > >>> + } > > >>> } > > >>> } > > > > > > [...] > > >> Also, do we still need netdev_dpdk_get_port_by_mac() if we're using > > >> devargs iterator ? > > > > > > The devargs iterator manages "mac=" property in DPDK 18.11. > > > > Good. > > In PATCH v1 I have already tried to remove the get_port_by_mac() call, but the devargs iterator failed to correctly parse the mac address. Re-testing it now I get the same failure. I will remove the get_port_by_mac() call as soon as there is a fix in devargs iterator. > > > > > > [...] > > >>> - rte_eth_dev_close(port_id); > > >>> + /* FIXME: avoid direct access to DPDK internal array > rte_eth_devices. */ > > >>> + rte_dev = rte_eth_devices[port_id].device; > > >>> + if (netdev_dpdk_get_num_ports(rte_dev)) { > > >> > > >> Probably same here. > > >> DPDK does not set RTE_ETH_DEV_UNUSED state for PMDs without > RTE_ETH_DEV_CLOSE_REMOVE flag. > > > > > > If RTE_ETH_DEV_CLOSE_REMOVE is not set, you should call > > > rte_dev_remove() just after rte_eth_dev_close(). > > > > And what if we call rte_dev_remove() after rte_eth_dev_close() for PMD > > with RTE_ETH_DEV_CLOSE_REMOVE ? > > Then you release the full device, as before with the old "detach" API. > It makes no difference for single-port device. > But it is bad for multi-port devices (which should already implement > RTE_ETH_DEV_CLOSE_REMOVE). In v5 I will implemented this last option: when RTE_ETH_DEV_CLOSE_REMOVE is not supported the code will be as before representors. > > > >>> + response = xasprintf("Device '%s' is being shared with other " > > >>> + "interfaces. Remove them before > > >>> detaching.", > > >>> + argv[1]); > > >>> + goto error; > > >>> + } > > >>> > > >>> - rte_eth_dev_info_get(port_id, &dev_info); > > >>> - if (!dev_info.device || rte_dev_remove(dev_info.device)) { > > >>> - response = xasprintf("Device '%s' can not be detached", > > >>> argv[1]); > > >>> + rte_eth_dev_close(port_id); > > >>> + if (rte_dev_remove(rte_dev) < 0) { > > >>> + response = xasprintf("Device '%s' can not be removed", > > >>> + argv[1]); > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
