[dpdk-dev] [PATCH] bus/pci: restricted bus scanning to allowed devices

[dpdk-dev] [PATCH] bus/pci: restricted bus scanning to allowed devices

[Sunil Kumar Kori]

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

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

Re: [dpdk-dev] [PATCH] bus/pci: restricted bus scanning to allowed devices

[Stephen Hemminger]

> 			/* 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

Re: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus scanning to allowed devices

[Sunil Kumar Kori]

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

Re: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus scanning to allowed devices

[Sunil Kumar Kori]

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

Re: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus scanning to allowed devices

[Sunil Kumar Kori]

Hello All, 

Is there any thought on this ? Otherwise it can be merged. 

Regards
Sunil Kumar Kori

>-----Original Message-----
>From: Sunil Kumar Kori
>Sent: Thursday, January 23, 2020 2:13 PM
>To: Stephen Hemminger <stephen@networkplumber.org>; Jerin Jacob
>Kollanukkaran <jerinj@marvell.com>
>Subject: FW: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus scanning to
>allowed devices
>
>Hello Stephen,
>
>Can you please look into this patch or provide your thought in this ? So that it
>can be merged within 20.02 release.
>
>Regards
>Sunil Kumar Kori
>
>-----Original Message-----
>From: Sunil Kumar Kori <skori@marvell.com>
>Sent: Tuesday, January 21, 2020 2:09 PM
>To: Sunil Kumar Kori <skori@marvell.com>; Stephen Hemminger
><stephen@networkplumber.org>
>Cc: dev@dpdk.org
>Subject: RE: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus scanning to
>allowed devices
>
>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

Re: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus scanning to allowed devices

[Sunil Kumar Kori]

Hello Stephen,

Please provide ack on below change if there is no concern so that it can be accepted on 20.05.

Regards
Sunil Kumar Kori

>-----Original Message-----
>From: Sunil Kumar Kori
>Sent: Thursday, February 27, 2020 2:00 PM
>To: Stephen Hemminger <stephen@networkplumber.org>; Jerin Jacob
>Kollanukkaran <jerinj@marvell.com>; dev@dpdk.org
>Subject: RE: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus scanning to
>allowed devices
>
>Hello All,
>
>Is there any thought on this ? Otherwise it can be merged.
>
>Regards
>Sunil Kumar Kori
>
>>-----Original Message-----
>>From: Sunil Kumar Kori
>>Sent: Thursday, January 23, 2020 2:13 PM
>>To: Stephen Hemminger <stephen@networkplumber.org>; Jerin Jacob
>>Kollanukkaran <jerinj@marvell.com>
>>Subject: FW: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus
>>scanning to allowed devices
>>
>>Hello Stephen,
>>
>>Can you please look into this patch or provide your thought in this ?
>>So that it can be merged within 20.02 release.
>>
>>Regards
>>Sunil Kumar Kori
>>
>>-----Original Message-----
>>From: Sunil Kumar Kori <skori@marvell.com>
>>Sent: Tuesday, January 21, 2020 2:09 PM
>>To: Sunil Kumar Kori <skori@marvell.com>; Stephen Hemminger
>><stephen@networkplumber.org>
>>Cc: dev@dpdk.org
>>Subject: RE: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus
>>scanning to allowed devices
>>
>>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

Re: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus scanning to allowed devices

[Sunil Kumar Kori]

Hello, 

It looks  like there is no comment/objection on following patch and it can be merged. 
I would request to @David Marchand, please take care of this towards the merging process for 20.05.


Regards
Sunil Kumar Kori

>-----Original Message-----
>From: Sunil Kumar Kori
>Sent: Monday, March 9, 2020 11:37 AM
>To: 'Stephen Hemminger' <stephen@networkplumber.org>; Jerin Jacob
>Kollanukkaran <jerinj@marvell.com>; 'dev@dpdk.org' <dev@dpdk.org>
>Subject: RE: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus scanning to
>allowed devices
>
>Hello Stephen,
>
>Please provide ack on below change if there is no concern so that it can be
>accepted on 20.05.
>
>Regards
>Sunil Kumar Kori
>
>>-----Original Message-----
>>From: Sunil Kumar Kori
>>Sent: Thursday, February 27, 2020 2:00 PM
>>To: Stephen Hemminger <stephen@networkplumber.org>; Jerin Jacob
>>Kollanukkaran <jerinj@marvell.com>; dev@dpdk.org
>>Subject: RE: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus
>>scanning to allowed devices
>>
>>Hello All,
>>
>>Is there any thought on this ? Otherwise it can be merged.
>>
>>Regards
>>Sunil Kumar Kori
>>
>>>-----Original Message-----
>>>From: Sunil Kumar Kori
>>>Sent: Thursday, January 23, 2020 2:13 PM
>>>To: Stephen Hemminger <stephen@networkplumber.org>; Jerin Jacob
>>>Kollanukkaran <jerinj@marvell.com>
>>>Subject: FW: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus
>>>scanning to allowed devices
>>>
>>>Hello Stephen,
>>>
>>>Can you please look into this patch or provide your thought in this ?
>>>So that it can be merged within 20.02 release.
>>>
>>>Regards
>>>Sunil Kumar Kori
>>>
>>>-----Original Message-----
>>>From: Sunil Kumar Kori <skori@marvell.com>
>>>Sent: Tuesday, January 21, 2020 2:09 PM
>>>To: Sunil Kumar Kori <skori@marvell.com>; Stephen Hemminger
>>><stephen@networkplumber.org>
>>>Cc: dev@dpdk.org
>>>Subject: RE: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus
>>>scanning to allowed devices
>>>
>>>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

Re: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus scanning to allowed devices

[David Marchand]

On Mon, Apr 6, 2020 at 11:32 AM Sunil Kumar Kori <skori@marvell.com> wrote:
> It looks  like there is no comment/objection on following patch and it can be merged.

The title does not reflect what this patch is about.
I understand this as an optimisation for the pci bus scanning.


I agree with Stephen comment.

If you change pci_ignore_device and pci_devargs_lookup to take a
rte_pci_addr as input, then pci_ignore_device can call
pci_devargs_lookup itself.
And pci_devargs_lookup does not need to be exported in the private header.

Untested (just tried compilation),
https://github.com/david-marchand/dpdk/commit/e7860231ecdce91f9f70027d4090a7057b8fd5f7


-- 
David Marchand

Re: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus scanning to allowed devices

[Sunil Kumar Kori]

Regards
Sunil Kumar Kori

>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Monday, April 6, 2020 6:52 PM
>To: Sunil Kumar Kori <skori@marvell.com>
>Cc: Stephen Hemminger <stephen@networkplumber.org>; Jerin Jacob
>Kollanukkaran <jerinj@marvell.com>; dev@dpdk.org
>Subject: Re: [dpdk-dev] [EXT] Re: [PATCH] bus/pci: restricted bus scanning to
>allowed devices
>
>On Mon, Apr 6, 2020 at 11:32 AM Sunil Kumar Kori <skori@marvell.com>
>wrote:
>> It looks  like there is no comment/objection on following patch and it can be
>merged.
>
>The title does not reflect what this patch is about.
>I understand this as an optimisation for the pci bus scanning.
>
>
>I agree with Stephen comment.
>
>If you change pci_ignore_device and pci_devargs_lookup to take a
>rte_pci_addr as input, then pci_ignore_device can call pci_devargs_lookup
>itself.
>And pci_devargs_lookup does not need to be exported in the private header.
>
>Untested (just tried compilation),
>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=H7IQgd6gnpQBcbVgN952C_oIcMj5Z34kRhYngZr8Pyc
>&s=eq-hCdZ-C4xZ0Y7rQEkYwJS04r5sh7G2I9CXHTuBmws&e=
>
>
>--
>David Marchand

Okay. I will share updated version of this based on review comments.

[dpdk-dev] [PATCH v2 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Sunil Kumar Kori]

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

Re: [dpdk-dev] [PATCH v2 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Sunil Kumar Kori]

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

Re: [dpdk-dev] [PATCH v2 1/1] bus/pci: optimise scanning with whitelist/blacklist

[David Marchand]

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.


-- 
David Marchand

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Sunil Kumar Kori]

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

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/1] bus/pci: optimise scanning with whitelist/blacklist

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


-- 
David Marchand

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Sunil Kumar Kori]

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

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/1] bus/pci: optimise scanning with whitelist/blacklist

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


-- 
David Marchand

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Sunil Kumar Kori]

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
>

[dpdk-dev] [PATCH v3 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Sunil Kumar Kori]

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>
---
v3:
 - remove __rte_experimental from private function.
 - remove entry from map file too.
v2:
 - Added function to validate ignorance of device based on PCI address.
 - Marked device validation function as experimental.

 drivers/bus/pci/bsd/pci.c    | 13 ++++++++++++-
 drivers/bus/pci/linux/pci.c  |  3 +++
 drivers/bus/pci/pci_common.c | 34 ++++++++++++++++++++++++++++++++++
 drivers/bus/pci/private.h    | 11 +++++++++++
 4 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
index ebbfeb13a..c8d954751 100644
--- a/drivers/bus/pci/bsd/pci.c
+++ b/drivers/bus/pci/bsd/pci.c
@@ -338,6 +338,7 @@ rte_pci_scan(void)
 			.match_buf_len = sizeof(matches),
 			.matches = &matches[0],
 	};
+	struct rte_pci_addr pci_addr;
 
 	/* for debug purposes, PCI can be disabled */
 	if (!rte_eal_has_pci())
@@ -357,9 +358,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..75af786f7 100644
--- a/drivers/bus/pci/private.h
+++ b/drivers/bus/pci/private.h
@@ -42,6 +42,17 @@ 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,
+ */
+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
-- 
2.17.1

Re: [dpdk-dev] [EXT] Re: [PATCH v2 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Sunil Kumar Kori]

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

Re: [dpdk-dev] [PATCH v3 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Gaëtan Rivet]

On 20/04/20 12:25 +0530, Sunil Kumar Kori 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.
> 

Hi Sunil,

The PCI bus was written at first with the understanding that all PCI
devices were scanned and made available on the bus -- the probe will filter
afterward.

Device hotplug and iteration were written with this in mind. Changing
this principle might have unintended consequences in other EAL parts.
I'm not fundamentally against it, but it is not how buses are currently
designed in DPDK.

To me, a one-time 26ms gain is not enough justification to change this
principle. How problematic is this for you? Do you encounter specific
issues due to this delay?

Thanks,
-- 
Gaëtan

Re: [dpdk-dev] [EXT] Re: [PATCH v3 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Sunil Kumar Kori]

>-----Original Message-----
>From: Gaëtan Rivet <grive@u256.net>
>Sent: Tuesday, April 21, 2020 8:48 PM
>To: Sunil Kumar Kori <skori@marvell.com>
>Cc: stephen@networkplumber.org; david.marchand@redhat.com; Jerin Jacob
>Kollanukkaran <jerinj@marvell.com>; dev@dpdk.org
>Subject: [EXT] Re: [dpdk-dev] [PATCH v3 1/1] bus/pci: optimise scanning with
>whitelist/blacklist
>
>External Email
>
>----------------------------------------------------------------------
>On 20/04/20 12:25 +0530, Sunil Kumar Kori 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.
>>
>
>Hi Sunil,
>
>The PCI bus was written at first with the understanding that all PCI devices
>were scanned and made available on the bus -- the probe will filter afterward.
>
>Device hotplug and iteration were written with this in mind. Changing this
>principle might have unintended consequences in other EAL parts.
>I'm not fundamentally against it, but it is not how buses are currently
>designed in DPDK.
>
I am also not sure about this. I would request you provide suggestion to ensure that there won't be
any negative consequences if any.  So that I can handle those too.

>To me, a one-time 26ms gain is not enough justification to change this
>principle. How problematic is this for you? Do you encounter specific issues
>due to this delay?
>
>Thanks,

Recently we observed this requirement to cater a use of having lowest bootup time for DPDK application.
One of the use-case for this to reduce the downtime as part of DPDK SW upgrade in the field. i.e
after the SW update, time to close the application and restart it again for packet processing.
Having this solution application will be up soon and lesser traffic impact will be there in a deployed system.

>--
>Gaëtan

Re: [dpdk-dev] [EXT] Re: [PATCH v3 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Gaëtan Rivet]

On 22/04/20 06:17 +0000, Sunil Kumar Kori wrote:
> >-----Original Message-----
> >From: Gaëtan Rivet <grive@u256.net>
> >Sent: Tuesday, April 21, 2020 8:48 PM
> >To: Sunil Kumar Kori <skori@marvell.com>
> >Cc: stephen@networkplumber.org; david.marchand@redhat.com; Jerin Jacob
> >Kollanukkaran <jerinj@marvell.com>; dev@dpdk.org
> >Subject: [EXT] Re: [dpdk-dev] [PATCH v3 1/1] bus/pci: optimise scanning with
> >whitelist/blacklist
> >
> >External Email
> >
> >----------------------------------------------------------------------
> >On 20/04/20 12:25 +0530, Sunil Kumar Kori 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.
> >>
> >
> >Hi Sunil,
> >
> >The PCI bus was written at first with the understanding that all PCI devices
> >were scanned and made available on the bus -- the probe will filter afterward.
> >
> >Device hotplug and iteration were written with this in mind. Changing this
> >principle might have unintended consequences in other EAL parts.
> >I'm not fundamentally against it, but it is not how buses are currently
> >designed in DPDK.
> >
> I am also not sure about this. I would request you provide suggestion to ensure that there won't be
> any negative consequences if any.  So that I can handle those too.
> 

I would like also to hear from other stakeholders for the PCI bus.

Generally, as long as the blacklist mode is the default, behavior should
not change, but devil is in the details.

I would have some comments on the patch itself if everyone agrees with
this direction.

If the principle of the patch is accepted, it would be great for you to
test hotplug and device listing with testpmd:

   hotplug:
    * You can spawn VMs with virtual e1000 ports on PCI using QEMU for this,
      and within testpmd `port attach <pci-id>` -- of course, the
      port(s) should not be attached when starting testpmd. You might
      have to either blacklist them, or you could hotplug them in QEMU using the
      monitor. I don't recall the QEMU commands to do that, sorry.

   device list:
    * `show device info all` in testpmd. I thought I had added a command to
      test the device iterator, taking an arbitrary device string, but
      the patch has been dropped it seems.

If you have no segfault and no surprise, it is a good start.

> >To me, a one-time 26ms gain is not enough justification to change this
> >principle. How problematic is this for you? Do you encounter specific issues
> >due to this delay?
> >
> >Thanks,
> 
> Recently we observed this requirement to cater a use of having lowest bootup time for DPDK application.
> One of the use-case for this to reduce the downtime as part of DPDK SW upgrade in the field. i.e
> after the SW update, time to close the application and restart it again for packet processing.
> Having this solution application will be up soon and lesser traffic impact will be there in a deployed system.

DPDK startup was not written with low latency in mind. You will find here
and there minute improvements, but I think it is a pipedream to reduce
service disruption on binary upgrade.

People in the field would be better served with HA, not relying on a
critical apps restarting as fast as possible.

Cheers,
-- 
Gaëtan

Re: [dpdk-dev] [EXT] Re: [PATCH v3 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Sunil Kumar Kori]

>-----Original Message-----
>From: Gaëtan Rivet <grive@u256.net>
>Sent: Wednesday, April 22, 2020 3:09 PM
>To: Sunil Kumar Kori <skori@marvell.com>
>Cc: stephen@networkplumber.org; david.marchand@redhat.com; Jerin Jacob
>Kollanukkaran <jerinj@marvell.com>; dev@dpdk.org
>Subject: Re: [EXT] Re: [dpdk-dev] [PATCH v3 1/1] bus/pci: optimise scanning
>with whitelist/blacklist
>
>On 22/04/20 06:17 +0000, Sunil Kumar Kori wrote:
>> >-----Original Message-----
>> >From: Gaëtan Rivet <grive@u256.net>
>> >Sent: Tuesday, April 21, 2020 8:48 PM
>> >To: Sunil Kumar Kori <skori@marvell.com>
>> >Cc: stephen@networkplumber.org; david.marchand@redhat.com; Jerin
>> >Jacob Kollanukkaran <jerinj@marvell.com>; dev@dpdk.org
>> >Subject: [EXT] Re: [dpdk-dev] [PATCH v3 1/1] bus/pci: optimise
>> >scanning with whitelist/blacklist
>> >
>> >External Email
>> >
>> >---------------------------------------------------------------------
>> >- On 20/04/20 12:25 +0530, Sunil Kumar Kori 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.
>> >>
>> >
>> >Hi Sunil,
>> >
>> >The PCI bus was written at first with the understanding that all PCI
>> >devices were scanned and made available on the bus -- the probe will filter
>afterward.
>> >
>> >Device hotplug and iteration were written with this in mind. Changing
>> >this principle might have unintended consequences in other EAL parts.
>> >I'm not fundamentally against it, but it is not how buses are
>> >currently designed in DPDK.
>> >
>> I am also not sure about this. I would request you provide suggestion
>> to ensure that there won't be any negative consequences if any.  So that I
>can handle those too.
>>
>
>I would like also to hear from other stakeholders for the PCI bus.
>
>Generally, as long as the blacklist mode is the default, behavior should not
>change, but devil is in the details.
>
>I would have some comments on the patch itself if everyone agrees with this
>direction.
>
>If the principle of the patch is accepted, it would be great for you to test
>hotplug and device listing with testpmd:
>
>   hotplug:
>    * You can spawn VMs with virtual e1000 ports on PCI using QEMU for this,
>      and within testpmd `port attach <pci-id>` -- of course, the
>      port(s) should not be attached when starting testpmd. You might
>      have to either blacklist them, or you could hotplug them in QEMU using the
>      monitor. I don't recall the QEMU commands to do that, sorry.
>
>   device list:
>    * `show device info all` in testpmd. I thought I had added a command to
>      test the device iterator, taking an arbitrary device string, but
>      the patch has been dropped it seems.
>
>If you have no segfault and no surprise, it is a good start.
>
Okay but before verification I would appreciate to have more comments and
closure on principle from other PCI stakeholders. If there is no objection on
principle then I will invest energy in testing.
I will wait for inputs by next week and if there are no inputs then
 assuming it fundamentally correct, I will start verification of above test cases. 

Also If anyone has already validated above mentioned test cases then
Suggestions, about the impact of this patch on PCI bus design, will be very
helpful to understand the real issues with this. 

>> >To me, a one-time 26ms gain is not enough justification to change
>> >this principle. How problematic is this for you? Do you encounter
>> >specific issues due to this delay?
>> >
>> >Thanks,
>>
>> Recently we observed this requirement to cater a use of having lowest
>bootup time for DPDK application.
>> One of the use-case for this to reduce the downtime as part of DPDK SW
>> upgrade in the field. i.e after the SW update, time to close the application
>and restart it again for packet processing.
>> Having this solution application will be up soon and lesser traffic impact will
>be there in a deployed system.
>
>DPDK startup was not written with low latency in mind. You will find here and
>there minute improvements, but I think it is a pipedream to reduce service
>disruption on binary upgrade.
>
>People in the field would be better served with HA, not relying on a critical
>apps restarting as fast as possible.
>
Recently we had a requirement to have bootup time <= 500ms and find
it as one of the candidate to be improved. So thought of to upstream it. 
Also having mechanism to improve bootup time is  good. I think, there is
no harm in this.

>Cheers,
>--
>Gaëtan

Re: [dpdk-dev] [PATCH v3 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Gaëtan Rivet]

Hello Sunil,

As it seems that this patch does not overly alarm people using the PCI
bus, I have a few comments on this version. Sending those a little
earlier will allow you to change as needed before proceeding with your
tests.

On 20/04/20 12:25 +0530, Sunil Kumar Kori 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>
> ---
> v3:
>  - remove __rte_experimental from private function.
>  - remove entry from map file too.
> v2:
>  - Added function to validate ignorance of device based on PCI address.
>  - Marked device validation function as experimental.
> 
>  drivers/bus/pci/bsd/pci.c    | 13 ++++++++++++-
>  drivers/bus/pci/linux/pci.c  |  3 +++
>  drivers/bus/pci/pci_common.c | 34 ++++++++++++++++++++++++++++++++++
>  drivers/bus/pci/private.h    | 11 +++++++++++
>  4 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
> index ebbfeb13a..c8d954751 100644
> --- a/drivers/bus/pci/bsd/pci.c
> +++ b/drivers/bus/pci/bsd/pci.c
> @@ -338,6 +338,7 @@ rte_pci_scan(void)
>  			.match_buf_len = sizeof(matches),
>  			.matches = &matches[0],
>  	};
> +	struct rte_pci_addr pci_addr;
>  
>  	/* for debug purposes, PCI can be disabled */
>  	if (!rte_eal_has_pci())
> @@ -357,9 +358,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  */

This comment is unnecessary, the function name should be sufficient to
describe the check done.

> +			if (pci_addr_ignore_device(&pci_addr))
> +				continue;
> +

As this function is almost a copy of pci_ignore_device(), with a twist
on the addr, I think the name pci_ignore_device_addr() would be more
helpful.

I think in general however, that exposed symbols, even internals,
should be prefixed with rte_. It was (almost) ok for
pci_ignore_device() to forego the namespace as it is a static function
that will be mangled on export, but that won't be the case for your
function.

Please add rte_ prefix.

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

Why not use rte_pci_addr_parse directly there? The bus->parse() API,
while stable, is one-level of indirection removed from what's done,
it's simpler for the reader to see the intent by using the proper function.

Return value should be checked. If the devargs name is not parseable,
there are other issues at hand (memory corruption), we should skip the
device as well or crash, but not proceed with comparison.

> +		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)
>  {

The logic seems ok to me.

However, the logic is the same as the one in rte_pci_probe(). During
probe, any device on the bus would have already been vetted during scan.
It should be ok to probe all existing rte_pci_device.

The same argument can be made for rte_pci_get_iommu_class() then, no
need to use pci_ignore_device(). It is done after the scan() so it
should be ok.

And if pci_ignore_device() can be completely removed, then you should
rename your function from rte_pci_ignore_device_addr() to
rte_pci_ignore_device() altogether.

> diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
> index a205d4d9f..75af786f7 100644
> --- a/drivers/bus/pci/private.h
> +++ b/drivers/bus/pci/private.h
> @@ -42,6 +42,17 @@ 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,
> + */
> +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
> -- 
> 2.17.1

Best regards,
-- 
Gaëtan

Re: [dpdk-dev] [EXT] Re: [PATCH v3 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Sunil Kumar Kori]

>-----Original Message-----
>From: Gaëtan Rivet <grive@u256.net>
>Sent: Tuesday, April 28, 2020 12:14 AM
>To: Sunil Kumar Kori <skori@marvell.com>
>Cc: stephen@networkplumber.org; david.marchand@redhat.com; Jerin Jacob
>Kollanukkaran <jerinj@marvell.com>; dev@dpdk.org
>Subject: [EXT] Re: [dpdk-dev] [PATCH v3 1/1] bus/pci: optimise scanning with
>whitelist/blacklist
>
>External Email
>
>----------------------------------------------------------------------
>Hello Sunil,
>
>As it seems that this patch does not overly alarm people using the PCI
>bus, I have a few comments on this version. Sending those a little
>earlier will allow you to change as needed before proceeding with your
>tests.
>
>On 20/04/20 12:25 +0530, Sunil Kumar Kori 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>
>> ---
>> v3:
>>  - remove __rte_experimental from private function.
>>  - remove entry from map file too.
>> v2:
>>  - Added function to validate ignorance of device based on PCI address.
>>  - Marked device validation function as experimental.
>>
>>  drivers/bus/pci/bsd/pci.c    | 13 ++++++++++++-
>>  drivers/bus/pci/linux/pci.c  |  3 +++
>>  drivers/bus/pci/pci_common.c | 34
>++++++++++++++++++++++++++++++++++
>>  drivers/bus/pci/private.h    | 11 +++++++++++
>>  4 files changed, 60 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
>> index ebbfeb13a..c8d954751 100644
>> --- a/drivers/bus/pci/bsd/pci.c
>> +++ b/drivers/bus/pci/bsd/pci.c
>> @@ -338,6 +338,7 @@ rte_pci_scan(void)
>>  			.match_buf_len = sizeof(matches),
>>  			.matches = &matches[0],
>>  	};
>> +	struct rte_pci_addr pci_addr;
>>
>>  	/* for debug purposes, PCI can be disabled */
>>  	if (!rte_eal_has_pci())
>> @@ -357,9 +358,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  */
>
>This comment is unnecessary, the function name should be sufficient to
>describe the check done.
>
Ack

>> +			if (pci_addr_ignore_device(&pci_addr))
>> +				continue;
>> +
>
>As this function is almost a copy of pci_ignore_device(), with a twist
>on the addr, I think the name pci_ignore_device_addr() would be more
>helpful.
>
>I think in general however, that exposed symbols, even internals,
>should be prefixed with rte_. It was (almost) ok for
>pci_ignore_device() to forego the namespace as it is a static function
>that will be mangled on export, but that won't be the case for your
>function.
>
>Please add rte_ prefix.
>
Ack

>>  			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);
>
>Why not use rte_pci_addr_parse directly there? The bus->parse() API,
>while stable, is one-level of indirection removed from what's done,
>it's simpler for the reader to see the intent by using the proper function.
>
>Return value should be checked. If the devargs name is not parseable,
>there are other issues at hand (memory corruption), we should skip the
>device as well or crash, but not proceed with comparison.
>
Ack

>> +		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)
>>  {
>
>The logic seems ok to me.
>
>However, the logic is the same as the one in rte_pci_probe(). During
>probe, any device on the bus would have already been vetted during scan.
>It should be ok to probe all existing rte_pci_device.
>
>The same argument can be made for rte_pci_get_iommu_class() then, no
>need to use pci_ignore_device(). It is done after the scan() so it
>should be ok.
>
>And if pci_ignore_device() can be completely removed, then you should
>rename your function from rte_pci_ignore_device_addr() to
>rte_pci_ignore_device() altogether.
>
Ack

>> diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
>> index a205d4d9f..75af786f7 100644
>> --- a/drivers/bus/pci/private.h
>> +++ b/drivers/bus/pci/private.h
>> @@ -42,6 +42,17 @@ 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,
>> + */
>> +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
>> --
>> 2.17.1
>
>Best regards,
>--
>Gaëtan

[dpdk-dev] [PATCH v4 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Sunil Kumar Kori]

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>
---
v4:
 - Review comments incorporated (Gaeten and David).
 - Rebased on top of tree.
v3:
 - Remove __rte_experimental from private function.
 - Remove entry from map file too.
v2:
 - Added function to validate ignorance of device based on PCI address.
 - Marked device validation function as experimental.

 drivers/bus/pci/bsd/pci.c    | 12 +++++++++++-
 drivers/bus/pci/linux/pci.c  |  3 +++
 drivers/bus/pci/pci_common.c | 33 ++++++++++++---------------------
 drivers/bus/pci/private.h    | 11 +++++++++++
 4 files changed, 37 insertions(+), 22 deletions(-)

diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
index ebbfeb13a..709a1e7e5 100644
--- a/drivers/bus/pci/bsd/pci.c
+++ b/drivers/bus/pci/bsd/pci.c
@@ -338,6 +338,7 @@ rte_pci_scan(void)
 			.match_buf_len = sizeof(matches),
 			.matches = &matches[0],
 	};
+	struct rte_pci_addr pci_addr;
 
 	/* for debug purposes, PCI can be disabled */
 	if (!rte_eal_has_pci())
@@ -357,9 +358,18 @@ 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;
+
+			if (rte_pci_ignore_device_addr(&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 ca783b157..ec347eff3 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 (rte_pci_ignore_device_addr(&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..d34e59536 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -42,14 +42,17 @@ const char *rte_pci_get_sysfs_path(void)
 	return path;
 }
 
-static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
+static struct rte_devargs *
+pci_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(&dev->addr, &addr))
+		if (rte_pci_addr_parse(devargs->name, &addr) < 0)
+			continue;
+
+		if (!rte_pci_addr_cmp(pci_addr, &addr))
 			return devargs;
 	}
 	return NULL;
@@ -63,7 +66,7 @@ pci_name_set(struct rte_pci_device *dev)
 	/* Each device has its internal, canonical name set. */
 	rte_pci_device_name(&dev->addr,
 			dev->name, sizeof(dev->name));
-	devargs = pci_devargs_lookup(dev);
+	devargs = pci_devargs_lookup(&dev->addr);
 	dev->device.devargs = devargs;
 	/* In blacklist mode, if the device is not blacklisted, no
 	 * rte_devargs exists for it.
@@ -293,23 +296,12 @@ rte_pci_probe(void)
 {
 	struct rte_pci_device *dev = NULL;
 	size_t probed = 0, failed = 0;
-	struct rte_devargs *devargs;
-	int probe_all = 0;
 	int ret = 0;
 
-	if (rte_pci_bus.bus.conf.scan_mode != RTE_BUS_SCAN_WHITELIST)
-		probe_all = 1;
-
 	FOREACH_DEVICE_ON_PCIBUS(dev) {
 		probed++;
 
-		devargs = dev->device.devargs;
-		/* probe all or only whitelisted devices */
-		if (probe_all)
-			ret = pci_probe_all_drivers(dev);
-		else if (devargs != NULL &&
-			devargs->policy == RTE_DEV_WHITELISTED)
-			ret = pci_probe_all_drivers(dev);
+		ret = pci_probe_all_drivers(dev);
 		if (ret < 0) {
 			if (ret != -EEXIST) {
 				RTE_LOG(ERR, EAL, "Requested device "
@@ -589,10 +581,10 @@ pci_dma_unmap(struct rte_device *dev, void *addr, uint64_t iova, size_t len)
 	return -1;
 }
 
-static bool
-pci_ignore_device(const struct rte_pci_device *dev)
+bool
+rte_pci_ignore_device_addr(const struct rte_pci_addr *pci_addr)
 {
-	struct rte_devargs *devargs = dev->device.devargs;
+	struct rte_devargs *devargs = pci_devargs_lookup(pci_addr);
 
 	switch (rte_pci_bus.bus.conf.scan_mode) {
 	case RTE_BUS_SCAN_WHITELIST:
@@ -627,8 +619,7 @@ rte_pci_get_iommu_class(void)
 		if (iommu_no_va == -1)
 			iommu_no_va = pci_device_iommu_support_va(dev)
 					? 0 : 1;
-		if (pci_ignore_device(dev))
-			continue;
+
 		if (dev->kdrv == RTE_KDRV_UNKNOWN ||
 		    dev->kdrv == RTE_KDRV_NONE)
 			continue;
diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
index a205d4d9f..3874219ba 100644
--- a/drivers/bus/pci/private.h
+++ b/drivers/bus/pci/private.h
@@ -42,6 +42,17 @@ 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,
+ */
+bool rte_pci_ignore_device_addr(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
-- 
2.17.1

Re: [dpdk-dev] [PATCH v4 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Sunil Kumar Kori]

Hello Gaeten,

I have validated hotplug functionality using multi process hotplug application.
Devices are being attached and detached at runtime properly.

Regards
Sunil Kumar Kori

>-----Original Message-----
>From: Sunil Kumar Kori <skori@marvell.com>
>Sent: Friday, May 1, 2020 5:09 PM
>To: stephen@networkplumber.org; david.marchand@redhat.com; Jerin Jacob
>Kollanukkaran <jerinj@marvell.com>; grive@u256.net
>Cc: dev@dpdk.org; Sunil Kumar Kori <skori@marvell.com>
>Subject: [PATCH v4 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>
>---
>v4:
> - Review comments incorporated (Gaeten and David).
> - Rebased on top of tree.
>v3:
> - Remove __rte_experimental from private function.
> - Remove entry from map file too.
>v2:
> - Added function to validate ignorance of device based on PCI address.
> - Marked device validation function as experimental.
>
> drivers/bus/pci/bsd/pci.c    | 12 +++++++++++-
> drivers/bus/pci/linux/pci.c  |  3 +++
> drivers/bus/pci/pci_common.c | 33 ++++++++++++---------------------
> drivers/bus/pci/private.h    | 11 +++++++++++
> 4 files changed, 37 insertions(+), 22 deletions(-)
>
[snip]

Re: [dpdk-dev] [PATCH v4 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Gaëtan Rivet]

Hello Sunil,

It's pretty close, thanks. Unfortunately, I just have a few nits
remaining.

On 01/05/20 17:09 +0530, Sunil Kumar Kori 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>
> ---
> v4:
>  - Review comments incorporated (Gaeten and David).
>  - Rebased on top of tree.
> v3:
>  - Remove __rte_experimental from private function.
>  - Remove entry from map file too.
> v2:
>  - Added function to validate ignorance of device based on PCI address.
>  - Marked device validation function as experimental.
> 
>  drivers/bus/pci/bsd/pci.c    | 12 +++++++++++-
>  drivers/bus/pci/linux/pci.c  |  3 +++
>  drivers/bus/pci/pci_common.c | 33 ++++++++++++---------------------
>  drivers/bus/pci/private.h    | 11 +++++++++++
>  4 files changed, 37 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
> index ebbfeb13a..709a1e7e5 100644
> --- a/drivers/bus/pci/bsd/pci.c
> +++ b/drivers/bus/pci/bsd/pci.c
> @@ -338,6 +338,7 @@ rte_pci_scan(void)
>  			.match_buf_len = sizeof(matches),
>  			.matches = &matches[0],
>  	};
> +	struct rte_pci_addr pci_addr;
>  
>  	/* for debug purposes, PCI can be disabled */
>  	if (!rte_eal_has_pci())
> @@ -357,9 +358,18 @@ 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;
> +
> +			if (rte_pci_ignore_device_addr(&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 ca783b157..ec347eff3 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 (rte_pci_ignore_device_addr(&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..d34e59536 100644
> --- a/drivers/bus/pci/pci_common.c
> +++ b/drivers/bus/pci/pci_common.c
> @@ -42,14 +42,17 @@ const char *rte_pci_get_sysfs_path(void)
>  	return path;
>  }
>  
> -static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
> +static struct rte_devargs *
> +pci_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(&dev->addr, &addr))
> +		if (rte_pci_addr_parse(devargs->name, &addr) < 0)
> +			continue;
> +
> +		if (!rte_pci_addr_cmp(pci_addr, &addr))

I'm really sorry, I was overzealous on your v4. I thought using
devargs->bus->parse() was from your patch, but it was already there.

It's even more shameful as it was me who wrote this.

Anyway, it's much better looking this way, but it should be a separate
patch.

>  			return devargs;
>  	}
>  	return NULL;
> @@ -63,7 +66,7 @@ pci_name_set(struct rte_pci_device *dev)
>  	/* Each device has its internal, canonical name set. */
>  	rte_pci_device_name(&dev->addr,
>  			dev->name, sizeof(dev->name));
> -	devargs = pci_devargs_lookup(dev);
> +	devargs = pci_devargs_lookup(&dev->addr);
>  	dev->device.devargs = devargs;
>  	/* In blacklist mode, if the device is not blacklisted, no
>  	 * rte_devargs exists for it.
> @@ -293,23 +296,12 @@ rte_pci_probe(void)
>  {
>  	struct rte_pci_device *dev = NULL;
>  	size_t probed = 0, failed = 0;
> -	struct rte_devargs *devargs;
> -	int probe_all = 0;
>  	int ret = 0;
>  
> -	if (rte_pci_bus.bus.conf.scan_mode != RTE_BUS_SCAN_WHITELIST)
> -		probe_all = 1;
> -
>  	FOREACH_DEVICE_ON_PCIBUS(dev) {
>  		probed++;
>  
> -		devargs = dev->device.devargs;
> -		/* probe all or only whitelisted devices */
> -		if (probe_all)
> -			ret = pci_probe_all_drivers(dev);
> -		else if (devargs != NULL &&
> -			devargs->policy == RTE_DEV_WHITELISTED)
> -			ret = pci_probe_all_drivers(dev);
> +		ret = pci_probe_all_drivers(dev);
>  		if (ret < 0) {
>  			if (ret != -EEXIST) {
>  				RTE_LOG(ERR, EAL, "Requested device "
> @@ -589,10 +581,10 @@ pci_dma_unmap(struct rte_device *dev, void *addr, uint64_t iova, size_t len)
>  	return -1;
>  }
>  
> -static bool
> -pci_ignore_device(const struct rte_pci_device *dev)
> +bool
> +rte_pci_ignore_device_addr(const struct rte_pci_addr *pci_addr)

I'd prefer naming this function rte_pci_ignore_device(),
given that pci_ignore_device() disappears.

>  {
> -	struct rte_devargs *devargs = dev->device.devargs;
> +	struct rte_devargs *devargs = pci_devargs_lookup(pci_addr);
>  
>  	switch (rte_pci_bus.bus.conf.scan_mode) {
>  	case RTE_BUS_SCAN_WHITELIST:
> @@ -627,8 +619,7 @@ rte_pci_get_iommu_class(void)
>  		if (iommu_no_va == -1)
>  			iommu_no_va = pci_device_iommu_support_va(dev)
>  					? 0 : 1;
> -		if (pci_ignore_device(dev))
> -			continue;
> +
>  		if (dev->kdrv == RTE_KDRV_UNKNOWN ||
>  		    dev->kdrv == RTE_KDRV_NONE)
>  			continue;
> diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
> index a205d4d9f..3874219ba 100644
> --- a/drivers/bus/pci/private.h
> +++ b/drivers/bus/pci/private.h
> @@ -42,6 +42,17 @@ 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,
> + */
> +bool rte_pci_ignore_device_addr(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
> -- 
> 2.17.1
> 

Otherwise it looks good to me, almost finished!

-- 
Gaëtan

Re: [dpdk-dev] [EXT] Re: [PATCH v4 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Sunil Kumar Kori]

>-----Original Message-----
>From: Gaëtan Rivet <grive@u256.net>
>Sent: Saturday, May 2, 2020 2:30 AM
>To: Sunil Kumar Kori <skori@marvell.com>
>Cc: stephen@networkplumber.org; david.marchand@redhat.com; Jerin Jacob
>Kollanukkaran <jerinj@marvell.com>; dev@dpdk.org
>Subject: [EXT] Re: [PATCH v4 1/1] bus/pci: optimise scanning with
>whitelist/blacklist
>
>External Email
>
>----------------------------------------------------------------------
>Hello Sunil,
>
>It's pretty close, thanks. Unfortunately, I just have a few nits remaining.
>
>On 01/05/20 17:09 +0530, Sunil Kumar Kori 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>
>> ---
>> v4:
>>  - Review comments incorporated (Gaeten and David).
>>  - Rebased on top of tree.
>> v3:
>>  - Remove __rte_experimental from private function.
>>  - Remove entry from map file too.
>> v2:
>>  - Added function to validate ignorance of device based on PCI address.
>>  - Marked device validation function as experimental.
>>
>>  drivers/bus/pci/bsd/pci.c    | 12 +++++++++++-
>>  drivers/bus/pci/linux/pci.c  |  3 +++  drivers/bus/pci/pci_common.c |
>> 33 ++++++++++++---------------------
>>  drivers/bus/pci/private.h    | 11 +++++++++++
>>  4 files changed, 37 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
>> index ebbfeb13a..709a1e7e5 100644
>> --- a/drivers/bus/pci/bsd/pci.c
>> +++ b/drivers/bus/pci/bsd/pci.c
>> @@ -338,6 +338,7 @@ rte_pci_scan(void)
>>  			.match_buf_len = sizeof(matches),
>>  			.matches = &matches[0],
>>  	};
>> +	struct rte_pci_addr pci_addr;
>>
>>  	/* for debug purposes, PCI can be disabled */
>>  	if (!rte_eal_has_pci())
>> @@ -357,9 +358,18 @@ 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;
>> +
>> +			if (rte_pci_ignore_device_addr(&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
>> ca783b157..ec347eff3 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 (rte_pci_ignore_device_addr(&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..d34e59536 100644
>> --- a/drivers/bus/pci/pci_common.c
>> +++ b/drivers/bus/pci/pci_common.c
>> @@ -42,14 +42,17 @@ const char *rte_pci_get_sysfs_path(void)
>>  	return path;
>>  }
>>
>> -static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device
>> *dev)
>> +static struct rte_devargs *
>> +pci_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(&dev->addr, &addr))
>> +		if (rte_pci_addr_parse(devargs->name, &addr) < 0)
>> +			continue;
>> +
>> +		if (!rte_pci_addr_cmp(pci_addr, &addr))
>
>I'm really sorry, I was overzealous on your v4. I thought using
>devargs->bus->parse() was from your patch, but it was already there.
>
>It's even more shameful as it was me who wrote this.
>
>Anyway, it's much better looking this way, but it should be a separate patch.
>
No problem. I will revert this change to its original state and later will share separate
Patch to replace devargs->bus->parse.

>>  			return devargs;
>>  	}
>>  	return NULL;
>> @@ -63,7 +66,7 @@ pci_name_set(struct rte_pci_device *dev)
>>  	/* Each device has its internal, canonical name set. */
>>  	rte_pci_device_name(&dev->addr,
>>  			dev->name, sizeof(dev->name));
>> -	devargs = pci_devargs_lookup(dev);
>> +	devargs = pci_devargs_lookup(&dev->addr);
>>  	dev->device.devargs = devargs;
>>  	/* In blacklist mode, if the device is not blacklisted, no
>>  	 * rte_devargs exists for it.
>> @@ -293,23 +296,12 @@ rte_pci_probe(void)  {
>>  	struct rte_pci_device *dev = NULL;
>>  	size_t probed = 0, failed = 0;
>> -	struct rte_devargs *devargs;
>> -	int probe_all = 0;
>>  	int ret = 0;
>>
>> -	if (rte_pci_bus.bus.conf.scan_mode != RTE_BUS_SCAN_WHITELIST)
>> -		probe_all = 1;
>> -
>>  	FOREACH_DEVICE_ON_PCIBUS(dev) {
>>  		probed++;
>>
>> -		devargs = dev->device.devargs;
>> -		/* probe all or only whitelisted devices */
>> -		if (probe_all)
>> -			ret = pci_probe_all_drivers(dev);
>> -		else if (devargs != NULL &&
>> -			devargs->policy == RTE_DEV_WHITELISTED)
>> -			ret = pci_probe_all_drivers(dev);
>> +		ret = pci_probe_all_drivers(dev);
>>  		if (ret < 0) {
>>  			if (ret != -EEXIST) {
>>  				RTE_LOG(ERR, EAL, "Requested device "
>> @@ -589,10 +581,10 @@ pci_dma_unmap(struct rte_device *dev, void
>*addr, uint64_t iova, size_t len)
>>  	return -1;
>>  }
>>
>> -static bool
>> -pci_ignore_device(const struct rte_pci_device *dev)
>> +bool
>> +rte_pci_ignore_device_addr(const struct rte_pci_addr *pci_addr)
>
>I'd prefer naming this function rte_pci_ignore_device(), given that
>pci_ignore_device() disappears.
>
Ack.

>>  {
>> -	struct rte_devargs *devargs = dev->device.devargs;
>> +	struct rte_devargs *devargs = pci_devargs_lookup(pci_addr);
>>
>>  	switch (rte_pci_bus.bus.conf.scan_mode) {
>>  	case RTE_BUS_SCAN_WHITELIST:
>> @@ -627,8 +619,7 @@ rte_pci_get_iommu_class(void)
>>  		if (iommu_no_va == -1)
>>  			iommu_no_va = pci_device_iommu_support_va(dev)
>>  					? 0 : 1;
>> -		if (pci_ignore_device(dev))
>> -			continue;
>> +
>>  		if (dev->kdrv == RTE_KDRV_UNKNOWN ||
>>  		    dev->kdrv == RTE_KDRV_NONE)
>>  			continue;
>> diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
>> index a205d4d9f..3874219ba 100644
>> --- a/drivers/bus/pci/private.h
>> +++ b/drivers/bus/pci/private.h
>> @@ -42,6 +42,17 @@ 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,
>> + */
>> +bool rte_pci_ignore_device_addr(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
>> --
>> 2.17.1
>>
>
>Otherwise it looks good to me, almost finished!
>
>--
>Gaëtan

[dpdk-dev] [PATCH v5 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Sunil Kumar Kori]

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>
---
v5:
 - revert devargs->bus->parse changes.
v4:
 - Review comments incorporated (Gaeten and David).
 - Rebased on top of tree.
v3:
 - Remove __rte_experimental from private function.
 - Remove entry from map file too.
v2:
 - Added function to validate ignorance of device based on PCI address.
 - Marked device validation function as experimental.

 drivers/bus/pci/bsd/pci.c    | 12 +++++++++++-
 drivers/bus/pci/linux/pci.c  |  3 +++
 drivers/bus/pci/pci_common.c | 29 +++++++++--------------------
 drivers/bus/pci/private.h    | 11 +++++++++++
 4 files changed, 34 insertions(+), 21 deletions(-)

diff --git a/drivers/bus/pci/bsd/pci.c b/drivers/bus/pci/bsd/pci.c
index ebbfeb13a..6ec27b4b5 100644
--- a/drivers/bus/pci/bsd/pci.c
+++ b/drivers/bus/pci/bsd/pci.c
@@ -338,6 +338,7 @@ rte_pci_scan(void)
 			.match_buf_len = sizeof(matches),
 			.matches = &matches[0],
 	};
+	struct rte_pci_addr pci_addr;
 
 	/* for debug purposes, PCI can be disabled */
 	if (!rte_eal_has_pci())
@@ -357,9 +358,18 @@ 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;
+
+			if (rte_pci_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 ca783b157..da2f55b3a 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 (rte_pci_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..5da11e4e2 100644
--- a/drivers/bus/pci/pci_common.c
+++ b/drivers/bus/pci/pci_common.c
@@ -42,14 +42,15 @@ const char *rte_pci_get_sysfs_path(void)
 	return path;
 }
 
-static struct rte_devargs *pci_devargs_lookup(struct rte_pci_device *dev)
+static struct rte_devargs *
+pci_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(&dev->addr, &addr))
+		if (!rte_pci_addr_cmp(pci_addr, &addr))
 			return devargs;
 	}
 	return NULL;
@@ -63,7 +64,7 @@ pci_name_set(struct rte_pci_device *dev)
 	/* Each device has its internal, canonical name set. */
 	rte_pci_device_name(&dev->addr,
 			dev->name, sizeof(dev->name));
-	devargs = pci_devargs_lookup(dev);
+	devargs = pci_devargs_lookup(&dev->addr);
 	dev->device.devargs = devargs;
 	/* In blacklist mode, if the device is not blacklisted, no
 	 * rte_devargs exists for it.
@@ -293,23 +294,12 @@ rte_pci_probe(void)
 {
 	struct rte_pci_device *dev = NULL;
 	size_t probed = 0, failed = 0;
-	struct rte_devargs *devargs;
-	int probe_all = 0;
 	int ret = 0;
 
-	if (rte_pci_bus.bus.conf.scan_mode != RTE_BUS_SCAN_WHITELIST)
-		probe_all = 1;
-
 	FOREACH_DEVICE_ON_PCIBUS(dev) {
 		probed++;
 
-		devargs = dev->device.devargs;
-		/* probe all or only whitelisted devices */
-		if (probe_all)
-			ret = pci_probe_all_drivers(dev);
-		else if (devargs != NULL &&
-			devargs->policy == RTE_DEV_WHITELISTED)
-			ret = pci_probe_all_drivers(dev);
+		ret = pci_probe_all_drivers(dev);
 		if (ret < 0) {
 			if (ret != -EEXIST) {
 				RTE_LOG(ERR, EAL, "Requested device "
@@ -589,10 +579,10 @@ pci_dma_unmap(struct rte_device *dev, void *addr, uint64_t iova, size_t len)
 	return -1;
 }
 
-static bool
-pci_ignore_device(const struct rte_pci_device *dev)
+bool
+rte_pci_ignore_device(const struct rte_pci_addr *pci_addr)
 {
-	struct rte_devargs *devargs = dev->device.devargs;
+	struct rte_devargs *devargs = pci_devargs_lookup(pci_addr);
 
 	switch (rte_pci_bus.bus.conf.scan_mode) {
 	case RTE_BUS_SCAN_WHITELIST:
@@ -627,8 +617,7 @@ rte_pci_get_iommu_class(void)
 		if (iommu_no_va == -1)
 			iommu_no_va = pci_device_iommu_support_va(dev)
 					? 0 : 1;
-		if (pci_ignore_device(dev))
-			continue;
+
 		if (dev->kdrv == RTE_KDRV_UNKNOWN ||
 		    dev->kdrv == RTE_KDRV_NONE)
 			continue;
diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
index a205d4d9f..3a9da7322 100644
--- a/drivers/bus/pci/private.h
+++ b/drivers/bus/pci/private.h
@@ -42,6 +42,17 @@ 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,
+ */
+bool rte_pci_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
-- 
2.17.1

Re: [dpdk-dev] [PATCH v5 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Gaëtan Rivet]

On 02/05/20 13:12 +0530, Sunil Kumar Kori 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>

LGTM,
Acked-by: Gaetan Rivet <grive@u256.net>

-- 
Gaëtan

Re: [dpdk-dev] [PATCH v5 1/1] bus/pci: optimise scanning with whitelist/blacklist

[David Marchand]

On Sat, May 2, 2020 at 9:42 AM Sunil Kumar Kori <skori@marvell.com> wrote:
> diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
> index a205d4d9f..3a9da7322 100644
> --- a/drivers/bus/pci/private.h
> +++ b/drivers/bus/pci/private.h
> @@ -42,6 +42,17 @@ 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,

true/false


> + */
> +bool rte_pci_ignore_device(const struct rte_pci_addr *pci_addr);

Gaetan, private API must not be prefixed with rte_, this is for public
APIs only.
I noticed inconsistencies in the pci bus some time ago, I will fix
this patch for now and send some followup patches I had in store.



> +
>  /**
>   * 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

Sunil, no need to send a new version, I will fix this when applying.


Thanks.

-- 
David Marchand

Re: [dpdk-dev] [EXT] Re: [PATCH v5 1/1] bus/pci: optimise scanning with whitelist/blacklist

[Sunil Kumar Kori]

>-----Original Message-----
>From: David Marchand <david.marchand@redhat.com>
>Sent: Monday, May 4, 2020 7:47 PM
>To: Sunil Kumar Kori <skori@marvell.com>; Gaetan Rivet <grive@u256.net>
>Cc: Stephen Hemminger <stephen@networkplumber.org>; Jerin Jacob
>Kollanukkaran <jerinj@marvell.com>; dev <dev@dpdk.org>
>Subject: [EXT] Re: [PATCH v5 1/1] bus/pci: optimise scanning with
>whitelist/blacklist
>
>External Email
>
>----------------------------------------------------------------------
>On Sat, May 2, 2020 at 9:42 AM Sunil Kumar Kori <skori@marvell.com> wrote:
>> diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
>> index a205d4d9f..3a9da7322 100644
>> --- a/drivers/bus/pci/private.h
>> +++ b/drivers/bus/pci/private.h
>> @@ -42,6 +42,17 @@ 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,
>
>true/false
>
>
>> + */
>> +bool rte_pci_ignore_device(const struct rte_pci_addr *pci_addr);
>
>Gaetan, private API must not be prefixed with rte_, this is for public APIs only.
>I noticed inconsistencies in the pci bus some time ago, I will fix this patch for
>now and send some followup patches I had in store.
>
>
>
>> +
>>  /**
>>   * 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
>
>Sunil, no need to send a new version, I will fix this when applying.
>
>
Ack. 

>Thanks.
>
>--
>David Marchand

Re: [dpdk-dev] [PATCH v5 1/1] bus/pci: optimise scanning with whitelist/blacklist

[David Marchand]

On Mon, May 4, 2020 at 4:17 PM David Marchand <david.marchand@redhat.com> wrote:
>
> On Sat, May 2, 2020 at 9:42 AM Sunil Kumar Kori <skori@marvell.com> wrote:
> I noticed inconsistencies in the pci bus some time ago, I will fix
> this patch for now and send some followup patches I had in store.

I only sent the minimum, there are other changes like hiding internal
structures that could go later.
https://patchwork.dpdk.org/project/dpdk/list/?series=9874


-- 
David Marchand

Re: [dpdk-dev] [PATCH v5 1/1] bus/pci: optimise scanning with whitelist/blacklist

[David Marchand]

On Sat, May 2, 2020 at 9:42 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>

Acked-by: Gaetan Rivet <grive@u256.net>


> diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
> index a205d4d9f..3a9da7322 100644
> --- a/drivers/bus/pci/private.h
> +++ b/drivers/bus/pci/private.h
> @@ -42,6 +42,17 @@ 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.

PCI address

> + *
> + * @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,

On Sat, May 2, 2020 at 9:42 AM Sunil Kumar Kori <skori@marvell.com> wrote:
> diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h
> index a205d4d9f..3a9da7322 100644
> --- a/drivers/bus/pci/private.h
> +++ b/drivers/bus/pci/private.h
> @@ -42,6 +42,17 @@ 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,

true/false

> + */
> +bool rte_pci_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

Applied, thanks.


-- 
David Marchand