Re: [PATCH 0/9] rte_bus parse API

On Thu, Jun 08, 2017 at 01:38:12PM +0200, Jan Blunck wrote:

On Wed, Jun 7, 2017 at 9:55 PM, Gaëtan Rivet [off-list ref] wrote:

quoted

On Wed, Jun 07, 2017 at 07:22:05PM +0200, Jan Blunck wrote:

quoted

On Wed, May 24, 2017 at 5:12 PM, Gaetan Rivet [off-list ref] wrote:

quoted

Following the evolutions announced in [1], here is the first part of
the rte_devargs rework planned for 17.08. The rationale has been partially
explained in [2].

This first part covers the introduction of the necessary facilities in
rte_bus to allow for generic device parsing. This API is implemented for
the virtual and PCI buses. Additionally, this rte_bus evolution is being
used within rte_devargs to characterize a device type by its bus.
This work is the first of two parts to reduce the dependency of the EAL
upon specific bus implementations.

Two public functions are added to rte_bus to help bus recognition:

- rte_bus_from_name
- rte_bus_from_dev

These functions are made public because the bus handle within devargs
becomes the generic device type. Recognizing device types is useful for
buses and PMDs alike.
The modified rte_devargs parsing allows declaring on the EAL command line
explicit buses to handle a device. The format is as follow:

  --vdev="virtual:net_ring0" --vdev="net_tap0,iface=tap0"
  -w PCI:00:02.0 -w 00:03.0

I don't see the point of doing this. The --vdev parameter implicitly
defines the bus by its name (--vdev aka virtual device).

Why don't you add a commandline "--dev" parameter that supports a
"bus=" devarg? You would need to clarify what that means for other
busses than the virtual one. Is the bus switched into whitelist mode
by that?

We cannot keep the current -w, -b and --vdev parameter. Those are
processed by the EAL, and use specifics from the virtual and PCI buses.

The rte_devargs rework has been to make the same functionality generic
to all rte_bus. As seen quickly in [2], rte_devargs has two functions:

* Validating a device declaration
* Keeping the relevant device info until it has been processed.

I don't agree with the validation step. This is highly
device/driver/bus specific and I don't believe that just because you
have created a rte_devargs it is a guarantee that the device is valid.

It is the current API. If an rte_devargs is added to the global list,
then it means that the embedded information has been validated. I
kept the same behavior but made it generic in the new version.

Besides that it makes statically embedding rte_devargs into other
structures impossible.

Why? In the failsafe PMD, I statically embed an rte_devargs to avoid
having to insert one in the global device list to remove it afterward.
What limitation to this do you see? I think I misunderstand what you
mean.

As I see it rte_devargs is a key-value list
with some keys that are generic. This would make application
development much easier.

The previously encoded information was:

- Scan policy / bus (in the type field)
- Device designation (PCI location / virtual driver)
- kvargs

Beside the kvargs, none were generic, so I made them so. I
divided the type field into two separate elements as it was conflating
two concepts.

I do not understand how much simpler we can make this. The fields have
not changed, nor has the information itself. Everything has however
been genericized, which is necessary.

quoted

Both functionalities have been genericized. This results in all parameters
being able to be used with all types of devices. This is inherent to the
EAL becoming bus-agnostic.

Now, it is absolutely possible to rename for example -w as --dev, as it
is the expected behavior from users. This however should be discussed by
the community, last time I talked about the possibility of switching the
default of the PCI bus to whitelist mode the community wasn't all that
enthused by the prospect.

Finally, I do not like the idea of a special devarg just for declaring
explicitly buses for devices. The bus is not a device modifier, nor is
it a driver parameter. The bus is a way to define the location of the
device on the system. Adding a special "bus=" devargs means having some
preprocessing done on devargs upon rte_devargs allocation. This was
already abused by the bonding PMD with the driver= parameter. I do not
support this and did not want to repeat it. Passing down the device args
is a simple process and we should keep it as simple as possible.

I know you do not like having the bus as part of the device name.
As a compromise, I made the current system flexible and allowed the legacy
device definition to be kept.

However with a purely generic process, it is necessary to at least offer
the possibility to the user to explicitly use a bus, as nothing prevents
conflicting device names from existing.

quoted

quoted

[2]: http://dpdk.org/ml/archives/dev/2017-May/065670.html

quoted

quoted

This explicit bus designation is optional; no evolution is currently
forced on users to migrate to this new format. The separating character is
arbitrary and can be any character illegal within a bus name.
Subsequently, what is allowed within a bus name has been formally
defined and is now enforced.

[1]: http://dpdk.org/ml/archives/dev/2017-May/065634.html
[2]: http://dpdk.org/ml/archives/dev/2017-May/065670.html

This patchset depends on:

bus: attach / detach API
http://dpdk.org/ml/archives/dev/2017-May/066330.html
http://dpdk.org/dev/patchwork/patch/24489/

Gaetan Rivet (9):
  bus: fix bus name registration
  bus: verify bus name on registration
  bus: introduce parsing functionality
  vdev: implement parse bus operation
  pci: implement parse bus operation
  bus: add helper to find bus from a name
  bus: add helper to find a bus from a device name
  vdev: expose bus name
  devargs: parse bus info

 lib/librte_eal/bsdapp/eal/rte_eal_version.map   |  8 +++
 lib/librte_eal/common/eal_common_bus.c          | 47 +++++++++++++++++
 lib/librte_eal/common/eal_common_devargs.c      | 17 +++++-
 lib/librte_eal/common/eal_common_pci.c          | 19 +++++++
 lib/librte_eal/common/eal_common_vdev.c         | 70 ++++++++++++++-----------
 lib/librte_eal/common/eal_private.h             | 16 ++++++
 lib/librte_eal/common/include/rte_bus.h         | 49 ++++++++++++++++-
 lib/librte_eal/common/include/rte_devargs.h     |  3 ++
 lib/librte_eal/common/include/rte_vdev.h        |  2 +
 lib/librte_eal/linuxapp/eal/rte_eal_version.map |  8 +++
 10 files changed, 205 insertions(+), 34 deletions(-)

--
2.1.4

--
Gaėtan Rivet
6WIND

-- 
Gaëtan Rivet
6WIND