[dpdk-dev,v2] net/bonding: support bifurcated driver in eal cli using --vdev
Checks
Commit Message
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(-)
@@ -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)
Comments
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
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,
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,
>
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,
>>
>
>
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,
>>
>
>
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,
>>>
>>
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
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;
}