[PATCH 1/1] pci: default to whitelist mode

[PATCH 1/1] pci: default to whitelist mode

[Gaetan Rivet]

Expects all devices to be explicitly defined before being probed. Using
the -b option will activate blacklist mode instead.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 15a0c48..ad0a8be 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -418,7 +418,7 @@ rte_eal_pci_probe(void)
 	int probe_all = 0;
 	int ret = 0;
 
-	if (rte_eal_devargs_type_count(RTE_DEVTYPE_WHITELISTED_PCI) == 0)
+	if (rte_eal_devargs_type_count(RTE_DEVTYPE_BLACKLISTED_PCI) > 0)
 		probe_all = 1;
 
 	TAILQ_FOREACH(dev, &pci_device_list, next) {
-- 
2.1.4

Re: [PATCH 1/1] pci: default to whitelist mode

[Bruce Richardson]

On Tue, Mar 28, 2017 at 01:51:57PM +0200, Gaetan Rivet wrote:
> Expects all devices to be explicitly defined before being probed. Using
> the -b option will activate blacklist mode instead.
> 
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
So does this change the default behaviour when no black or whitelist
options are defined? Instead of having all devices polled, we will have
none?

/Bruce

[PATCH v2 1/1] pci: default to whitelist mode

[Gaetan Rivet]

Expects all devices to be explicitly defined before being probed.

The blacklist mode can be prone to errors, coaxing users in capturing
devices that could be used for management or otherwise.
The whitelist mode offers users more control and highlight mistakes by
making them visible on the command line.

This is more useful to have a clear idea of the state of the system used,
which is better in the context of standalone / headless applications.

Using the -b option will revert to the original behavior.

Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
v2: justify this default behavior evolution.
---
 lib/librte_eal/common/eal_common_pci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_pci.c b/lib/librte_eal/common/eal_common_pci.c
index 15a0c48..ad0a8be 100644
--- a/lib/librte_eal/common/eal_common_pci.c
+++ b/lib/librte_eal/common/eal_common_pci.c
@@ -418,7 +418,7 @@ rte_eal_pci_probe(void)
 	int probe_all = 0;
 	int ret = 0;
 
-	if (rte_eal_devargs_type_count(RTE_DEVTYPE_WHITELISTED_PCI) == 0)
+	if (rte_eal_devargs_type_count(RTE_DEVTYPE_BLACKLISTED_PCI) > 0)
 		probe_all = 1;
 
 	TAILQ_FOREACH(dev, &pci_device_list, next) {
-- 
2.1.4

Re: [PATCH 1/1] pci: default to whitelist mode

[Gaëtan Rivet]

Hi Bruce,

On Tue, Mar 28, 2017 at 12:58:53PM +0100, Bruce Richardson wrote:
>On Tue, Mar 28, 2017 at 01:51:57PM +0200, Gaetan Rivet wrote:
>> Expects all devices to be explicitly defined before being probed. Using
>> the -b option will activate blacklist mode instead.
>>
>> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
>> ---
>So does this change the default behaviour when no black or whitelist
>options are defined? Instead of having all devices polled, we will have
>none?
>

Yes, precisely.

I was a little quick to send this patch, I sent a new version with more 
explanation as for my rationale behind this change:

http://dpdk.org/ml/archives/dev/2017-March/061818.html

>/Bruce

-- 
Gaëtan Rivet
6WIND

Re: [PATCH v2 1/1] pci: default to whitelist mode

[Bruce Richardson]

On Tue, Mar 28, 2017 at 02:01:29PM +0200, Gaetan Rivet wrote:
> Expects all devices to be explicitly defined before being probed.
> 
> The blacklist mode can be prone to errors, coaxing users in capturing
> devices that could be used for management or otherwise.
> The whitelist mode offers users more control and highlight mistakes by
> making them visible on the command line.
> 
> This is more useful to have a clear idea of the state of the system used,
> which is better in the context of standalone / headless applications.
> 
> Using the -b option will revert to the original behavior.
> 
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
> v2: justify this default behavior evolution.
> ---

I don't have major objections to this patch, though it does make it
mandatory to use port parameters where before it was not. The one
suggestion I will make is that, if we take this approach, we should
probably add a --wl-all (whitelist-all) flag to go back to having all
ports automatically bound, if so desired.

/Bruce

Re: [PATCH v2 1/1] pci: default to whitelist mode

[Gaëtan Rivet]

On Tue, Mar 28, 2017 at 01:20:00PM +0100, Bruce Richardson wrote:
>On Tue, Mar 28, 2017 at 02:01:29PM +0200, Gaetan Rivet wrote:
>> Expects all devices to be explicitly defined before being probed.
>>
>> The blacklist mode can be prone to errors, coaxing users in capturing
>> devices that could be used for management or otherwise.
>> The whitelist mode offers users more control and highlight mistakes by
>> making them visible on the command line.
>>
>> This is more useful to have a clear idea of the state of the system used,
>> which is better in the context of standalone / headless applications.
>>
>> Using the -b option will revert to the original behavior.
>>
>> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
>> ---
>> v2: justify this default behavior evolution.
>> ---
>
>I don't have major objections to this patch, though it does make it
>mandatory to use port parameters where before it was not. The one
>suggestion I will make is that, if we take this approach, we should
>probably add a --wl-all (whitelist-all) flag to go back to having all
>ports automatically bound, if so desired.
>

Are there use cases where the blacklist mode would be used without 
blacklisting any device? The current -b option is almost enough for the 
same level of functionality.

If there is an actual need to a full PCI probe, adding this option is 
certainly possible. I was thinking otherwise of allowing "all" as an 
argument to -w, which would have our users using -wall or -w=all, which 
seems clear enough. This would essentially be the inverse of the 
--no-pci parameter.

Which could probably be removed if this patch is accepted.

-- 
Gaëtan Rivet
6WIND

Re: [PATCH v2 1/1] pci: default to whitelist mode

[Van Haaren, Harry]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gaëtan Rivet
> Sent: Tuesday, March 28, 2017 1:44 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; Thomas Monjalon <thomas.monjalon@6wind.com>
> Subject: Re: [dpdk-dev] [PATCH v2 1/1] pci: default to whitelist mode
> 
> On Tue, Mar 28, 2017 at 01:20:00PM +0100, Bruce Richardson wrote:
> >On Tue, Mar 28, 2017 at 02:01:29PM +0200, Gaetan Rivet wrote:
> >> Expects all devices to be explicitly defined before being probed.
> >>
> >> The blacklist mode can be prone to errors, coaxing users in capturing
> >> devices that could be used for management or otherwise.
> >> The whitelist mode offers users more control and highlight mistakes by
> >> making them visible on the command line.
> >>
> >> This is more useful to have a clear idea of the state of the system used,
> >> which is better in the context of standalone / headless applications.
> >>
> >> Using the -b option will revert to the original behavior.
> >>
> >> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> >> ---
> >> v2: justify this default behavior evolution.
> >> ---
> >
> >I don't have major objections to this patch, though it does make it
> >mandatory to use port parameters where before it was not. The one
> >suggestion I will make is that, if we take this approach, we should
> >probably add a --wl-all (whitelist-all) flag to go back to having all
> >ports automatically bound, if so desired.
> >
> 
> Are there use cases where the blacklist mode would be used without
> blacklisting any device? The current -b option is almost enough for the
> same level of functionality.
> 
> If there is an actual need to a full PCI probe, adding this option is
> certainly possible. I was thinking otherwise of allowing "all" as an
> argument to -w, which would have our users using -wall or -w=all, which
> seems clear enough.


If I understand correctly an app that runs without any port parameters to EAL would now fail to find any ports?

That would result in;
- testing frameworks (DTS, fd.io perf lab, customers, etc) would fail if not specifying ports
- beginners just running ./app/testpmd would need to specify the "magic" -w-all
- confusion about why previously working DPDK apps are now failing due to not finding any devices

I'm not totally opposed, but we should consider carefully what impacts this change will have across the whole DPDK ecosystem, and if the change is worth it. If decided that "Yes its worth it", we would need to communicate this change very clearly. All documentation regarding running any DPDK app would need to be updated as part of this change.

Personally I don't see the large benefit this patch brings, but more of a disturbance in the DPDK; I'm open to be convinced otherwise.


> This would essentially be the inverse of the
> --no-pci parameter.
> 
> Which could probably be removed if this patch is accepted.
> 
> --
> Gaëtan Rivet
> 6WIND

Re: [PATCH v2 1/1] pci: default to whitelist mode

[Bruce Richardson]

On Tue, Mar 28, 2017 at 02:44:09PM +0200, Gaëtan Rivet wrote:
> On Tue, Mar 28, 2017 at 01:20:00PM +0100, Bruce Richardson wrote:
> > On Tue, Mar 28, 2017 at 02:01:29PM +0200, Gaetan Rivet wrote:
> > > Expects all devices to be explicitly defined before being probed.
> > > 
> > > The blacklist mode can be prone to errors, coaxing users in
> > > capturing devices that could be used for management or otherwise.
> > > The whitelist mode offers users more control and highlight
> > > mistakes by making them visible on the command line.
> > > 
> > > This is more useful to have a clear idea of the state of the
> > > system used, which is better in the context of standalone /
> > > headless applications.
> > > 
> > > Using the -b option will revert to the original behavior.
> > > 
> > > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com> --- v2:
> > > justify this default behavior evolution.  ---
> > 
> > I don't have major objections to this patch, though it does make it
> > mandatory to use port parameters where before it was not. The one
> > suggestion I will make is that, if we take this approach, we should
> > probably add a --wl-all (whitelist-all) flag to go back to having
> > all ports automatically bound, if so desired.
> > 
> 
> Are there use cases where the blacklist mode would be used without
> blacklisting any device? The current -b option is almost enough for
> the same level of functionality.
>
Yes. 
For ports used for management, those will probably remain bound to
the regular kernel driver, and not available for DPDK use. That means
that the DPDK app need not specify any blacklist or whitelist options
right now, you can determine what ports to use or not simply by binding
to a uio/vfio driver or not at system setup time.

Is this not the normal way people do port setup for DPDK?

> If there is an actual need to a full PCI probe, adding this option is
> certainly possible. I was thinking otherwise of allowing "all" as an
> argument to -w, which would have our users using -wall or -w=all, which
> seems clear enough. This would essentially be the inverse of the --no-pci
> parameter.
> 
> Which could probably be removed if this patch is accepted.
> 
> -- 
> Gaëtan Rivet
> 6WIND

Re: [PATCH v2 1/1] pci: default to whitelist mode

[Gaëtan Rivet]

On Tue, Mar 28, 2017 at 02:03:48PM +0100, Bruce Richardson wrote:
>On Tue, Mar 28, 2017 at 02:44:09PM +0200, Gaëtan Rivet wrote:
>> On Tue, Mar 28, 2017 at 01:20:00PM +0100, Bruce Richardson wrote:
>> > On Tue, Mar 28, 2017 at 02:01:29PM +0200, Gaetan Rivet wrote:
>> > > Expects all devices to be explicitly defined before being probed.
>> > >
>> > > The blacklist mode can be prone to errors, coaxing users in
>> > > capturing devices that could be used for management or otherwise.
>> > > The whitelist mode offers users more control and highlight
>> > > mistakes by making them visible on the command line.
>> > >
>> > > This is more useful to have a clear idea of the state of the
>> > > system used, which is better in the context of standalone /
>> > > headless applications.
>> > >
>> > > Using the -b option will revert to the original behavior.
>> > >
>> > > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com> --- v2:
>> > > justify this default behavior evolution.  ---
>> >
>> > I don't have major objections to this patch, though it does make it
>> > mandatory to use port parameters where before it was not. The one
>> > suggestion I will make is that, if we take this approach, we should
>> > probably add a --wl-all (whitelist-all) flag to go back to having
>> > all ports automatically bound, if so desired.
>> >
>>
>> Are there use cases where the blacklist mode would be used without
>> blacklisting any device? The current -b option is almost enough for
>> the same level of functionality.
>>
>Yes.
>For ports used for management, those will probably remain bound to
>the regular kernel driver, and not available for DPDK use. That means
>that the DPDK app need not specify any blacklist or whitelist options
>right now, you can determine what ports to use or not simply by binding
>to a uio/vfio driver or not at system setup time.
>

Yes, what I (slightly) dislike about that is that we uselessly scan a 
few devices as well as run probes against them by enumerating all DPDK 
drivers and seeing what sticks to the wall. Sure it works and it worked 
that way for some time, but in the end this just seems a little hackish.

>Is this not the normal way people do port setup for DPDK?
>

I cannot really speak for other users, my point in putting this patch 
out there is also to have a survey of usages and see if anyone is 
resolutely against it.

At least for 6WIND, we indeed use the whitelist mode in general. This is 
better for having deterministic test runs among heterogeneous hardware 
platforms and explicit test and bug reports.

>> If there is an actual need to a full PCI probe, adding this option is
>> certainly possible. I was thinking otherwise of allowing "all" as an
>> argument to -w, which would have our users using -wall or -w=all, which
>> seems clear enough. This would essentially be the inverse of the --no-pci
>> parameter.
>>
>> Which could probably be removed if this patch is accepted.
>>
>> --
>> Gaëtan Rivet
>> 6WIND

-- 
Gaëtan Rivet
6WIND

Re: [PATCH v2 1/1] pci: default to whitelist mode

[Gaëtan Rivet]

On Tue, Mar 28, 2017 at 01:02:13PM +0000, Van Haaren, Harry wrote:
>If I understand correctly an app that runs without any port parameters to EAL would now fail to find any ports?
>
>That would result in;
>- testing frameworks (DTS, fd.io perf lab, customers, etc) would fail if not specifying ports

Yes, sure. There are certainly people who would be impacted. I'd be 
curious however to hear from them and know exactly how many are using 
the blacklist mode.

If I am writing a test for a device usually I explicitly specify the 
device and the corresponding topology. This always results in whitelist 
parameters. I can certainly imagine other people working differently.

>- beginners just running ./app/testpmd would need to specify the "magic" -w-all

Remembering starting with DPDK a few years back, I was actually confused 
a few times by needing to blacklist a few devices. The DPDK 
use case is extremely specific and my first intuition was that I'd 
have to assign specific ports.

The blacklist mode was pretty much justified to me at the time as an 
historic cruft left there because no one wanted the hassle of removing 
it. I have never used it personally, so I'd be curious to hear about 
other users that would design their tests and application to rely on 
this blacklist mode.

>- confusion about why previously working DPDK apps are now failing due to not finding any devices
>
>I'm not totally opposed, but we should consider carefully what impacts this change will have across the whole DPDK ecosystem, and if the change is worth it. If decided that "Yes its worth it", we would need to communicate this change very clearly. All documentation regarding running any DPDK app would need to be updated as part of this change.
>
>Personally I don't see the large benefit this patch brings, but more of a disturbance in the DPDK; I'm open to be convinced otherwise.
>

Ah sure, it won't happen overnight. It would be coming no sooner than 
17.08, even maybe 17.11. It would have several deprecation notices for 
parameters that would change or disappear, and indeed the documentation 
should be updated in many places.

I'm all for it personally.

-- 
Gaëtan Rivet
6WIND

Re: [PATCH v2 1/1] pci: default to whitelist mode

[Thomas Monjalon]

2017-03-28 13:02, Van Haaren, Harry:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gaëtan Rivet
> > On Tue, Mar 28, 2017 at 01:20:00PM +0100, Bruce Richardson wrote:
> > >On Tue, Mar 28, 2017 at 02:01:29PM +0200, Gaetan Rivet wrote:
> > >> Expects all devices to be explicitly defined before being probed.
> > >>
> > >> The blacklist mode can be prone to errors, coaxing users in capturing
> > >> devices that could be used for management or otherwise.
> > >> The whitelist mode offers users more control and highlight mistakes by
> > >> making them visible on the command line.
> > >>
> > >> This is more useful to have a clear idea of the state of the system used,
> > >> which is better in the context of standalone / headless applications.
> > >>
> > >> Using the -b option will revert to the original behavior.
> > >>
> > >> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > >> ---
> > >> v2: justify this default behavior evolution.
> > >> ---
> > >
> > >I don't have major objections to this patch, though it does make it
> > >mandatory to use port parameters where before it was not. The one
> > >suggestion I will make is that, if we take this approach, we should
> > >probably add a --wl-all (whitelist-all) flag to go back to having all
> > >ports automatically bound, if so desired.
> > >
> > 
> > Are there use cases where the blacklist mode would be used without
> > blacklisting any device? The current -b option is almost enough for the
> > same level of functionality.
> > 
> > If there is an actual need to a full PCI probe, adding this option is
> > certainly possible. I was thinking otherwise of allowing "all" as an
> > argument to -w, which would have our users using -wall or -w=all, which
> > seems clear enough.
> 
> 
> If I understand correctly an app that runs without any port parameters to EAL would now fail to find any ports?
> 
> That would result in;
> - testing frameworks (DTS, fd.io perf lab, customers, etc) would fail if not specifying ports
> - beginners just running ./app/testpmd would need to specify the "magic" -w-all
> - confusion about why previously working DPDK apps are now failing due to not finding any devices
> 
> I'm not totally opposed, but we should consider carefully what impacts this change will have across the whole DPDK ecosystem, and if the change is worth it. If decided that "Yes its worth it", we would need to communicate this change very clearly. All documentation regarding running any DPDK app would need to be updated as part of this change.
> 
> Personally I don't see the large benefit this patch brings, but more of a disturbance in the DPDK; I'm open to be convinced otherwise.

I agree it is much more a disturbance to change the default behaviour.
Both options -b/-w are already available for people who really care.

Anyway, the default behaviour should be an application policy.
One day, we will have to make all these command line magics optional,
and rely on the application implementation.