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

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

[Gowrishankar]

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 adds RTE_KDRV_BIFURCATED in rte_kernel_driver for the PMD
driver like mlx5 to be a known one.

Signed-off-by: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>
---
 lib/librte_eal/common/include/rte_pci.h | 1 +
 lib/librte_eal/linuxapp/eal/eal_pci.c   | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
index b82ab9e..bbfd50d 100644
--- a/lib/librte_eal/common/include/rte_pci.h
+++ b/lib/librte_eal/common/include/rte_pci.h
@@ -126,6 +126,7 @@ enum rte_kernel_driver {
 	RTE_KDRV_VFIO,
 	RTE_KDRV_UIO_GENERIC,
 	RTE_KDRV_NIC_UIO,
+	RTE_KDRV_BIFURCATED,
 	RTE_KDRV_NONE,
 };
 
diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 595622b..a222ec3 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -351,6 +351,8 @@
 			dev->kdrv = RTE_KDRV_IGB_UIO;
 		else if (!strcmp(driver, "uio_pci_generic"))
 			dev->kdrv = RTE_KDRV_UIO_GENERIC;
+		else if (!strcmp(driver, "mlx5_core"))
+			dev->kdrv = RTE_KDRV_BIFURCATED;
 		else
 			dev->kdrv = RTE_KDRV_UNKNOWN;
 	} else
-- 
1.9.1

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

[Ferruh Yigit]

On 6/14/2017 11:49 AM, 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 adds RTE_KDRV_BIFURCATED in rte_kernel_driver for the PMD
> driver like mlx5 to be a known one.
> 
> Signed-off-by: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>

Patch is for bonding but changes are in eal and related to mlx support,
so adding more developers may be interested.

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

[Thomas Monjalon]

14/06/2017 12:49, Gowrishankar:
> 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

Thanks for reporting.

> This patch adds RTE_KDRV_BIFURCATED in rte_kernel_driver for the PMD
> driver like mlx5 to be a known one.

The current kdrv value should be RTE_KDRV_UNKNOWN for Mellanox devices.
I do not see the value of creating a new type.

I think the issue is in the bonding code (find_port_id_by_pci_addr):

	* TODO: Once the PCI bus has arrived we should have a better
	* way to test for being a PCI device or not.
	*/
	if (rte_eth_devices[i].data->kdrv == RTE_KDRV_UNKNOWN ||
	    rte_eth_devices[i].data->kdrv == RTE_KDRV_NONE)
			continue;

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

[Gowrishankar]

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>
---
 drivers/net/bonding/rte_eth_bond_args.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
index c718e61..180586c 100644
--- a/drivers/net/bonding/rte_eth_bond_args.c
+++ b/drivers/net/bonding/rte_eth_bond_args.c
@@ -40,6 +40,8 @@
 #include "rte_eth_bond.h"
 #include "rte_eth_bond_private.h"
 
+extern struct rte_pci_bus rte_pci_bus;
+
 const char *pmd_bond_init_valid_arguments[] = {
 	PMD_BOND_SLAVE_PORT_KVARG,
 	PMD_BOND_PRIMARY_SLAVE_KVARG,
@@ -59,16 +61,6 @@
 	unsigned i;
 
 	for (i = 0; i < rte_eth_dev_count(); i++) {
-
-		/* Currently populated by rte_eth_copy_pci_info().
-		 *
-		 * TODO: Once the PCI bus has arrived we should have a better
-		 * way to test for being a PCI device or not.
-		 */
-		if (rte_eth_devices[i].data->kdrv == RTE_KDRV_UNKNOWN ||
-		    rte_eth_devices[i].data->kdrv == RTE_KDRV_NONE)
-			continue;
-
 		pci_dev = RTE_ETH_DEV_TO_PCI(&rte_eth_devices[i]);
 		eth_pci_addr = &pci_dev->addr;
 
@@ -103,12 +95,18 @@
 static inline int
 parse_port_id(const char *port_str)
 {
+	struct rte_pci_device *dev;
 	struct rte_pci_addr dev_addr;
-	int port_id;
+	int port_id = -1;
 
 	/* try parsing as pci address, physical devices */
 	if (eal_parse_pci_DomBDF(port_str, &dev_addr) == 0) {
-		port_id = find_port_id_by_pci_addr(&dev_addr);
+		FOREACH_DEVICE_ON_PCIBUS(dev) {
+			if (rte_eal_compare_pci_addr(&dev->addr, &dev_addr))
+				continue;
+
+			port_id = find_port_id_by_pci_addr(&dev_addr);
+		}
 		if (port_id < 0)
 			return -1;
 	} else {
-- 
1.9.1

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

[Declan Doherty]

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;


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.

Can you try the following changes which removes any dependency on 
parsing the PCI bus tree directly, which works fine for me with UIO 
devices. If this works I'll send this fix to the list.

Thanks
Declan


---
  drivers/net/bonding/rte_eth_bond_args.c | 109 
++++----------------------------
  1 file changed, 14 insertions(+), 95 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_args.c 
b/drivers/net/bonding/rte_eth_bond_args.c
index ffab6e0..9fb98c6 100644
--- a/drivers/net/bonding/rte_eth_bond_args.c
+++ b/drivers/net/bonding/rte_eth_bond_args.c
@@ -40,7 +40,6 @@
  #include "rte_eth_bond.h"
  #include "rte_eth_bond_private.h"

-extern struct rte_pci_bus rte_pci_bus;

  const char *pmd_bond_init_valid_arguments[] = {
  	PMD_BOND_SLAVE_PORT_KVARG,
@@ -53,104 +52,22 @@ const char *pmd_bond_init_valid_arguments[] = {
  	NULL
  };

-static inline int
-find_port_id_by_pci_addr(const struct rte_pci_addr *pci_addr)
-{
-	struct rte_pci_device *pci_dev;
-	struct rte_pci_addr *eth_pci_addr;
-	unsigned i;
-
-	for (i = 0; i < rte_eth_dev_count(); i++) {
-		pci_dev = RTE_ETH_DEV_TO_PCI(&rte_eth_devices[i]);
-		eth_pci_addr = &pci_dev->addr;
-
-		if (pci_addr->bus == eth_pci_addr->bus &&
-			pci_addr->devid == eth_pci_addr->devid &&
-			pci_addr->domain == eth_pci_addr->domain &&
-			pci_addr->function == eth_pci_addr->function)
-			return i;
-	}
-	return -1;
-}
-
-static inline int
-find_port_id_by_dev_name(const char *name)
-{
-	unsigned i;
-
-	for (i = 0; i < rte_eth_dev_count(); i++) {
-		if (rte_eth_devices[i].data == NULL)
-			continue;
-
-		if (strcmp(rte_eth_devices[i].device->name, name) == 0)
-			return i;
-	}
-	return -1;
-}
-
-/**
- * Parses a port identifier string to a port id by pci address, then by 
name,
- * and finally port id.
- */
-static inline int
-parse_port_id(const char *port_str)
-{
-	struct rte_pci_device *dev;
-	struct rte_pci_addr dev_addr;
-	int port_id = -1;
-
-	/* try parsing as pci address, physical devices */
-	if (eal_parse_pci_DomBDF(port_str, &dev_addr) == 0) {
-		FOREACH_DEVICE_ON_PCIBUS(dev) {
-			if (rte_eal_compare_pci_addr(&dev->addr, &dev_addr))
-				continue;
-
-			port_id = find_port_id_by_pci_addr(&dev_addr);
-		}
-		if (port_id < 0)
-			return -1;
-	} else {
-		/* try parsing as device name, virtual devices */
-		port_id = find_port_id_by_dev_name(port_str);
-		if (port_id < 0) {
-			char *end;
-			errno = 0;
-
-			/* try parsing as port id */
-			port_id = strtol(port_str, &end, 10);
-			if (*end != 0 || errno != 0)
-				return -1;
-		}
-	}
-
-	if (port_id < 0 || port_id > RTE_MAX_ETHPORTS) {
-		RTE_BOND_LOG(ERR, "Slave port specified (%s) outside expected range",
-				port_str);
-		return -1;
-	}
-	return port_id;
-}
-
  int
-bond_ethdev_parse_slave_port_kvarg(const char *key,
+bond_ethdev_parse_slave_port_kvarg(const char *key __rte_unused,
  		const char *value, void *extra_args)
  {
-	struct bond_ethdev_slave_ports *slave_ports;
+	struct bond_ethdev_slave_ports *slaves;
+	uint8_t pid;

  	if (value == NULL || extra_args == NULL)
  		return -1;

-	slave_ports = extra_args;
+	if (rte_eth_dev_get_port_by_name(value, &pid) < 0)
+		return -1;
+
+	slaves = extra_args;
+	slaves->slaves[slaves->slave_count++] = pid;

-	if (strcmp(key, PMD_BOND_SLAVE_PORT_KVARG) == 0) {
-		int port_id = parse_port_id(value);
-		if (port_id < 0) {
-			RTE_BOND_LOG(ERR, "Invalid slave port value (%s) specified", value);
-			return -1;
-		} else
-			slave_ports->slaves[slave_ports->slave_count++] =
-					(uint8_t)port_id;
-	}
  	return 0;
  }

@@ -214,20 +131,22 @@ int
  bond_ethdev_parse_primary_slave_port_id_kvarg(const char *key 
__rte_unused,
  		const char *value, void *extra_args)
  {
-	int primary_slave_port_id;
+	uint8_t pid, *primary_pid;

  	if (value == NULL || extra_args == NULL)
  		return -1;

-	primary_slave_port_id = parse_port_id(value);
-	if (primary_slave_port_id < 0)
+	if (rte_eth_dev_get_port_by_name(value, &pid) < 0)
  		return -1;

-	*(uint8_t *)extra_args = (uint8_t)primary_slave_port_id;
+	primary_pid = (uint8_t *)extra_args;
+	*primary_pid = pid;

  	return 0;
  }

+
+
  int
  bond_ethdev_parse_balance_xmit_policy_kvarg(const char *key __rte_unused,
  		const char *value, void *extra_args)
-- 
2.9.4

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

[gowrishankar muthukrishnan]

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.
>

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

With your patch, it failed to parse params as below.

testpmd -l 0,8,16 --socket-mem 512,512 --vdev 
'net_bonding0,mode=1,slave=0002:01:00.0,slave=0002:01:00.1,primary=0002:01:00.0,socket_id=0' 


Configuring Port 0 (socket 1)
PMD: net_mlx5: 0x4a7f8f80: TX queues number update: 0 -> 1
PMD: net_mlx5: 0x4a7f8f80: RX queues number update: 0 -> 1
Port 0: E4:1D:2D:C9:C7:56
Configuring Port 1 (socket 1)
PMD: net_mlx5: 0x4a7fd000: TX queues number update: 0 -> 1
PMD: net_mlx5: 0x4a7fd000: RX queues number update: 0 -> 1
Port 1: E4:1D:2D:C9:C7:57
Configuring Port 2 (socket 0)
EAL: Failed to parse slave ports for bonded device net_bonding0
Fail to configure port 2
EAL: Error - exiting with code: 1
   Cause: Start ports failed

With my patch, I have tested with -w and -b options in testpmd as well.

Please let me know if I am wrong on anything of above.

Thanks,
Gowrishankar

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

[Gaëtan Rivet]

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

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

[Thomas Monjalon]

Ping - any news?

31/07/2017 16:34, Gaëtan Rivet:
> 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,
> 

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

[gowrishankar muthukrishnan]

Hi Thomas,
I will rework on my patch with these suggestions and send new version.
Thanks Declan and Gaëtan. Thank you Thomas too reminding me.

Regards,
Gowrishankar

On Tuesday 05 September 2017 02:43 PM, Thomas Monjalon wrote:
> Ping - any news?
>
> 31/07/2017 16:34, Gaëtan Rivet:
>> 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,
>>
>
>

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

[Raslan Darawsheh]

Hi Guys,
This is gentle remainder of this patch,
Do we have any updates about it?

Kindest regards
Raslan Darawsheh

-----Original Message-----
From: gowrishankar muthukrishnan [mailto:gowrishankar.m@linux.vnet.ibm.com] 
Sent: Wednesday, September 6, 2017 11:59 AM
To: Thomas Monjalon <thomas@monjalon.net>
Cc: dev@dpdk.org; Gaëtan Rivet <gaetan.rivet@6wind.com>; Declan Doherty <declan.doherty@intel.com>; Ferruh Yigit <ferruh.yigit@intel.com>; Raslan Darawsheh <rasland@mellanox.com>
Subject: [Suspected-Phishing]Re: [dpdk-dev] [PATCH v2] net/bonding: support bifurcated driver in eal cli using --vdev

Hi Thomas,
I will rework on my patch with these suggestions and send new version.
Thanks Declan and Gaëtan. Thank you Thomas too reminding me.

Regards,
Gowrishankar

On Tuesday 05 September 2017 02:43 PM, Thomas Monjalon wrote:
> Ping - any news?
>
> 31/07/2017 16:34, Gaëtan Rivet:
>> 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,
>>
>
>

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

[gowrishankar muthukrishnan]

Hi Raslan,
I had submitted newer version and waiting for ack/merge.

dpdk.org/dev/patchwork/patch/29039/

Thanks,
Gowrishankar

On Monday 02 October 2017 02:11 PM, Raslan Darawsheh wrote:
> Hi Guys,
> This is gentle remainder of this patch,
> Do we have any updates about it?
>
> Kindest regards
> Raslan Darawsheh
>
> -----Original Message-----
> From: gowrishankar muthukrishnan [mailto:gowrishankar.m@linux.vnet.ibm.com]
> Sent: Wednesday, September 6, 2017 11:59 AM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Gaëtan Rivet <gaetan.rivet@6wind.com>; Declan Doherty <declan.doherty@intel.com>; Ferruh Yigit <ferruh.yigit@intel.com>; Raslan Darawsheh <rasland@mellanox.com>
> Subject: [Suspected-Phishing]Re: [dpdk-dev] [PATCH v2] net/bonding: support bifurcated driver in eal cli using --vdev
>
> Hi Thomas,
> I will rework on my patch with these suggestions and send new version.
> Thanks Declan and Gaëtan. Thank you Thomas too reminding me.
>
> Regards,
> Gowrishankar
>
> On Tuesday 05 September 2017 02:43 PM, Thomas Monjalon wrote:
>> Ping - any news?
>>
>> 31/07/2017 16:34, Gaëtan Rivet:
>>> 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,
>>>
>>

-- 
Regards,
Gowrishankar M
Linux Networking

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

[Raslan Darawsheh]

Hi, 
I've just tested it and looks like the issue is fixed with this patch.

Kindest regards
Raslan Darawsheh

-----Original Message-----
From: gowrishankar muthukrishnan [mailto:gowrishankar.m@linux.vnet.ibm.com] 
Sent: Monday, October 2, 2017 11:44 AM
To: Raslan Darawsheh <rasland@mellanox.com>; Thomas Monjalon <thomas@monjalon.net>
Cc: dev@dpdk.org; Gaëtan Rivet <gaetan.rivet@6wind.com>; Declan Doherty <declan.doherty@intel.com>; Ferruh Yigit <ferruh.yigit@intel.com>
Subject: [Suspected-Phishing]Re: [Suspected-Phishing]Re: [dpdk-dev] [PATCH v2] net/bonding: support bifurcated driver in eal cli using --vdev

Hi Raslan,
I had submitted newer version and waiting for ack/merge.

dpdk.org/dev/patchwork/patch/29039/

Thanks,
Gowrishankar

On Monday 02 October 2017 02:11 PM, Raslan Darawsheh wrote:
> Hi Guys,
> This is gentle remainder of this patch, Do we have any updates about 
> it?
>
> Kindest regards
> Raslan Darawsheh
>
> -----Original Message-----
> From: gowrishankar muthukrishnan 
> [mailto:gowrishankar.m@linux.vnet.ibm.com]
> Sent: Wednesday, September 6, 2017 11:59 AM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Gaëtan Rivet <gaetan.rivet@6wind.com>; Declan 
> Doherty <declan.doherty@intel.com>; Ferruh Yigit 
> <ferruh.yigit@intel.com>; Raslan Darawsheh <rasland@mellanox.com>
> Subject: [Suspected-Phishing]Re: [dpdk-dev] [PATCH v2] net/bonding: 
> support bifurcated driver in eal cli using --vdev
>
> Hi Thomas,
> I will rework on my patch with these suggestions and send new version.
> Thanks Declan and Gaëtan. Thank you Thomas too reminding me.
>
> Regards,
> Gowrishankar
>
> On Tuesday 05 September 2017 02:43 PM, Thomas Monjalon wrote:
>> Ping - any news?
>>
>> 31/07/2017 16:34, Gaëtan Rivet:
>>> 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,
>>>
>>

--
Regards,
Gowrishankar M
Linux Networking

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

[Gowrishankar]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 2802 bytes --]

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.

Fixes: eac901ce ("ethdev: decouple from PCI device")
Signed-off-by: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>
---
v3:
 - adapt rte_bus API (with suggestions from Declan and Gaëtan)

 drivers/net/bonding/rte_eth_bond_args.c | 35 ++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
index bb634c6..7c65dda 100644
--- a/drivers/net/bonding/rte_eth_bond_args.c
+++ b/drivers/net/bonding/rte_eth_bond_args.c
@@ -61,16 +61,6 @@
 	unsigned i;
 
 	for (i = 0; i < rte_eth_dev_count(); i++) {
-
-		/* Currently populated by rte_eth_copy_pci_info().
-		 *
-		 * TODO: Once the PCI bus has arrived we should have a better
-		 * way to test for being a PCI device or not.
-		 */
-		if (rte_eth_devices[i].data->kdrv == RTE_KDRV_UNKNOWN ||
-		    rte_eth_devices[i].data->kdrv == RTE_KDRV_NONE)
-			continue;
-
 		pci_dev = RTE_ETH_DEV_TO_PCI(&rte_eth_devices[i]);
 		eth_pci_addr = &pci_dev->addr;
 
@@ -98,6 +88,16 @@
 	return -1;
 }
 
+static inline int
+pci_addr_cmp(const struct rte_device *dev, const void *_pci_addr)
+{
+	struct rte_pci_device *pdev;
+	const struct rte_pci_addr *paddr = _pci_addr;
+
+	pdev = RTE_DEV_TO_PCI(*(struct rte_device **)(void *)&dev);
+	return rte_eal_compare_pci_addr(&pdev->addr, paddr);
+}
+
 /**
  * Parses a port identifier string to a port id by pci address, then by name,
  * and finally port id.
@@ -106,10 +106,23 @@
 parse_port_id(const char *port_str)
 {
 	struct rte_pci_addr dev_addr;
+	struct rte_bus *pci_bus;
+	struct rte_device *dev;
 	int port_id;
 
+	pci_bus = rte_bus_find_by_name("pci");
+	if (pci_bus == NULL) {
+		RTE_LOG(ERR, PMD, "unable to find PCI bus\n");
+		return -1;
+	}
+
 	/* try parsing as pci address, physical devices */
-	if (eal_parse_pci_DomBDF(port_str, &dev_addr) == 0) {
+	if (pci_bus->parse(port_str, &dev_addr) == 0) {
+		dev = pci_bus->find_device(NULL, pci_addr_cmp, &dev_addr);
+		if (dev == NULL) {
+			RTE_LOG(ERR, PMD, "unable to find PCI device\n");
+			return -1;
+		}
 		port_id = find_port_id_by_pci_addr(&dev_addr);
 		if (port_id < 0)
 			return -1;
-- 
1.9.1

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

[Doherty, Declan]

On 20/09/2017 7:04 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.
> 
> Fixes: eac901ce ("ethdev: decouple from PCI device")
> Signed-off-by: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>
> ---
> v3:
>   - adapt rte_bus API (with suggestions from Declan and Gaëtan)
> 
...
> 

Acked-by: Declan Doherty <declan.doherty@intel.com>

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

[Ferruh Yigit]

On 10/2/2017 12:06 PM, Doherty, Declan wrote:
> On 20/09/2017 7:04 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.
>>
>> Fixes: eac901ce ("ethdev: decouple from PCI device")
>> Signed-off-by: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>
>> ---
<...>
> Acked-by: Declan Doherty <declan.doherty@intel.com>

Reviewed-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Applied to dpdk-next-net/master, thanks.

(Gaetan, we may ask your help during merge on this.)

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

[Gaëtan Rivet]

Hi Gowrishankar,

There will be a trivial conflict with my PCI patchset on the
pci_addr_cmp function. I don't know the best way to solve this.

It depends on my patchset being accepted as-is or not, and which
address namespace has precedence over the other.

You could rename pci_addr_cmp with a reference to the bonding namespace,
it is always nice when debugging to know that we are within the bonding
PMD, even if the function is static inlined...

Anyway, on the principle anyway the code seems ok to me, so

Reviewed-by: Gaetan Rivet <gaetan.rivet@6wind.com>

On Wed, Sep 20, 2017 at 11:34:58PM +0530, 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.
> 
> Fixes: eac901ce ("ethdev: decouple from PCI device")
> Signed-off-by: Gowrishankar Muthukrishnan <gowrishankar.m@linux.vnet.ibm.com>
> ---
> v3:
>  - adapt rte_bus API (with suggestions from Declan and Gaëtan)
> 
>  drivers/net/bonding/rte_eth_bond_args.c | 35 ++++++++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_args.c b/drivers/net/bonding/rte_eth_bond_args.c
> index bb634c6..7c65dda 100644
> --- a/drivers/net/bonding/rte_eth_bond_args.c
> +++ b/drivers/net/bonding/rte_eth_bond_args.c
> @@ -61,16 +61,6 @@
>  	unsigned i;
>  
>  	for (i = 0; i < rte_eth_dev_count(); i++) {
> -
> -		/* Currently populated by rte_eth_copy_pci_info().
> -		 *
> -		 * TODO: Once the PCI bus has arrived we should have a better
> -		 * way to test for being a PCI device or not.
> -		 */
> -		if (rte_eth_devices[i].data->kdrv == RTE_KDRV_UNKNOWN ||
> -		    rte_eth_devices[i].data->kdrv == RTE_KDRV_NONE)
> -			continue;
> -
>  		pci_dev = RTE_ETH_DEV_TO_PCI(&rte_eth_devices[i]);
>  		eth_pci_addr = &pci_dev->addr;
>  
> @@ -98,6 +88,16 @@
>  	return -1;
>  }
>  
> +static inline int
> +pci_addr_cmp(const struct rte_device *dev, const void *_pci_addr)
> +{
> +	struct rte_pci_device *pdev;
> +	const struct rte_pci_addr *paddr = _pci_addr;
> +
> +	pdev = RTE_DEV_TO_PCI(*(struct rte_device **)(void *)&dev);
> +	return rte_eal_compare_pci_addr(&pdev->addr, paddr);
> +}
> +
>  /**
>   * Parses a port identifier string to a port id by pci address, then by name,
>   * and finally port id.
> @@ -106,10 +106,23 @@
>  parse_port_id(const char *port_str)
>  {
>  	struct rte_pci_addr dev_addr;
> +	struct rte_bus *pci_bus;
> +	struct rte_device *dev;
>  	int port_id;
>  
> +	pci_bus = rte_bus_find_by_name("pci");
> +	if (pci_bus == NULL) {
> +		RTE_LOG(ERR, PMD, "unable to find PCI bus\n");
> +		return -1;
> +	}
> +
>  	/* try parsing as pci address, physical devices */
> -	if (eal_parse_pci_DomBDF(port_str, &dev_addr) == 0) {
> +	if (pci_bus->parse(port_str, &dev_addr) == 0) {
> +		dev = pci_bus->find_device(NULL, pci_addr_cmp, &dev_addr);
> +		if (dev == NULL) {
> +			RTE_LOG(ERR, PMD, "unable to find PCI device\n");
> +			return -1;
> +		}
>  		port_id = find_port_id_by_pci_addr(&dev_addr);
>  		if (port_id < 0)
>  			return -1;
> -- 
> 1.9.1
> 

-- 
Gaëtan Rivet
6WIND