On 17.01.2019 19:03, Ophir Munk wrote: > Hi Ilya, > Please see inline > >> -----Original Message----- >> From: Ilya Maximets <[email protected]> >> Sent: Thursday, January 17, 2019 5:34 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 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. >> > > Yes, added you as co-author (need to sign-off?)
Yes. Sign-off required. Checkpatch will complain. You could run ./utilities/checkpatch.sh <file.patch> before sending. Or even add a commit-message hook for that. > >> Looking forward for v6 for the next round of review. >> > > V6 was sent (my mail server may delay the sending for unknown reasons). > > >> Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
