[ovs-dev] [PATCH] netdev-dpdk: fix port addition for ports sharing same PCI id

[Stokes, Ian]

Hi Yuanhan,

Thanks for working on this, I've done some testing with ani40e device and a review of the current patch, please find comments below.

> On 12/20/2017 04:03 PM, Yuanhan Liu wrote:
> > Some NICs have only one PCI address associated with multiple ports.
> > This patch extends the dpdk-devargs option's format to cater for such
> devices.
> >
> > To achieve that, this patch uses a new syntax that will be adapted and
> > implemented in future DPDK release (likely, v18.05):
> >     http://dpdk.org/ml/archives/dev/2017-December/084234.html
> >
> > And since it's the DPDK duty to parse the (complete and full) syntax
> > and this patch is more likely to serve as an intermediate workaround,
> > here I take a simpler and shorter syntax from it (note it's allowed to
> > have only one category being provided):
> >     class=eth,mac=00:11:22:33:44:55:66
> >
> > Also, old compatibility is kept. Users can still go on with using the
> > PCI id to add a port (if that's enough for them). Meaning, this patch
> > will not break anything.

I've validated that both the old method/new method both work with i40e devices as expected.

> >
> 
> Hi Yuanhan, I think there would need to be some doc updates also for a new
> syntax.

Fully agree, would need updates to the relevant OVS DPDK related docs as well as an explanation regarding why there are 2 approaches to adding ports.

> 
> How settled/agreed is the syntax in DPDK now? Ideally it is totally
> settled and we use it for these types of devices.
> 
> But if not...then considering we will continue to keep compatibility with
> older simpler "pci" anyway, maybe it would be safer to add something
> simple now like "pci","mac" or just "mac" and keep compatibility for that
> when the new syntax is finally agreed in DPDK. It may mean some parsing to
> distinguish pci from mac, or vdev from pci,mac though.
> 
> Anyway, agreed syntax in DPDK is better so hopefully that can be done.
> 
> > This patch is basically based on the one from Ciara:
> >
> > https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/339496.htm
> > l
> >
> > Cc: Loftus Ciara <ciara.loftus at intel.com>
> > Cc: Thomas Monjalon <thomas at monjalon.net>
> > Cc: Kevin Traynor <ktraynor at redhat.com>
> > Signed-off-by: Yuanhan Liu <yliu at fridaylinux.org>
> > ---
> >  lib/netdev-dpdk.c | 77
> > ++++++++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 62 insertions(+), 15 deletions(-)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index
> > 45fcc74..4e5cc25 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -1205,30 +1205,77 @@ netdev_dpdk_lookup_by_port_id(dpdk_port_t
> port_id)
> >      return NULL;
> >  }
> >
> > +static int
> > +netdev_dpdk_str_to_ether(const char *mac, struct ether_addr *ea) {
> > +    unsigned int bytes[6];
> > +    int i;
> > +
> > +    if (sscanf(mac, "%x:%x:%x:%x:%x:%x",
> > +               &bytes[0], &bytes[1], &bytes[2],
> > +               &bytes[3], &bytes[4], &bytes[5]) != 6) {
> > +        return -1;

Should an error be logged here? I flag the same question when checking the return type for this function later but something to think about. The error log could be here or after the return type check but I do think it's useful.

> > +    }
> > +
> > +    for (i = 0; i < 6; i++) {
> > +        ea->addr_bytes[i] = bytes[i];
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static dpdk_port_t
> > +netdev_dpdk_get_port_by_mac(const char *mac_str) {
> > +    int i;
> > +    struct ether_addr mac;
> > +    struct ether_addr port_mac;
> > +
> > +    netdev_dpdk_str_to_ether(mac_str, &mac);

I think there should be an error check for the call to netdev_dpdk_str_to_ether() above and an associated error log, in the case where sscanf fails within netdev_dpdk_str_to_ether() (i.e. Mac is too long, too short etc.) it will help with the logs to zero in on the issue.

> > +    for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
> > +        if (!rte_eth_dev_is_valid_port(i)) {
> > +            continue;
> > +        }
> > +
> > +        rte_eth_macaddr_get(i, &port_mac);
> > +        if (is_same_ether_addr(&mac, &port_mac)) {
> > +            return i;
> > +        }
> > +    }
> > +
> > +    return DPDK_ETH_PORT_ID_INVALID;
> > +}
> > +

In general for this function I'd like to see a comment added explaining the behavior now that there are 2 methods to add ports.

> >  static dpdk_port_t
> >  netdev_dpdk_process_devargs(struct netdev_dpdk *dev,
> >                              const char *devargs, char **errp)  {
> > -    /* Get the name up to the first comma. */
> > -    char *name = xmemdup0(devargs, strcspn(devargs, ","));
> > +    char *name;
> >      dpdk_port_t new_port_id = DPDK_ETH_PORT_ID_INVALID;
> >
> > -    if (rte_eth_dev_get_port_by_name(name, &new_port_id)
> > -            || !rte_eth_dev_is_valid_port(new_port_id)) {
> > -        /* Device not found in DPDK, attempt to attach it */
> > -        if (!rte_eth_dev_attach(devargs, &new_port_id)) {
> > -            /* Attach successful */
> > -            dev->attached = true;
> > -            VLOG_INFO("Device '%s' attached to DPDK", devargs);
> > -        } else {
> > -            /* Attach unsuccessful */
> > -            new_port_id = DPDK_ETH_PORT_ID_INVALID;
> > -            VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK",
> > -                          devargs);
> > +    if (strncmp(devargs, "class=eth,mac=", 14) == 0) {
> > +        new_port_id = netdev_dpdk_get_port_by_mac(&devargs[14]);
> > +    } else {
> 
> It means that hotplug attach will not work for these devices - that needs
> to be documented too. Ian also mentioned concerns about how detach would
> work in one of the previous threads
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-October/339738.html

+1 on hotplug functionality, needs to be documented when it is available and how different port attach methods will interact with it.

Thanks
Ian

> 
> Kevin.
> 
> > +        name = xmemdup0(devargs, strcspn(devargs, ","));
> > +        if (rte_eth_dev_get_port_by_name(name, &new_port_id)
> > +                || !rte_eth_dev_is_valid_port(new_port_id)) {
> > +            /* Device not found in DPDK, attempt to attach it */
> > +            if (!rte_eth_dev_attach(devargs, &new_port_id)) {
> > +                /* Attach successful */
> > +                dev->attached = true;
> > +                VLOG_INFO("Device '%s' attached to DPDK", devargs);
> > +            } else {
> > +                /* Attach unsuccessful */
> > +                new_port_id = DPDK_ETH_PORT_ID_INVALID;
> > +            }
> >          }
> > +        free(name);
> > +    }
> > +
> > +    if (new_port_id == DPDK_ETH_PORT_ID_INVALID) {
> > +        VLOG_WARN_BUF(errp, "Error attaching device '%s' to DPDK",
> > + devargs);
> >      }
> >
> > -    free(name);
> >      return new_port_id;
> >  }
> >
> >