[v2,1/1] bus/pci: optimise scanning with whitelist/blacklist
Checks
Commit Message
rte_bus_scan API scans all the available PCI devices irrespective of white
or black listing parameters then further devices are probed based on white
or black listing parameters. So unnecessary CPU cycles are wasted during
rte_pci_scan.
For Octeontx2 platform with core frequency 2.4 Ghz, rte_bus_scan consumes
around 26ms to scan around 90 PCI devices but all may not be used by the
application. So for the application which uses 2 NICs, rte_bus_scan
consumes few microseconds and rest time is saved with this patch.
Patch restricts devices to be scanned as per below mentioned conditions:
- All devices will be scanned if no parameters are passed.
- Only white listed devices will be scanned if white list is available.
- All devices, except black listed, will be scanned if black list is
available.
Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
---
v2:
- Added function to validate ignorance of device based on PCI address.
- Marked device validation function as experimental.
drivers/bus/pci/bsd/pci.c | 15 ++++++++++-
drivers/bus/pci/linux/pci.c | 3 +++
drivers/bus/pci/pci_common.c | 34 +++++++++++++++++++++++++
drivers/bus/pci/private.h | 12 +++++++++
drivers/bus/pci/rte_bus_pci_version.map | 5 ++++
5 files changed, 68 insertions(+), 1 deletion(-)
Comments
Hello David and Stephen,
I have uploaded second version of patch as per your suggestion.
Please have a look and ack if it looks okay.
Regards
Sunil Kumar Kori
>-----Original Message-----
>From: Sunil Kumar Kori <skori@marvell.com>
>Sent: Tuesday, April 7, 2020 2:59 PM
>To: stephen@networkplumber.org; david.marchand@redhat.com; Jerin Jacob
>Kollanukkaran <jerinj@marvell.com>
>Cc: dev@dpdk.org; Sunil Kumar Kori <skori@marvell.com>
>Subject: [PATCH v2 1/1] bus/pci: optimise scanning with whitelist/blacklist
>
>rte_bus_scan API scans all the available PCI devices irrespective of white or
>black listing parameters then further devices are probed based on white or
>black listing parameters. So unnecessary CPU cycles are wasted during
>rte_pci_scan.
>
>For Octeontx2 platform with core frequency 2.4 Ghz, rte_bus_scan consumes
>around 26ms to scan around 90 PCI devices but all may not be used by the
>application. So for the application which uses 2 NICs, rte_bus_scan consumes
>few microseconds and rest time is saved with this patch.
>
>Patch restricts devices to be scanned as per below mentioned conditions:
> - All devices will be scanned if no parameters are passed.
> - Only white listed devices will be scanned if white list is available.
> - All devices, except black listed, will be scanned if black list is
> available.
>
>Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
>---
>v2:
> - Added function to validate ignorance of device based on PCI address.
> - Marked device validation function as experimental.
>
> drivers/bus/pci/bsd/pci.c | 15 ++++++++++-
> drivers/bus/pci/linux/pci.c | 3 +++
> drivers/bus/pci/pci_common.c | 34 +++++++++++++++++++++++++
> drivers/bus/pci/private.h | 12 +++++++++
> drivers/bus/pci/rte_bus_pci_version.map | 5 ++++
> 5 files changed, 68 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c index
>ebbfeb13a..074f4556e 100644
>--- a/drivers/bus/pci/bsd/pci.c
>+++ b/drivers/bus/pci/bsd/pci.c
>@@ -338,6 +338,9 @@ rte_pci_scan(void)
> .match_buf_len = sizeof(matches),
> .matches = &matches[0],
> };
>+ struct rte_pci_addr pci_addr;
>+
>+ memset(&pci_addr, 0, sizeof(struct rte_pci_addr));
>
> /* for debug purposes, PCI can be disabled */
> if (!rte_eal_has_pci())
>@@ -357,9 +360,19 @@ rte_pci_scan(void)
> goto error;
> }
>
>- for (i = 0; i < conf_io.num_matches; i++)
>+ for (i = 0; i < conf_io.num_matches; i++) {
>+ pci_addr.domain = matches[i].pc_sel.pc_domain;
>+ pci_addr.bus = matches[i].pc_sel.pc_bus;
>+ pci_addr.devid = matches[i].pc_sel.pc_dev;
>+ pci_addr.function = matches[i].pc_sel.pc_func;
>+
>+ /* Check that device should be ignored or not */
>+ if (pci_addr_ignore_device(&pci_addr))
>+ continue;
>+
> if (pci_scan_one(fd, &matches[i]) < 0)
> goto error;
>+ }
>
> dev_count += conf_io.num_matches;
> } while(conf_io.status == PCI_GETCONF_MORE_DEVS); diff --git
>a/drivers/bus/pci/linux/pci.c b/drivers/bus/pci/linux/pci.c index
>71b0a3053..92bdad826 100644
>--- a/drivers/bus/pci/linux/pci.c
>+++ b/drivers/bus/pci/linux/pci.c
>@@ -487,6 +487,9 @@ rte_pci_scan(void)
> if (parse_pci_addr_format(e->d_name, sizeof(e->d_name),
>&addr) != 0)
> continue;
>
>+ if (pci_addr_ignore_device(&addr))
>+ continue;
>+
> snprintf(dirname, sizeof(dirname), "%s/%s",
> rte_pci_get_sysfs_path(), e->d_name);
>
>diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c
>index 3f5542076..a350a1993 100644
>--- a/drivers/bus/pci/pci_common.c
>+++ b/drivers/bus/pci/pci_common.c
>@@ -589,6 +589,40 @@ pci_dma_unmap(struct rte_device *dev, void *addr,
>uint64_t iova, size_t len)
> return -1;
> }
>
>+static struct rte_devargs *
>+pci_addr_devargs_lookup(const struct rte_pci_addr *pci_addr) {
>+ struct rte_devargs *devargs;
>+ struct rte_pci_addr addr;
>+
>+ RTE_EAL_DEVARGS_FOREACH("pci", devargs) {
>+ devargs->bus->parse(devargs->name, &addr);
>+ if (!rte_pci_addr_cmp(pci_addr, &addr))
>+ return devargs;
>+ }
>+ return NULL;
>+}
>+
>+bool
>+pci_addr_ignore_device(const struct rte_pci_addr *pci_addr) {
>+ struct rte_devargs *devargs = pci_addr_devargs_lookup(pci_addr);
>+
>+ switch (rte_pci_bus.bus.conf.scan_mode) {
>+ case RTE_BUS_SCAN_WHITELIST:
>+ if (devargs && devargs->policy == RTE_DEV_WHITELISTED)
>+ return false;
>+ break;
>+ case RTE_BUS_SCAN_UNDEFINED:
>+ case RTE_BUS_SCAN_BLACKLIST:
>+ if (devargs == NULL ||
>+ devargs->policy != RTE_DEV_BLACKLISTED)
>+ return false;
>+ break;
>+ }
>+ return true;
>+}
>+
> static bool
> pci_ignore_device(const struct rte_pci_device *dev) { diff --git
>a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h index
>a205d4d9f..25075b806 100644
>--- a/drivers/bus/pci/private.h
>+++ b/drivers/bus/pci/private.h
>@@ -42,6 +42,18 @@ int rte_pci_scan(void); void pci_name_set(struct
>rte_pci_device *dev);
>
>+/**
>+ * Validate whether a device with given pci address should be ignored or not.
>+ *
>+ * @param pci_addr
>+ * PCI address of device to be validated
>+ * @return
>+ * 1: if device is to be ignored,
>+ * 0: if device is to be scanned,
>+ */
>+__rte_experimental
>+bool pci_addr_ignore_device(const struct rte_pci_addr *pci_addr);
>+
> /**
> * Add a PCI device to the PCI Bus (append to PCI Device list). This function
> * also updates the bus references of the PCI Device (and the generic device
>diff --git a/drivers/bus/pci/rte_bus_pci_version.map
>b/drivers/bus/pci/rte_bus_pci_version.map
>index 012d817e1..da4ae2640 100644
>--- a/drivers/bus/pci/rte_bus_pci_version.map
>+++ b/drivers/bus/pci/rte_bus_pci_version.map
>@@ -16,3 +16,8 @@ DPDK_20.0 {
>
> local: *;
> };
>+
>+EXPERIMENTAL {
>+ # added in 20.05
>+ pci_addr_ignore_device;
>+};
>--
>2.17.1
On Tue, Apr 7, 2020 at 11:29 AM Sunil Kumar Kori <skori@marvell.com> wrote:
>
> rte_bus_scan API scans all the available PCI devices irrespective of white
> or black listing parameters then further devices are probed based on white
> or black listing parameters. So unnecessary CPU cycles are wasted during
> rte_pci_scan.
>
> For Octeontx2 platform with core frequency 2.4 Ghz, rte_bus_scan consumes
> around 26ms to scan around 90 PCI devices but all may not be used by the
> application. So for the application which uses 2 NICs, rte_bus_scan
> consumes few microseconds and rest time is saved with this patch.
>
> Patch restricts devices to be scanned as per below mentioned conditions:
> - All devices will be scanned if no parameters are passed.
> - Only white listed devices will be scanned if white list is available.
> - All devices, except black listed, will be scanned if black list is
> available.
>
> Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
> ---
> v2:
> - Added function to validate ignorance of device based on PCI address.
First you objected to Stephen comment, and later announced there was
no objection.
Now, it seems you ignored what I replied without any explanation.
So tell me, what was wrong with
https://github.com/david-marchand/dpdk/commit/e7860231ecdce91f9f70027d4090a7057b8fd5f7
?
> - Marked device validation function as experimental.
Useless, this symbol is internal and not exported.
Hello David,
Answers inline.
Regards
Sunil Kumar Kori
>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Friday, April 17, 2020 2:14 PM
>To: Sunil Kumar Kori <skori@marvell.com>
>Cc: Stephen Hemminger <stephen@networkplumber.org>; Jerin Jacob
>Kollanukkaran <jerinj@marvell.com>; dev <dev@dpdk.org>
>Subject: [EXT] Re: [PATCH v2 1/1] bus/pci: optimise scanning with
>whitelist/blacklist
>
>External Email
>
>----------------------------------------------------------------------
>On Tue, Apr 7, 2020 at 11:29 AM Sunil Kumar Kori <skori@marvell.com>
>wrote:
>>
>> rte_bus_scan API scans all the available PCI devices irrespective of
>> white or black listing parameters then further devices are probed
>> based on white or black listing parameters. So unnecessary CPU cycles
>> are wasted during rte_pci_scan.
>>
>> For Octeontx2 platform with core frequency 2.4 Ghz, rte_bus_scan
>> consumes around 26ms to scan around 90 PCI devices but all may not be
>> used by the application. So for the application which uses 2 NICs,
>> rte_bus_scan consumes few microseconds and rest time is saved with this
>patch.
>>
>> Patch restricts devices to be scanned as per below mentioned conditions:
>> - All devices will be scanned if no parameters are passed.
>> - Only white listed devices will be scanned if white list is available.
>> - All devices, except black listed, will be scanned if black list is
>> available.
>>
>> Signed-off-by: Sunil Kumar Kori <skori@marvell.com>
>> ---
>> v2:
>> - Added function to validate ignorance of device based on PCI address.
>
>First you objected to Stephen comment, and later announced there was no
>objection.
Please refer: https://patches.dpdk.org/patch/63924/. In first reply to Stephen's comment, Itself I said that I agree with that approach and why I have not taken that path is also mentioned,
and requested for suggestions. But there were no further inputs after asking multiple times then I thought no one has any concern and then I asked to get it merged for 20.05.
>
>Now, it seems you ignored what I replied without any explanation.
>So tell me, what was wrong with
>https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_david-
>2Dmarchand_dpdk_commit_e7860231ecdce91f9f70027d4090a7057b8fd5f7&
>d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=dXeXaAMkP5COgn1zxHMyaF1_d
>9IIuq6vHQO6NrIPjaE&m=3nE0hIIwz2cXBpYrewLujeRWz5WPE7LB9j_HvOtBd68
>&s=OjPCDnof_PNgATyzPIbjG8EtSYa5fE4EwbLD0oaIw5w&e=
No, Neither I have ignored your code changes nor denied. Both submitted patches uses similar approaches having one difference only that is you modified existing functions and I have written the new without touching the existing one. I have already explained in v1 that why I have not taken that path what you have implemented.
Also I thought, its not good to change pci_ignore_device and pci_devargs_lookup because in future if more parameters (part of rte_pci_device structure) are considered to ignore a device then again we have to change this function to support it.
It may be a rare case but it was one thought process.
>?
>
>
>> - Marked device validation function as experimental.
>
>Useless, this symbol is internal and not exported.
>
>
>--
>David Marchand
On Fri, Apr 17, 2020 at 1:16 PM Sunil Kumar Kori <skori@marvell.com> wrote:
> >Now, it seems you ignored what I replied without any explanation.
> >So tell me, what was wrong with
> >https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_david-
> >2Dmarchand_dpdk_commit_e7860231ecdce91f9f70027d4090a7057b8fd5f7&
> >d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=dXeXaAMkP5COgn1zxHMyaF1_d
> >9IIuq6vHQO6NrIPjaE&m=3nE0hIIwz2cXBpYrewLujeRWz5WPE7LB9j_HvOtBd68
> >&s=OjPCDnof_PNgATyzPIbjG8EtSYa5fE4EwbLD0oaIw5w&e=
> No, Neither I have ignored your code changes nor denied. Both submitted patches uses similar approaches having one difference only that is you modified existing functions and I have written the new without touching the existing one. I have already explained in v1 that why I have not taken that path what you have implemented.
> Also I thought, its not good to change pci_ignore_device and pci_devargs_lookup because in future if more parameters (part of rte_pci_device structure) are considered to ignore a device then again we have to change this function to support it.
> It may be a rare case but it was one thought process.
Your current patch is a no go anyway.
The __rte_experimental tagging makes no sense.
>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Friday, April 17, 2020 6:56 PM
>To: Sunil Kumar Kori <skori@marvell.com>
>Cc: Stephen Hemminger <stephen@networkplumber.org>; Jerin Jacob
>Kollanukkaran <jerinj@marvell.com>; dev <dev@dpdk.org>
>Subject: Re: [EXT] Re: [PATCH v2 1/1] bus/pci: optimise scanning with
>whitelist/blacklist
>
>On Fri, Apr 17, 2020 at 1:16 PM Sunil Kumar Kori <skori@marvell.com> wrote:
>> >Now, it seems you ignored what I replied without any explanation.
>> >So tell me, what was wrong with
>> >https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_david
>> >-
>2Dmarchand_dpdk_commit_e7860231ecdce91f9f70027d4090a7057b8fd5f7&
>>
>>d=DwIFaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=dXeXaAMkP5COgn1zxHMyaF1_
>d
>>
>>9IIuq6vHQO6NrIPjaE&m=3nE0hIIwz2cXBpYrewLujeRWz5WPE7LB9j_HvOtBd6
>8
>> >&s=OjPCDnof_PNgATyzPIbjG8EtSYa5fE4EwbLD0oaIw5w&e=
>> No, Neither I have ignored your code changes nor denied. Both submitted
>patches uses similar approaches having one difference only that is you
>modified existing functions and I have written the new without touching the
>existing one. I have already explained in v1 that why I have not taken that
>path what you have implemented.
>> Also I thought, its not good to change pci_ignore_device and
>pci_devargs_lookup because in future if more parameters (part of
>rte_pci_device structure) are considered to ignore a device then again we
>have to change this function to support it.
>> It may be a rare case but it was one thought process.
>
>Your current patch is a no go anyway.
>The __rte_experimental tagging makes no sense.
That’s not a problem. My only intention is to resolve a problem what we have observed and
corresponding fix caters a practical use case.
If you don’t like the way it is implemented then we can go with your suggested way.
Please let me know, will you be sending that github patch or should I send ?
>
>
>--
>David Marchand
On Fri, Apr 17, 2020 at 5:12 PM Sunil Kumar Kori <skori@marvell.com> wrote:
> >Your current patch is a no go anyway.
> >The __rte_experimental tagging makes no sense.
>
> That’s not a problem. My only intention is to resolve a problem what we have observed and
> corresponding fix caters a practical use case.
> If you don’t like the way it is implemented then we can go with your suggested way.
> Please let me know, will you be sending that github patch or should I send ?
And again.
The experimental tag is *useless*.
private.h is not exported to users, the symbol you want to add is
internal to the pci bus library.
I will stop at this.
Sent from Workspace ONE Boxer
On 17-Apr-2020 9:06 PM, David Marchand <david.marchand@redhat.com> wrote:
>
> On Fri, Apr 17, 2020 at 5:12 PM Sunil Kumar Kori <skori@marvell.com> wrote:
> > >Your current patch is a no go anyway.
> > >The __rte_experimental tagging makes no sense.
> >
> > That’s not a problem. My only intention is to resolve a problem what we have observed and
> > corresponding fix caters a practical use case.
> > If you don’t like the way it is implemented then we can go with your suggested way.
> > Please let me know, will you be sending that github patch or should I send ?
>
> And again.
> The experimental tag is *useless*.
> private.h is not exported to users, the symbol you want to add is
> internal to the pci bus library.
>
> I will stop at this.
>
Got it. If it's only about exporting the symbol to the user then I will send next version of my patch after removing __rte_experimental and will make symbol to internal.
>
> --
> David Marchand
>
>-----Original Message-----
>From: dev <dev-bounces@dpdk.org> On Behalf Of Sunil Kumar Kori
>Sent: Friday, April 17, 2020 9:30 PM
>To: David Marchand <david.marchand@redhat.com>
>Cc: Jerin Jacob Kollanukkaran <jerinj@marvell.com>; dev <dev@dpdk.org>;
>Stephen Hemminger <stephen@networkplumber.org>
>Subject: Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/1] bus/pci: optimise scanning
>with whitelist/blacklist
>
>Sent from Workspace ONE Boxer
>On 17-Apr-2020 9:06 PM, David Marchand <david.marchand@redhat.com>
>wrote:
>>
>> On Fri, Apr 17, 2020 at 5:12 PM Sunil Kumar Kori <skori@marvell.com>
>wrote:
>> > >Your current patch is a no go anyway.
>> > >The __rte_experimental tagging makes no sense.
>> >
>> > That’s not a problem. My only intention is to resolve a problem
>> > what we have observed and corresponding fix caters a practical use case.
>> > If you don’t like the way it is implemented then we can go with your
>suggested way.
>> > Please let me know, will you be sending that github patch or should I send
>?
>>
>> And again.
>> The experimental tag is *useless*.
>> private.h is not exported to users, the symbol you want to add is
>> internal to the pci bus library.
>>
>> I will stop at this.
>>
>Got it. If it's only about exporting the symbol to the user then I will send next
>version of my patch after removing __rte_experimental and will make symbol
>to internal.
Uploaded v3.
>>
>> --
>> David Marchand
>>
@@ -338,6 +338,9 @@ rte_pci_scan(void)
.match_buf_len = sizeof(matches),
.matches = &matches[0],
};
+ struct rte_pci_addr pci_addr;
+
+ memset(&pci_addr, 0, sizeof(struct rte_pci_addr));
/* for debug purposes, PCI can be disabled */
if (!rte_eal_has_pci())
@@ -357,9 +360,19 @@ rte_pci_scan(void)
goto error;
}
- for (i = 0; i < conf_io.num_matches; i++)
+ for (i = 0; i < conf_io.num_matches; i++) {
+ pci_addr.domain = matches[i].pc_sel.pc_domain;
+ pci_addr.bus = matches[i].pc_sel.pc_bus;
+ pci_addr.devid = matches[i].pc_sel.pc_dev;
+ pci_addr.function = matches[i].pc_sel.pc_func;
+
+ /* Check that device should be ignored or not */
+ if (pci_addr_ignore_device(&pci_addr))
+ continue;
+
if (pci_scan_one(fd, &matches[i]) < 0)
goto error;
+ }
dev_count += conf_io.num_matches;
} while(conf_io.status == PCI_GETCONF_MORE_DEVS);
@@ -487,6 +487,9 @@ rte_pci_scan(void)
if (parse_pci_addr_format(e->d_name, sizeof(e->d_name), &addr) != 0)
continue;
+ if (pci_addr_ignore_device(&addr))
+ continue;
+
snprintf(dirname, sizeof(dirname), "%s/%s",
rte_pci_get_sysfs_path(), e->d_name);
@@ -589,6 +589,40 @@ pci_dma_unmap(struct rte_device *dev, void *addr, uint64_t iova, size_t len)
return -1;
}
+static struct rte_devargs *
+pci_addr_devargs_lookup(const struct rte_pci_addr *pci_addr)
+{
+ struct rte_devargs *devargs;
+ struct rte_pci_addr addr;
+
+ RTE_EAL_DEVARGS_FOREACH("pci", devargs) {
+ devargs->bus->parse(devargs->name, &addr);
+ if (!rte_pci_addr_cmp(pci_addr, &addr))
+ return devargs;
+ }
+ return NULL;
+}
+
+bool
+pci_addr_ignore_device(const struct rte_pci_addr *pci_addr)
+{
+ struct rte_devargs *devargs = pci_addr_devargs_lookup(pci_addr);
+
+ switch (rte_pci_bus.bus.conf.scan_mode) {
+ case RTE_BUS_SCAN_WHITELIST:
+ if (devargs && devargs->policy == RTE_DEV_WHITELISTED)
+ return false;
+ break;
+ case RTE_BUS_SCAN_UNDEFINED:
+ case RTE_BUS_SCAN_BLACKLIST:
+ if (devargs == NULL ||
+ devargs->policy != RTE_DEV_BLACKLISTED)
+ return false;
+ break;
+ }
+ return true;
+}
+
static bool
pci_ignore_device(const struct rte_pci_device *dev)
{
@@ -42,6 +42,18 @@ int rte_pci_scan(void);
void
pci_name_set(struct rte_pci_device *dev);
+/**
+ * Validate whether a device with given pci address should be ignored or not.
+ *
+ * @param pci_addr
+ * PCI address of device to be validated
+ * @return
+ * 1: if device is to be ignored,
+ * 0: if device is to be scanned,
+ */
+__rte_experimental
+bool pci_addr_ignore_device(const struct rte_pci_addr *pci_addr);
+
/**
* Add a PCI device to the PCI Bus (append to PCI Device list). This function
* also updates the bus references of the PCI Device (and the generic device
@@ -16,3 +16,8 @@ DPDK_20.0 {
local: *;
};
+
+EXPERIMENTAL {
+ # added in 20.05
+ pci_addr_ignore_device;
+};