bus/pci: restricted bus scanning to allowed devices
diff mbox series

Message ID 20191216075501.15669-1-skori@marvell.com
State Deferred
Delegated to: David Marchand
Headers show
Series
  • bus/pci: restricted bus scanning to allowed devices
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/travis-robot warning Travis build: failed
ci/checkpatch success coding style OK

Commit Message

Sunil Kumar Kori Dec. 16, 2019, 7:55 a.m. UTC
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>
---
 drivers/bus/pci/bsd/pci.c    | 18 +++++++++++++++++-
 drivers/bus/pci/linux/pci.c  | 11 +++++++++++
 drivers/bus/pci/pci_common.c |  4 ++--
 drivers/bus/pci/private.h    | 20 ++++++++++++++++++++
 4 files changed, 50 insertions(+), 3 deletions(-)

Comments

Stephen Hemminger Dec. 16, 2019, 4:13 p.m. UTC | #1
> 			/* Create dummy pci device to get devargs */
> +			dummy_dev.addr.domain = matches[i].pc_sel.pc_domain;
> +			dummy_dev.addr.bus = matches[i].pc_sel.pc_bus;
> +			dummy_dev.addr.devid = matches[i].pc_sel.pc_dev;
> +			dummy_dev.addr.function = matches[i].pc_sel.pc_func;
> +			dummy_dev.device.devargs =
> +						pci_devargs_lookup(&dummy_dev);
> +
> +			/* Check that device should be ignored or not  */
> +			if (pci_ignore_device(&dummy_dev))
> +				continue;

It seems that you are creating dummy_dev as an alternative to passing
just the PCI bus/device/function. Wouldn't be easier to just use BDF
instead. Dummy arguments on the stack can lead to more corner cases
in the future if device subsystem changes.

> +/**
> + * Get the devargs of a PCI device.
> + *
> + * @param pci_dev
> + *	PCI device to be validated
> + * @return
> + *	devargs on succes, NULL otherwise
> + */
> +struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *pci_dev);

Must be marked experimental (or internal).
The pci_device should be marked const.


> +
> +/**
> + * Validate whether a pci device should be ignored or not.
> + *
> + * @param pci_dev
> + *	PCI device to be validated
> + * @return
> + *	1 if device is to be ignored, 0 otherwise
> + */
> +bool pci_ignore_device(const struct rte_pci_device *pci_dev);

ditto
Sunil Kumar Kori Dec. 17, 2019, 11 a.m. UTC | #2
Regards
Sunil Kumar Kori

>-----Original Message-----
>From: Stephen Hemminger <stephen@networkplumber.org>
>Sent: Monday, December 16, 2019 9:43 PM
>To: Sunil Kumar Kori <skori@marvell.com>
>Cc: dev@dpdk.org
>Subject: [EXT] Re: [dpdk-dev] [PATCH] bus/pci: restricted bus scanning to
>allowed devices
>
>External Email
>
>----------------------------------------------------------------------
>> 			/* Create dummy pci device to get devargs */
>> +			dummy_dev.addr.domain =
>matches[i].pc_sel.pc_domain;
>> +			dummy_dev.addr.bus = matches[i].pc_sel.pc_bus;
>> +			dummy_dev.addr.devid = matches[i].pc_sel.pc_dev;
>> +			dummy_dev.addr.function =
>matches[i].pc_sel.pc_func;
>> +			dummy_dev.device.devargs =
>> +
>	pci_devargs_lookup(&dummy_dev);
>> +
>> +			/* Check that device should be ignored or not  */
>> +			if (pci_ignore_device(&dummy_dev))
>> +				continue;
>
>It seems that you are creating dummy_dev as an alternative to passing just the
>PCI bus/device/function. Wouldn't be easier to just use BDF instead. Dummy
>arguments on the stack can lead to more corner cases in the future if device
>subsystem changes.
Agreed and initially I have implemented using BDF only instead of using dummy device.
But using that approach, I was not able to use existing APIs to get devargs and ignore device.
I had to write almost same functions to solve the purpose. So just to avoid having replica of
same code, I followed this approach. Please suggest.
>
>> +/**
>> + * Get the devargs of a PCI device.
>> + *
>> + * @param pci_dev
>> + *	PCI device to be validated
>> + * @return
>> + *	devargs on succes, NULL otherwise
>> + */
>> +struct rte_devargs *pci_devargs_lookup(struct rte_pci_device
>> +*pci_dev);
>
>Must be marked experimental (or internal).
>The pci_device should be marked const.
Okay but If I go with BDF one then this change is not required anyway. 
>
>
>> +
>> +/**
>> + * Validate whether a pci device should be ignored or not.
>> + *
>> + * @param pci_dev
>> + *	PCI device to be validated
>> + * @return
>> + *	1 if device is to be ignored, 0 otherwise
>> + */
>> +bool pci_ignore_device(const struct rte_pci_device *pci_dev);
>
>ditto
ditto
Sunil Kumar Kori Jan. 21, 2020, 8:39 a.m. UTC | #3
Hello Stephen,
Any suggestions ? 

Regards
Sunil Kumar Kori

>-----Original Message-----
>From: dev <dev-bounces@dpdk.org> On Behalf Of Sunil Kumar Kori
>Sent: Tuesday, December 17, 2019 4:30 PM
>To: Stephen Hemminger <stephen@networkplumber.org>
>Cc: dev@dpdk.org
>Subject: Re: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus scanning to
>allowed devices
>
>
>
>Regards
>Sunil Kumar Kori
>
>>-----Original Message-----
>>From: Stephen Hemminger <stephen@networkplumber.org>
>>Sent: Monday, December 16, 2019 9:43 PM
>>To: Sunil Kumar Kori <skori@marvell.com>
>>Cc: dev@dpdk.org
>>Subject: [EXT] Re: [dpdk-dev] [PATCH] bus/pci: restricted bus scanning
>>to allowed devices
>>
>>External Email
>>
>>----------------------------------------------------------------------
>>> 			/* Create dummy pci device to get devargs */
>>> +			dummy_dev.addr.domain =
>>matches[i].pc_sel.pc_domain;
>>> +			dummy_dev.addr.bus = matches[i].pc_sel.pc_bus;
>>> +			dummy_dev.addr.devid = matches[i].pc_sel.pc_dev;
>>> +			dummy_dev.addr.function =
>>matches[i].pc_sel.pc_func;
>>> +			dummy_dev.device.devargs =
>>> +
>>	pci_devargs_lookup(&dummy_dev);
>>> +
>>> +			/* Check that device should be ignored or not  */
>>> +			if (pci_ignore_device(&dummy_dev))
>>> +				continue;
>>
>>It seems that you are creating dummy_dev as an alternative to passing
>>just the PCI bus/device/function. Wouldn't be easier to just use BDF
>>instead. Dummy arguments on the stack can lead to more corner cases in
>>the future if device subsystem changes.
>Agreed and initially I have implemented using BDF only instead of using
>dummy device.
>But using that approach, I was not able to use existing APIs to get devargs and
>ignore device.
>I had to write almost same functions to solve the purpose. So just to avoid
>having replica of same code, I followed this approach. Please suggest.
>>
>>> +/**
>>> + * Get the devargs of a PCI device.
>>> + *
>>> + * @param pci_dev
>>> + *	PCI device to be validated
>>> + * @return
>>> + *	devargs on succes, NULL otherwise
>>> + */
>>> +struct rte_devargs *pci_devargs_lookup(struct rte_pci_device
>>> +*pci_dev);
>>
>>Must be marked experimental (or internal).
>>The pci_device should be marked const.
>Okay but If I go with BDF one then this change is not required anyway.
>>
>>
>>> +
>>> +/**
>>> + * Validate whether a pci device should be ignored or not.
>>> + *
>>> + * @param pci_dev
>>> + *	PCI device to be validated
>>> + * @return
>>> + *	1 if device is to be ignored, 0 otherwise
>>> + */
>>> +bool pci_ignore_device(const struct rte_pci_device *pci_dev);
>>
>>ditto
>ditto

Patch
diff mbox series

diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
index ebbfeb13a..58fa7a241 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_device dummy_dev;
+
+	memset(&dummy_dev, 0, sizeof(struct rte_pci_device));
 
 	/* for debug purposes, PCI can be disabled */
 	if (!rte_eal_has_pci())
@@ -357,9 +360,22 @@  rte_pci_scan(void)
 			goto error;
 		}
 
-		for (i = 0; i < conf_io.num_matches; i++)
+		for (i = 0; i < conf_io.num_matches; i++) {
+			/* Create dummy pci device to get devargs */
+			dummy_dev.addr.domain = matches[i].pc_sel.pc_domain;
+			dummy_dev.addr.bus = matches[i].pc_sel.pc_bus;
+			dummy_dev.addr.devid = matches[i].pc_sel.pc_dev;
+			dummy_dev.addr.function = matches[i].pc_sel.pc_func;
+			dummy_dev.device.devargs =
+						pci_devargs_lookup(&dummy_dev);
+
+			/* Check that device should be ignored or not  */
+			if (pci_ignore_device(&dummy_dev))
+				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 740a2cdad..f6335810b 100644
--- a/drivers/bus/pci/linux/pci.c
+++ b/drivers/bus/pci/linux/pci.c
@@ -458,6 +458,9 @@  rte_pci_scan(void)
 	DIR *dir;
 	char dirname[PATH_MAX];
 	struct rte_pci_addr addr;
+	struct rte_pci_device dummy_dev;
+
+	memset(&dummy_dev, 0, sizeof(struct rte_pci_device));
 
 	/* for debug purposes, PCI can be disabled */
 	if (!rte_eal_has_pci())
@@ -482,6 +485,14 @@  rte_pci_scan(void)
 		if (parse_pci_addr_format(e->d_name, sizeof(e->d_name), &addr) != 0)
 			continue;
 
+		/* Create dummy pci device to get devargs */
+		dummy_dev.addr = addr;
+		dummy_dev.device.devargs = pci_devargs_lookup(&dummy_dev);
+
+		/* Check that device should be ignored or not  */
+		if (pci_ignore_device(&dummy_dev))
+			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..ec063e832 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -42,7 +42,7 @@  const char *rte_pci_get_sysfs_path(void)
 	return path;
 }
 
-static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
+struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
 {
 	struct rte_devargs *devargs;
 	struct rte_pci_addr addr;
@@ -589,7 +589,7 @@  pci_dma_unmap(struct rte_device *dev, void *addr, uint64_t iova, size_t len)
 	return -1;
 }
 
-static bool
+bool
 pci_ignore_device(const struct rte_pci_device *dev)
 {
 	struct rte_devargs *devargs = dev->device.devargs;
diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
index a205d4d9f..fc47768c6 100644
--- a/drivers/bus/pci/private.h
+++ b/drivers/bus/pci/private.h
@@ -42,6 +42,26 @@  int rte_pci_scan(void);
 void
 pci_name_set(struct rte_pci_device *dev);
 
+/**
+ * Get the devargs of a PCI device.
+ *
+ * @param pci_dev
+ *	PCI device to be validated
+ * @return
+ *	devargs on succes, NULL otherwise
+ */
+struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *pci_dev);
+
+/**
+ * Validate whether a pci device should be ignored or not.
+ *
+ * @param pci_dev
+ *	PCI device to be validated
+ * @return
+ *	1 if device is to be ignored, 0 otherwise
+ */
+bool pci_ignore_device(const struct rte_pci_device *pci_dev);
+
 /**
  * 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