> -----Original Message-----
> From: Ilya Maximets <[email protected]>
> Sent: Thursday, January 17, 2019 6:19 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 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.
>
I thought sign-off must be added only by you.
I assume no need for v7 just for adding signed-off-by
> >
> >> 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
