From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Monjalon Subject: Re: [PATCH v2] net/bonding: support bifurcated driver in eal cli using --vdev Date: Tue, 05 Sep 2017 11:13:00 +0200 Message-ID: <19317053.0GOQUN8yPN@xps> References:

<20170731143458.GL11154@bidouze.vm.6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Cc: dev@dpdk.org, =?ISO-8859-1?Q?Ga=EBtan?= Rivet , Declan Doherty , Ferruh Yigit , rasland@mellanox.com To: gowrishankar muthukrishnan Return-path: Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id 879AB3254 for ; Tue, 5 Sep 2017 11:13:02 +0200 (CEST) In-Reply-To: <20170731143458.GL11154@bidouze.vm.6wind.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Ping - any news? 31/07/2017 16:34, Ga=EBtan Rivet: > Hi Gowrishankar, Declan, >=20 > On Mon, Jul 10, 2017 at 12:02:24PM +0530, gowrishankar muthukrishnan wrot= e: > > On Friday 07 July 2017 09:08 PM, Declan Doherty wrote: > > >On 04/07/2017 12:57 PM, Gowrishankar wrote: > > >>From: Gowrishankar Muthukrishnan > > >> > > >>At present, creating bonding devices using --vdev is broken for PMD l= ike > > >>mlx5 as it is neither UIO nor VFIO based and hence PMD driver is unkn= own > > >>to find_port_id_by_pci_addr(), as below. > > >> > > >>testpmd --vdev 'net_bonding0,mode=3D1,slave=3D,socket= _id=3D0' > > >> > > >>PMD: bond_ethdev_parse_slave_port_kvarg(150) - Invalid slave port val= ue > > >> () specified > > >>EAL: Failed to parse slave ports for bonded device net_bonding0 > > >> > > >>This patch fixes parsing PCI ID from bonding device params by verifyi= ng > > >>it in RTE PCI bus, rather than checking dev->kdrv. > > >> > > >>Changes: > > >> v2 - revisit fix by iterating rte_pci_bus > > >> > > >>Signed-off-by: Gowrishankar Muthukrishnan > > >> > > >>--- > > >... > > >> > > > > > >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. > >=20 > > > > > >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, a= nd > > >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_na= me > > >API. > > > >=20 > I agree that it would be better to be able to use the ether API for > this. >=20 > 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. >=20 > 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). >=20 > The matching must be refined. >=20 > >=20 > > 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 f= rom > > any blacklisted PCI ids (as we test with -b or -w). > >=20 >=20 > 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. >=20 > 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. >=20 > static int > pci_addr_cmp(const struct rte_device *dev, const void *_pci_addr) > { > struct rte_pci_device *pdev; > char *addr =3D _pci_addr; > struct rte_pci_addr paddr; > static struct rte_bus *pci_bus =3D NULL; >=20 > if (pci_bus =3D=3D NULL) > pci_bus =3D rte_bus_find_by_name("pci"); >=20 > if (pci_bus->parse(addr, &paddr) !=3D 0) { > /* Invalid PCI addr given as input. */ > return -1; > } > pdev =3D RTE_DEV_TO_PCI(dev); > return rte_eal_compare_pci_addr(&pdev->addr, &paddr); > } >=20 > Then verify that you are able to get a device by using it as follows: >=20 > { > struct rte_bus *pci_bus; > struct rte_device *dev; >=20 > pci_bus =3D rte_bus_find_by_name("pci"); > if (pci_bus =3D=3D NULL) { > RTE_LOG(ERR, PMD, "Unable to find PCI bus\n"); > return -1; > } > dev =3D pci_bus->find_device(NULL, pci_addr_cmp, devname); > if (dev =3D=3D NULL) { > RTE_LOG(ERR, PMD, "Unable to find the device %s to enslave.\n", > devname); > return -EINVAL; > } > } >=20 > I hope it's clear enough. You can find examples of use for this API in > lib/librte_eal/common/eal_common_dev.c >=20 > It's a quick implementation to outline the possible direction, I > haven't compiled it. It should be refined. >=20 > 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... >=20 > But the logic should work. >=20 > Best regards, >=20