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?) > 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
