On 17.01.2019 18:12, Ophir Munk wrote: > Hi Ilay, > Please see inline > >> -----Original Message----- >> From: Ilya Maximets <[email protected]> >> Sent: Thursday, January 17, 2019 12:50 PM >> To: Ophir Munk <[email protected]>; [email protected]; Ian >> Stokes <[email protected]> >> Cc: Asaf Penso <[email protected]>; Shahaf Shuler >> <[email protected]>; Thomas Monjalon <[email protected]>; Olga >> Shern <[email protected]>; Kevin Traynor <[email protected]>; Aaron >> Conole <[email protected]> >> Subject: Re: [PATCH v5] netdev-dpdk: support port representors >> >> On 17.01.2019 3:25, Ophir Munk wrote: >>>>> + if ((eth_dev->data->dev_flags & RTE_ETH_DEV_CLOSE_REMOVE) && >>>>> + (netdev_dpdk_get_num_ports(rte_dev) > 1)) { >>> >>> This line requires a fix. It should be ">=1" instead of ">1". >> >> But current device is not closed yet. >> It'll be counted by netdev_dpdk_get_num_ports(). >> > > This line will be eventually deleted in v6 based on your code suggestion below > >>> The two lines above are rewritten in v6. >>> >>>> >>>> We still need to close the port here because if you'll have 2 sibling >>>> ports you'll not be able do detach any of them because another one still >> open. >>>> >>>> Anyway, Logic seems wrong here. >>>> >>> >>> I will try to explain the logic >>> >>>> The function intended to detach pci devices. And the usecase is >>>> detaching devices that was added by the whitelist and not attached in >>>> 'netdev_dpdk_process_devargs', i.e. has 'dev->attached = false'. >>>> >>> >>> Correct >>> >>>> The workflow should be: >>>> >>>> * Iterate over all the siblings of the desired device. >>>> * if port_id still used by OVS >>>> (netdev_dpdk_lookup_by_port_id(port_id) != >>>> NULL) >>>> print "Device '%s' is being used by interface '%s'. Remove >>>> it before detaching" >>>> >>> >>> For PMDs not supporting representors the code is backward compatible with >> previous versions. >>> For PMDs supporting representors and in cases we have dozens of multi-ports >> (representors/interfaces) - is it desirable to print dozens of such messages? >>> The current patch suggests one message for all cases: >>> "Device '%s' is being shared with other interfaces. Remove them before >>> detaching.", IMHO, I would leave it this way for this patch. If you still >>> think it >> is useful to have a message per interface - I suggest adding it in a follow >> up >> patch. >>> What do you think? >> >> This function used only in manual appctl command. It's not an issue to have a >> big output. By the way, you may collect all the port names and print like >> this: >> >> struct netdev_dpdk *dev; >> struct ds used_interfaces = DS_EMPTY_INITIALIZER; bool used = false; >> >> ds_put_format(&used_interfaces, >> "Device '%s' is being used by following interfaces:", >> argv[1]); >> >> FOR_EACH_SIBLING (&port_id) { >> dev = netdev_dpdk_lookup_by_port_id(port_id); >> if (dev) { >> used = true; >> ds_put_format(&used_interfaces, " %s", netdev_get_name(&dev->up)); >> } >> } >> >> if (used) { >> ds_put_cstr(&used_interfaces, ". Remove them before detaching."); >> response = ds_steal_cstr(&used_interfaces); >> ds_destroy(&used_interfaces); >> goto error; >> } >> >> ds_destroy(&used_interfaces); >> > > Many thanks for the nice code suggestion!. I used most of it (however > iterating over the netdev_dpdk devices rather than over the DPDK ports). > If you agree - please become co-author of this patch. >
If you wish. Looking forward for v6 for the next round of review. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
