[dpdk-dev,7/9] bus: add helper to find a bus from a device name
Checks
Commit Message
Find which bus should be able to parse this device name into an internal
device representation.
Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
lib/librte_eal/bsdapp/eal/rte_eal_version.map | 1 +
lib/librte_eal/common/eal_common_bus.c | 15 +++++++++++++++
lib/librte_eal/common/include/rte_bus.h | 12 ++++++++++++
lib/librte_eal/linuxapp/eal/rte_eal_version.map | 1 +
4 files changed, 29 insertions(+)
Comments
On Wed, May 24, 2017 at 5:12 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> Find which bus should be able to parse this device name into an internal
> device representation.
>
No, please don't add this. One should know to what bus a device
belongs to before plugging it. Artificially encoding the parent bus
into the device name is not the right thing to do. Please keep those
things separate.
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
> lib/librte_eal/bsdapp/eal/rte_eal_version.map | 1 +
> lib/librte_eal/common/eal_common_bus.c | 15 +++++++++++++++
> lib/librte_eal/common/include/rte_bus.h | 12 ++++++++++++
> lib/librte_eal/linuxapp/eal/rte_eal_version.map | 1 +
> 4 files changed, 29 insertions(+)
>
> diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> index 3517d74..04fa882 100644
> --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> @@ -202,5 +202,6 @@ DPDK_17.08 {
> global:
>
> rte_bus_from_name;
> + rte_bus_from_dev;
>
> } DPDK_17.05;
> diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
> index 7977190..08fff60 100644
> --- a/lib/librte_eal/common/eal_common_bus.c
> +++ b/lib/librte_eal/common/eal_common_bus.c
> @@ -242,3 +242,18 @@ rte_bus_from_name(const char *str)
> return NULL;
> return rte_bus_find(bus_cmp_name, str);
> }
> +
> +static int
> +bus_can_parse(const struct rte_bus *bus, const void *_name)
> +{
> + const char *name = _name;
> +
> + return (bus->parse && !bus->parse(name, NULL));
> +}
> +
> +/* find a bus capable of parsing a device description */
> +struct rte_bus *
> +rte_bus_from_dev(const char *str)
> +{
> + return rte_bus_find(bus_can_parse, str);
> +}
> diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
> index 5b87ac4..0b48e66 100644
> --- a/lib/librte_eal/common/include/rte_bus.h
> +++ b/lib/librte_eal/common/include/rte_bus.h
> @@ -251,6 +251,18 @@ struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);
> struct rte_bus *rte_bus_from_name(const char *str);
>
> /**
> + * Find a bus capable of identifying a device.
> + *
> + * @param str
> + * A device identifier (PCI address, virtual PMD name, ...).
> + *
> + * @return
> + * A valid bus structure if found.
> + * NULL if no bus is able to parse this device.
> + */
> +struct rte_bus *rte_bus_from_dev(const char *str);
> +
> +/**
> * Helper for Bus registration.
> * The constructor has higher priority than PMD constructors.
> */
> diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> index 6607acc..a5127d6 100644
> --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> @@ -206,5 +206,6 @@ DPDK_17.08 {
> global:
>
> rte_bus_from_name;
> + rte_bus_from_dev;
>
> } DPDK_17.05;
> --
> 2.1.4
>
On Wed, Jun 07, 2017 at 07:28:07PM +0200, Jan Blunck wrote:
> On Wed, May 24, 2017 at 5:12 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> > Find which bus should be able to parse this device name into an internal
> > device representation.
> >
>
> No, please don't add this. One should know to what bus a device
> belongs to before plugging it. Artificially encoding the parent bus
> into the device name is not the right thing to do. Please keep those
> things separate.
>
The EAL has no way to know this currently. As you noted, it has to know
onto which bus a device belongs before plugging it.
> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> > ---
> > lib/librte_eal/bsdapp/eal/rte_eal_version.map | 1 +
> > lib/librte_eal/common/eal_common_bus.c | 15 +++++++++++++++
> > lib/librte_eal/common/include/rte_bus.h | 12 ++++++++++++
> > lib/librte_eal/linuxapp/eal/rte_eal_version.map | 1 +
> > 4 files changed, 29 insertions(+)
> >
> > diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > index 3517d74..04fa882 100644
> > --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> > @@ -202,5 +202,6 @@ DPDK_17.08 {
> > global:
> >
> > rte_bus_from_name;
> > + rte_bus_from_dev;
> >
> > } DPDK_17.05;
> > diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
> > index 7977190..08fff60 100644
> > --- a/lib/librte_eal/common/eal_common_bus.c
> > +++ b/lib/librte_eal/common/eal_common_bus.c
> > @@ -242,3 +242,18 @@ rte_bus_from_name(const char *str)
> > return NULL;
> > return rte_bus_find(bus_cmp_name, str);
> > }
> > +
> > +static int
> > +bus_can_parse(const struct rte_bus *bus, const void *_name)
> > +{
> > + const char *name = _name;
> > +
> > + return (bus->parse && !bus->parse(name, NULL));
> > +}
> > +
> > +/* find a bus capable of parsing a device description */
> > +struct rte_bus *
> > +rte_bus_from_dev(const char *str)
> > +{
> > + return rte_bus_find(bus_can_parse, str);
> > +}
> > diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
> > index 5b87ac4..0b48e66 100644
> > --- a/lib/librte_eal/common/include/rte_bus.h
> > +++ b/lib/librte_eal/common/include/rte_bus.h
> > @@ -251,6 +251,18 @@ struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);
> > struct rte_bus *rte_bus_from_name(const char *str);
> >
> > /**
> > + * Find a bus capable of identifying a device.
> > + *
> > + * @param str
> > + * A device identifier (PCI address, virtual PMD name, ...).
> > + *
> > + * @return
> > + * A valid bus structure if found.
> > + * NULL if no bus is able to parse this device.
> > + */
> > +struct rte_bus *rte_bus_from_dev(const char *str);
> > +
> > +/**
> > * Helper for Bus registration.
> > * The constructor has higher priority than PMD constructors.
> > */
> > diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > index 6607acc..a5127d6 100644
> > --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> > @@ -206,5 +206,6 @@ DPDK_17.08 {
> > global:
> >
> > rte_bus_from_name;
> > + rte_bus_from_dev;
> >
> > } DPDK_17.05;
> > --
> > 2.1.4
> >
On Wed, Jun 7, 2017 at 10:03 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> On Wed, Jun 07, 2017 at 07:28:07PM +0200, Jan Blunck wrote:
>> On Wed, May 24, 2017 at 5:12 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
>> > Find which bus should be able to parse this device name into an internal
>> > device representation.
>> >
>>
>> No, please don't add this. One should know to what bus a device
>> belongs to before plugging it. Artificially encoding the parent bus
>> into the device name is not the right thing to do. Please keep those
>> things separate.
>>
>
When plugging a device the users know about:
- bus name
- device name
Its not the case that the users invent the device names out of thin
air. The EAL shouldn't codify what the users of the EAL already know
about.
> The EAL has no way to know this currently. As you noted, it has to know
> onto which bus a device belongs before plugging it.
>
>> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
>> > ---
>> > lib/librte_eal/bsdapp/eal/rte_eal_version.map | 1 +
>> > lib/librte_eal/common/eal_common_bus.c | 15 +++++++++++++++
>> > lib/librte_eal/common/include/rte_bus.h | 12 ++++++++++++
>> > lib/librte_eal/linuxapp/eal/rte_eal_version.map | 1 +
>> > 4 files changed, 29 insertions(+)
>> >
>> > diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
>> > index 3517d74..04fa882 100644
>> > --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
>> > +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
>> > @@ -202,5 +202,6 @@ DPDK_17.08 {
>> > global:
>> >
>> > rte_bus_from_name;
>> > + rte_bus_from_dev;
>> >
>> > } DPDK_17.05;
>> > diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
>> > index 7977190..08fff60 100644
>> > --- a/lib/librte_eal/common/eal_common_bus.c
>> > +++ b/lib/librte_eal/common/eal_common_bus.c
>> > @@ -242,3 +242,18 @@ rte_bus_from_name(const char *str)
>> > return NULL;
>> > return rte_bus_find(bus_cmp_name, str);
>> > }
>> > +
>> > +static int
>> > +bus_can_parse(const struct rte_bus *bus, const void *_name)
>> > +{
>> > + const char *name = _name;
>> > +
>> > + return (bus->parse && !bus->parse(name, NULL));
>> > +}
>> > +
>> > +/* find a bus capable of parsing a device description */
>> > +struct rte_bus *
>> > +rte_bus_from_dev(const char *str)
>> > +{
>> > + return rte_bus_find(bus_can_parse, str);
>> > +}
>> > diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
>> > index 5b87ac4..0b48e66 100644
>> > --- a/lib/librte_eal/common/include/rte_bus.h
>> > +++ b/lib/librte_eal/common/include/rte_bus.h
>> > @@ -251,6 +251,18 @@ struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);
>> > struct rte_bus *rte_bus_from_name(const char *str);
>> >
>> > /**
>> > + * Find a bus capable of identifying a device.
>> > + *
>> > + * @param str
>> > + * A device identifier (PCI address, virtual PMD name, ...).
>> > + *
>> > + * @return
>> > + * A valid bus structure if found.
>> > + * NULL if no bus is able to parse this device.
>> > + */
>> > +struct rte_bus *rte_bus_from_dev(const char *str);
>> > +
>> > +/**
>> > * Helper for Bus registration.
>> > * The constructor has higher priority than PMD constructors.
>> > */
>> > diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
>> > index 6607acc..a5127d6 100644
>> > --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
>> > +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
>> > @@ -206,5 +206,6 @@ DPDK_17.08 {
>> > global:
>> >
>> > rte_bus_from_name;
>> > + rte_bus_from_dev;
>> >
>> > } DPDK_17.05;
>> > --
>> > 2.1.4
>> >
>
> --
> Gaėtan Rivet
> 6WIND
On Thu, Jun 08, 2017 at 12:45:17PM +0200, Jan Blunck wrote:
> On Wed, Jun 7, 2017 at 10:03 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> > On Wed, Jun 07, 2017 at 07:28:07PM +0200, Jan Blunck wrote:
> >> On Wed, May 24, 2017 at 5:12 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> >> > Find which bus should be able to parse this device name into an internal
> >> > device representation.
> >> >
> >>
> >> No, please don't add this. One should know to what bus a device
> >> belongs to before plugging it. Artificially encoding the parent bus
> >> into the device name is not the right thing to do. Please keep those
> >> things separate.
> >>
> >
>
> When plugging a device the users know about:
> - bus name
> - device name
>
> Its not the case that the users invent the device names out of thin
> air. The EAL shouldn't codify what the users of the EAL already know
> about.
>
>
Yes, but in that case the user is forced to explicitly name the bus used
for a device.
I think it might be sufficient to have this as a private function to the
EAL, as it is currently only used within the rte_devargs parsing.
Applications could use this helper to recognize a bus from a device
name, but this is contrived.
> > The EAL has no way to know this currently. As you noted, it has to know
> > onto which bus a device belongs before plugging it.
> >
> >> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> >> > ---
> >> > lib/librte_eal/bsdapp/eal/rte_eal_version.map | 1 +
> >> > lib/librte_eal/common/eal_common_bus.c | 15 +++++++++++++++
> >> > lib/librte_eal/common/include/rte_bus.h | 12 ++++++++++++
> >> > lib/librte_eal/linuxapp/eal/rte_eal_version.map | 1 +
> >> > 4 files changed, 29 insertions(+)
> >> >
> >> > diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> >> > index 3517d74..04fa882 100644
> >> > --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> >> > +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> >> > @@ -202,5 +202,6 @@ DPDK_17.08 {
> >> > global:
> >> >
> >> > rte_bus_from_name;
> >> > + rte_bus_from_dev;
> >> >
> >> > } DPDK_17.05;
> >> > diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
> >> > index 7977190..08fff60 100644
> >> > --- a/lib/librte_eal/common/eal_common_bus.c
> >> > +++ b/lib/librte_eal/common/eal_common_bus.c
> >> > @@ -242,3 +242,18 @@ rte_bus_from_name(const char *str)
> >> > return NULL;
> >> > return rte_bus_find(bus_cmp_name, str);
> >> > }
> >> > +
> >> > +static int
> >> > +bus_can_parse(const struct rte_bus *bus, const void *_name)
> >> > +{
> >> > + const char *name = _name;
> >> > +
> >> > + return (bus->parse && !bus->parse(name, NULL));
> >> > +}
> >> > +
> >> > +/* find a bus capable of parsing a device description */
> >> > +struct rte_bus *
> >> > +rte_bus_from_dev(const char *str)
> >> > +{
> >> > + return rte_bus_find(bus_can_parse, str);
> >> > +}
> >> > diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
> >> > index 5b87ac4..0b48e66 100644
> >> > --- a/lib/librte_eal/common/include/rte_bus.h
> >> > +++ b/lib/librte_eal/common/include/rte_bus.h
> >> > @@ -251,6 +251,18 @@ struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);
> >> > struct rte_bus *rte_bus_from_name(const char *str);
> >> >
> >> > /**
> >> > + * Find a bus capable of identifying a device.
> >> > + *
> >> > + * @param str
> >> > + * A device identifier (PCI address, virtual PMD name, ...).
> >> > + *
> >> > + * @return
> >> > + * A valid bus structure if found.
> >> > + * NULL if no bus is able to parse this device.
> >> > + */
> >> > +struct rte_bus *rte_bus_from_dev(const char *str);
> >> > +
> >> > +/**
> >> > * Helper for Bus registration.
> >> > * The constructor has higher priority than PMD constructors.
> >> > */
> >> > diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> >> > index 6607acc..a5127d6 100644
> >> > --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> >> > +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> >> > @@ -206,5 +206,6 @@ DPDK_17.08 {
> >> > global:
> >> >
> >> > rte_bus_from_name;
> >> > + rte_bus_from_dev;
> >> >
> >> > } DPDK_17.05;
> >> > --
> >> > 2.1.4
> >> >
> >
> > --
> > Gaėtan Rivet
> > 6WIND
On Thu, Jun 8, 2017 at 1:36 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> On Thu, Jun 08, 2017 at 12:45:17PM +0200, Jan Blunck wrote:
>> On Wed, Jun 7, 2017 at 10:03 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
>> > On Wed, Jun 07, 2017 at 07:28:07PM +0200, Jan Blunck wrote:
>> >> On Wed, May 24, 2017 at 5:12 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
>> >> > Find which bus should be able to parse this device name into an internal
>> >> > device representation.
>> >> >
>> >>
>> >> No, please don't add this. One should know to what bus a device
>> >> belongs to before plugging it. Artificially encoding the parent bus
>> >> into the device name is not the right thing to do. Please keep those
>> >> things separate.
>> >>
>> >
>>
>> When plugging a device the users know about:
>> - bus name
>> - device name
>>
>> Its not the case that the users invent the device names out of thin
>> air. The EAL shouldn't codify what the users of the EAL already know
>> about.
>>
>>
>
> Yes, but in that case the user is forced to explicitly name the bus used
> for a device.
>
> I think it might be sufficient to have this as a private function to the
> EAL, as it is currently only used within the rte_devargs parsing.
> Applications could use this helper to recognize a bus from a device
> name, but this is contrived.
>
Just remove it. Putting the knowledge of what bus a device name could
be for into code has failed before (e.g. biosdevname etc.). If the
application doesn't know what bus the device is living on we have a
different problem.
>> > The EAL has no way to know this currently. As you noted, it has to know
>> > onto which bus a device belongs before plugging it.
>> >
>> >> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
>> >> > ---
>> >> > lib/librte_eal/bsdapp/eal/rte_eal_version.map | 1 +
>> >> > lib/librte_eal/common/eal_common_bus.c | 15 +++++++++++++++
>> >> > lib/librte_eal/common/include/rte_bus.h | 12 ++++++++++++
>> >> > lib/librte_eal/linuxapp/eal/rte_eal_version.map | 1 +
>> >> > 4 files changed, 29 insertions(+)
>> >> >
>> >> > diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
>> >> > index 3517d74..04fa882 100644
>> >> > --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
>> >> > +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
>> >> > @@ -202,5 +202,6 @@ DPDK_17.08 {
>> >> > global:
>> >> >
>> >> > rte_bus_from_name;
>> >> > + rte_bus_from_dev;
>> >> >
>> >> > } DPDK_17.05;
>> >> > diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
>> >> > index 7977190..08fff60 100644
>> >> > --- a/lib/librte_eal/common/eal_common_bus.c
>> >> > +++ b/lib/librte_eal/common/eal_common_bus.c
>> >> > @@ -242,3 +242,18 @@ rte_bus_from_name(const char *str)
>> >> > return NULL;
>> >> > return rte_bus_find(bus_cmp_name, str);
>> >> > }
>> >> > +
>> >> > +static int
>> >> > +bus_can_parse(const struct rte_bus *bus, const void *_name)
>> >> > +{
>> >> > + const char *name = _name;
>> >> > +
>> >> > + return (bus->parse && !bus->parse(name, NULL));
>> >> > +}
>> >> > +
>> >> > +/* find a bus capable of parsing a device description */
>> >> > +struct rte_bus *
>> >> > +rte_bus_from_dev(const char *str)
>> >> > +{
>> >> > + return rte_bus_find(bus_can_parse, str);
>> >> > +}
>> >> > diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
>> >> > index 5b87ac4..0b48e66 100644
>> >> > --- a/lib/librte_eal/common/include/rte_bus.h
>> >> > +++ b/lib/librte_eal/common/include/rte_bus.h
>> >> > @@ -251,6 +251,18 @@ struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);
>> >> > struct rte_bus *rte_bus_from_name(const char *str);
>> >> >
>> >> > /**
>> >> > + * Find a bus capable of identifying a device.
>> >> > + *
>> >> > + * @param str
>> >> > + * A device identifier (PCI address, virtual PMD name, ...).
>> >> > + *
>> >> > + * @return
>> >> > + * A valid bus structure if found.
>> >> > + * NULL if no bus is able to parse this device.
>> >> > + */
>> >> > +struct rte_bus *rte_bus_from_dev(const char *str);
>> >> > +
>> >> > +/**
>> >> > * Helper for Bus registration.
>> >> > * The constructor has higher priority than PMD constructors.
>> >> > */
>> >> > diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
>> >> > index 6607acc..a5127d6 100644
>> >> > --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
>> >> > +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
>> >> > @@ -206,5 +206,6 @@ DPDK_17.08 {
>> >> > global:
>> >> >
>> >> > rte_bus_from_name;
>> >> > + rte_bus_from_dev;
>> >> >
>> >> > } DPDK_17.05;
>> >> > --
>> >> > 2.1.4
>> >> >
>> >
>> > --
>> > Gaėtan Rivet
>> > 6WIND
>
> --
> Gaëtan Rivet
> 6WIND
On Thu, Jun 08, 2017 at 01:40:46PM +0200, Jan Blunck wrote:
> On Thu, Jun 8, 2017 at 1:36 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> > On Thu, Jun 08, 2017 at 12:45:17PM +0200, Jan Blunck wrote:
> >> On Wed, Jun 7, 2017 at 10:03 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> >> > On Wed, Jun 07, 2017 at 07:28:07PM +0200, Jan Blunck wrote:
> >> >> On Wed, May 24, 2017 at 5:12 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> >> >> > Find which bus should be able to parse this device name into an internal
> >> >> > device representation.
> >> >> >
> >> >>
> >> >> No, please don't add this. One should know to what bus a device
> >> >> belongs to before plugging it. Artificially encoding the parent bus
> >> >> into the device name is not the right thing to do. Please keep those
> >> >> things separate.
> >> >>
> >> >
> >>
> >> When plugging a device the users know about:
> >> - bus name
> >> - device name
> >>
> >> Its not the case that the users invent the device names out of thin
> >> air. The EAL shouldn't codify what the users of the EAL already know
> >> about.
> >>
> >>
> >
> > Yes, but in that case the user is forced to explicitly name the bus used
> > for a device.
> >
> > I think it might be sufficient to have this as a private function to the
> > EAL, as it is currently only used within the rte_devargs parsing.
> > Applications could use this helper to recognize a bus from a device
> > name, but this is contrived.
> >
>
> Just remove it. Putting the knowledge of what bus a device name could
> be for into code has failed before (e.g. biosdevname etc.). If the
> application doesn't know what bus the device is living on we have a
> different problem.
>
This means that devices will be declared as follows:
-w PCI:00:02.0 -w virtual:net_ring0
Without a way to keep the legacy behavior.
Do you agree with it?
> >> > The EAL has no way to know this currently. As you noted, it has to know
> >> > onto which bus a device belongs before plugging it.
> >> >
> >> >> > Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> >> >> > ---
> >> >> > lib/librte_eal/bsdapp/eal/rte_eal_version.map | 1 +
> >> >> > lib/librte_eal/common/eal_common_bus.c | 15 +++++++++++++++
> >> >> > lib/librte_eal/common/include/rte_bus.h | 12 ++++++++++++
> >> >> > lib/librte_eal/linuxapp/eal/rte_eal_version.map | 1 +
> >> >> > 4 files changed, 29 insertions(+)
> >> >> >
> >> >> > diff --git a/lib/librte_eal/bsdapp/eal/rte_eal_version.map b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> >> >> > index 3517d74..04fa882 100644
> >> >> > --- a/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> >> >> > +++ b/lib/librte_eal/bsdapp/eal/rte_eal_version.map
> >> >> > @@ -202,5 +202,6 @@ DPDK_17.08 {
> >> >> > global:
> >> >> >
> >> >> > rte_bus_from_name;
> >> >> > + rte_bus_from_dev;
> >> >> >
> >> >> > } DPDK_17.05;
> >> >> > diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
> >> >> > index 7977190..08fff60 100644
> >> >> > --- a/lib/librte_eal/common/eal_common_bus.c
> >> >> > +++ b/lib/librte_eal/common/eal_common_bus.c
> >> >> > @@ -242,3 +242,18 @@ rte_bus_from_name(const char *str)
> >> >> > return NULL;
> >> >> > return rte_bus_find(bus_cmp_name, str);
> >> >> > }
> >> >> > +
> >> >> > +static int
> >> >> > +bus_can_parse(const struct rte_bus *bus, const void *_name)
> >> >> > +{
> >> >> > + const char *name = _name;
> >> >> > +
> >> >> > + return (bus->parse && !bus->parse(name, NULL));
> >> >> > +}
> >> >> > +
> >> >> > +/* find a bus capable of parsing a device description */
> >> >> > +struct rte_bus *
> >> >> > +rte_bus_from_dev(const char *str)
> >> >> > +{
> >> >> > + return rte_bus_find(bus_can_parse, str);
> >> >> > +}
> >> >> > diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
> >> >> > index 5b87ac4..0b48e66 100644
> >> >> > --- a/lib/librte_eal/common/include/rte_bus.h
> >> >> > +++ b/lib/librte_eal/common/include/rte_bus.h
> >> >> > @@ -251,6 +251,18 @@ struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);
> >> >> > struct rte_bus *rte_bus_from_name(const char *str);
> >> >> >
> >> >> > /**
> >> >> > + * Find a bus capable of identifying a device.
> >> >> > + *
> >> >> > + * @param str
> >> >> > + * A device identifier (PCI address, virtual PMD name, ...).
> >> >> > + *
> >> >> > + * @return
> >> >> > + * A valid bus structure if found.
> >> >> > + * NULL if no bus is able to parse this device.
> >> >> > + */
> >> >> > +struct rte_bus *rte_bus_from_dev(const char *str);
> >> >> > +
> >> >> > +/**
> >> >> > * Helper for Bus registration.
> >> >> > * The constructor has higher priority than PMD constructors.
> >> >> > */
> >> >> > diff --git a/lib/librte_eal/linuxapp/eal/rte_eal_version.map b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> >> >> > index 6607acc..a5127d6 100644
> >> >> > --- a/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> >> >> > +++ b/lib/librte_eal/linuxapp/eal/rte_eal_version.map
> >> >> > @@ -206,5 +206,6 @@ DPDK_17.08 {
> >> >> > global:
> >> >> >
> >> >> > rte_bus_from_name;
> >> >> > + rte_bus_from_dev;
> >> >> >
> >> >> > } DPDK_17.05;
> >> >> > --
> >> >> > 2.1.4
> >> >> >
> >> >
> >> > --
> >> > Gaėtan Rivet
> >> > 6WIND
> >
> > --
> > Gaëtan Rivet
> > 6WIND
On Thu, Jun 8, 2017 at 2:51 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> On Thu, Jun 08, 2017 at 01:40:46PM +0200, Jan Blunck wrote:
>> On Thu, Jun 8, 2017 at 1:36 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
>> > On Thu, Jun 08, 2017 at 12:45:17PM +0200, Jan Blunck wrote:
>> >> On Wed, Jun 7, 2017 at 10:03 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
>> >> > On Wed, Jun 07, 2017 at 07:28:07PM +0200, Jan Blunck wrote:
>> >> >> On Wed, May 24, 2017 at 5:12 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
>> >> >> > Find which bus should be able to parse this device name into an internal
>> >> >> > device representation.
>> >> >> >
>> >> >>
>> >> >> No, please don't add this. One should know to what bus a device
>> >> >> belongs to before plugging it. Artificially encoding the parent bus
>> >> >> into the device name is not the right thing to do. Please keep those
>> >> >> things separate.
>> >> >>
>> >> >
>> >>
>> >> When plugging a device the users know about:
>> >> - bus name
>> >> - device name
>> >>
>> >> Its not the case that the users invent the device names out of thin
>> >> air. The EAL shouldn't codify what the users of the EAL already know
>> >> about.
>> >>
>> >>
>> >
>> > Yes, but in that case the user is forced to explicitly name the bus used
>> > for a device.
>> >
>> > I think it might be sufficient to have this as a private function to the
>> > EAL, as it is currently only used within the rte_devargs parsing.
>> > Applications could use this helper to recognize a bus from a device
>> > name, but this is contrived.
>> >
>>
>> Just remove it. Putting the knowledge of what bus a device name could
>> be for into code has failed before (e.g. biosdevname etc.). If the
>> application doesn't know what bus the device is living on we have a
>> different problem.
>>
>
> This means that devices will be declared as follows:
>
> -w PCI:00:02.0 -w virtual:net_ring0
>
> Without a way to keep the legacy behavior.
> Do you agree with it?
>
This adds ':' as another character not usable in identifiers for buses.
I wouldn't touch the meaning of -w at all and instead use it as an alias for
--dev 00:02.0,bus=pci --bus pci,whitelist
This would clearly separate the device arguments from the bus arguments.
What do you think? Does this make sense?
On Sat, Jun 10, 2017 at 10:50:12AM +0200, Jan Blunck wrote:
> On Thu, Jun 8, 2017 at 2:51 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> > On Thu, Jun 08, 2017 at 01:40:46PM +0200, Jan Blunck wrote:
> >> On Thu, Jun 8, 2017 at 1:36 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> >> > On Thu, Jun 08, 2017 at 12:45:17PM +0200, Jan Blunck wrote:
> >> >> On Wed, Jun 7, 2017 at 10:03 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> >> >> > On Wed, Jun 07, 2017 at 07:28:07PM +0200, Jan Blunck wrote:
> >> >> >> On Wed, May 24, 2017 at 5:12 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> >> >> >> > Find which bus should be able to parse this device name into an internal
> >> >> >> > device representation.
> >> >> >> >
> >> >> >>
> >> >> >> No, please don't add this. One should know to what bus a device
> >> >> >> belongs to before plugging it. Artificially encoding the parent bus
> >> >> >> into the device name is not the right thing to do. Please keep those
> >> >> >> things separate.
> >> >> >>
> >> >> >
> >> >>
> >> >> When plugging a device the users know about:
> >> >> - bus name
> >> >> - device name
> >> >>
> >> >> Its not the case that the users invent the device names out of thin
> >> >> air. The EAL shouldn't codify what the users of the EAL already know
> >> >> about.
> >> >>
> >> >>
> >> >
> >> > Yes, but in that case the user is forced to explicitly name the bus used
> >> > for a device.
> >> >
> >> > I think it might be sufficient to have this as a private function to the
> >> > EAL, as it is currently only used within the rte_devargs parsing.
> >> > Applications could use this helper to recognize a bus from a device
> >> > name, but this is contrived.
> >> >
> >>
> >> Just remove it. Putting the knowledge of what bus a device name could
> >> be for into code has failed before (e.g. biosdevname etc.). If the
> >> application doesn't know what bus the device is living on we have a
> >> different problem.
> >>
> >
> > This means that devices will be declared as follows:
> >
> > -w PCI:00:02.0 -w virtual:net_ring0
> >
> > Without a way to keep the legacy behavior.
> > Do you agree with it?
> >
>
> This adds ':' as another character not usable in identifiers for buses.
>
I understand your issue with this.
However, we are not forced to define a separating character that would thus
be forbidden within bus names.
We can strncmp() the name of the device, limited to strlen(bus->name).
If no bus name could be read this way, then we are sure to have a device
name (if we want to keep backward compability) or to throw an error if
we want to force defining the bus explicitly.
My current approach has been to define what is allowed within a bus name
and to limit the name comparison to the valid characters.
This allowed to use any illegal character as separator, but I think it
was a mistake. This is not a good, stable API for the future and does
not give clear rules to bus writers as to what is, or will be allowed
within a bus name.
But the strncmp approach allows the use of any separating character,
legal or illegal within a bus name. Additionally, it's then possible not
to add a magic devargs that must be parsed prior to device probing.
This last point is important I think. I am strongly opposed to having
parsing done upon the devargs, and to have any special logic added to
the rte_devargs constructor. As you saw with the driver= special
parameter, this led you to either add rte_kvargs as a dependency or roll
out your own parser. This was an undocumented hack for the bonding PMD test,
and personally I do not support this kind of addition to the public
rte_bus API.
> I wouldn't touch the meaning of -w at all and instead use it as an alias for
>
> --dev 00:02.0,bus=pci --bus pci,whitelist
>
I agree that we should add a --bus parameter or similar to offer bus
configuration.
I think that in the end, -w, -b and --vdev should disappear to only
leave a common --dev parameter, capable of handling all devices.
It may be possible to have a special rule for -w, -b for some time, that
would either prepend "PCI:" or append ",bus=PCI" to any device
definition, but this is hacky as it relies on PCI_BUS_NAME without
being able to include its definition.
But considering the possibility to declare a device without explicitly
naming its bus: if we go in this direction, then we already have a
public API that can be leveraged for this. The implementation is
simple and can be made private to the EAL if exposing it is a problem.
Additionally it does not inject implicit bus dependencies within the
EAL.
> This would clearly separate the device arguments from the bus arguments.
>
> What do you think? Does this make sense?
@@ -202,5 +202,6 @@ DPDK_17.08 {
global:
rte_bus_from_name;
+ rte_bus_from_dev;
} DPDK_17.05;
@@ -242,3 +242,18 @@ rte_bus_from_name(const char *str)
return NULL;
return rte_bus_find(bus_cmp_name, str);
}
+
+static int
+bus_can_parse(const struct rte_bus *bus, const void *_name)
+{
+ const char *name = _name;
+
+ return (bus->parse && !bus->parse(name, NULL));
+}
+
+/* find a bus capable of parsing a device description */
+struct rte_bus *
+rte_bus_from_dev(const char *str)
+{
+ return rte_bus_find(bus_can_parse, str);
+}
@@ -251,6 +251,18 @@ struct rte_bus *rte_bus_find_by_device(const struct rte_device *dev);
struct rte_bus *rte_bus_from_name(const char *str);
/**
+ * Find a bus capable of identifying a device.
+ *
+ * @param str
+ * A device identifier (PCI address, virtual PMD name, ...).
+ *
+ * @return
+ * A valid bus structure if found.
+ * NULL if no bus is able to parse this device.
+ */
+struct rte_bus *rte_bus_from_dev(const char *str);
+
+/**
* Helper for Bus registration.
* The constructor has higher priority than PMD constructors.
*/
@@ -206,5 +206,6 @@ DPDK_17.08 {
global:
rte_bus_from_name;
+ rte_bus_from_dev;
} DPDK_17.05;