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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
