Re: [PATCH v2] net/bonding: support bifurcated driver in eal cli using --vdev

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

Hi Gowrishankar, Declan,

On Mon, Jul 10, 2017 at 12:02:24PM +0530, gowrishankar muthukrishnan wrote:
> On Friday 07 July 2017 09:08 PM, Declan Doherty wrote:
> >On 04/07/2017 12:57 PM, Gowrishankar wrote:
> >>From: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>
> >>
> >>At present, creating bonding devices using --vdev is broken for PMD like
> >>mlx5 as it is neither UIO nor VFIO based and hence PMD driver is unknown
> >>to find_port_id_by_pci_addr(), as below.
> >>
> >>testpmd <EAL args> --vdev 'net_bonding0,mode=1,slave=<PCI>,socket_id=0'
> >>
> >>PMD: bond_ethdev_parse_slave_port_kvarg(150) - Invalid slave port value
> >> (<PCI ID>) specified
> >>EAL: Failed to parse slave ports for bonded device net_bonding0
> >>
> >>This patch fixes parsing PCI ID from bonding device params by verifying
> >>it in RTE PCI bus, rather than checking dev->kdrv.
> >>
> >>Changes:
> >>  v2 - revisit fix by iterating rte_pci_bus
> >>
> >>Signed-off-by: Gowrishankar Muthukrishnan
> >><gowrishankar.m@linux.vnet.ibm.com>
> >>---
> >...
> >>
> >
> >Hey Gowrishankar,
> >
> >I was having a look at this patch and there is the following checkpatch
> >error.
> >
> >_coding style issues_
> >
> >
> >WARNING:AVOID_EXTERNS: externs should be avoided in .c files
> >#48: FILE: drivers/net/bonding/rte_eth_bond_args.c:43:
> >+extern struct rte_pci_bus rte_pci_bus;
> >
> Hi Declan,
> Thank you for your review.
> Yes, but I also saw some references like above in older code.
> 
> >
> >Looking at bit closer at the issue I think there is a simpler solution,
> >the bonding driver really shouldn't be parsing the PCI bus directly, and
> >since PCI devices use the PCI DBF as their name we can simply replace the
> >all the scanning code with a simple call to rte_eth_dev_get_port_by_name
> >API.
> >

I agree that it would be better to be able to use the ether API for
this.

The issue is that PCI devices are inconsistent regarding their names. The
possibility is given to the user to employ the simplified BDF format for
PCI device name, instead of the DomBDF format.

Unfortunately, the default device name for a PCI device is in the DomBDF
format. This means that the name won't match if the device was probed by
using the PCI blacklist mode (the default PCI mode).

The matching must be refined.

> 
> But you are removing an option to mention ports by PCI addresses right  (as
> I see parse_port_id() completely removed in your patch) ?.
> IMO, we just need to check if given eth pci id (incase we mention ports ib
> PCI ID) is one of what EAL scanned in PCI. Also, slaves should not be from
> any blacklisted PCI ids (as we test with -b or -w).
> 

Declan is right about the iteration of PCI devices. The device list for
the PCI bus is private, the extern declaration to the rte_pci_bus is the
telltale sign that there is something wrong in the approach here.

In order to respect the new rte_bus logic, I think what you want to
achieve can be done by using the rte_bus->find_device with the correct
device comparison function.

static int
pci_addr_cmp(const struct rte_device *dev, const void *_pci_addr)
{
    struct rte_pci_device *pdev;
    char *addr = _pci_addr;
    struct rte_pci_addr paddr;
    static struct rte_bus *pci_bus = NULL;

    if (pci_bus == NULL)
        pci_bus = rte_bus_find_by_name("pci");

    if (pci_bus->parse(addr, &paddr) != 0) {
        /* Invalid PCI addr given as input. */
        return -1;
    }
    pdev = RTE_DEV_TO_PCI(dev);
    return rte_eal_compare_pci_addr(&pdev->addr, &paddr);
}

Then verify that you are able to get a device by using it as follows:

{
    struct rte_bus *pci_bus;
    struct rte_device *dev;

    pci_bus = rte_bus_find_by_name("pci");
    if (pci_bus == NULL) {
        RTE_LOG(ERR, PMD, "Unable to find PCI bus\n");
        return -1;
    }
    dev = pci_bus->find_device(NULL, pci_addr_cmp, devname);
    if (dev == NULL) {
        RTE_LOG(ERR, PMD, "Unable to find the device %s to enslave.\n",
                devname);
        return -EINVAL;
    }
}

I hope it's clear enough. You can find examples of use for this API in
lib/librte_eal/common/eal_common_dev.c

It's a quick implementation to outline the possible direction, I
haven't compiled it. It should be refined.

For example, the PCI address validation should not be happening in the
comparison function, the pci_bus could be matched once instead of twice,
etc...

But the logic should work.

Best regards,
-- 
Gaëtan Rivet
6WIND