[dpdk-dev,v6,05/11] bus: introduce hotplug functionality

  From: Jan Blunck <jblunck@infradead.org>

Signed-off-by: Jan Blunck <jblunck@infradead.org>
Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
 lib/librte_eal/common/eal_common_bus.c  |  2 ++
 lib/librte_eal/common/include/rte_bus.h | 31 +++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)
  

On Tue, Jun 27, 2017 at 6:11 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> From: Jan Blunck <jblunck@infradead.org>
>
> Signed-off-by: Jan Blunck <jblunck@infradead.org>
> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> ---
>  lib/librte_eal/common/eal_common_bus.c  |  2 ++
>  lib/librte_eal/common/include/rte_bus.h | 31 +++++++++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/lib/librte_eal/common/eal_common_bus.c b/lib/librte_eal/common/eal_common_bus.c
> index dde6c83..83d9c07 100644
> --- a/lib/librte_eal/common/eal_common_bus.c
> +++ b/lib/librte_eal/common/eal_common_bus.c
> @@ -50,6 +50,8 @@ rte_bus_register(struct rte_bus *bus)
>         /* A bus should mandatorily have the scan implemented */
>         RTE_VERIFY(bus->scan);
>         RTE_VERIFY(bus->probe);
> +       /* Buses supporting hotplug also require unplug. */
> +       RTE_VERIFY(!bus->plug || bus->unplug);
>
>         TAILQ_INSERT_TAIL(&rte_bus_list, bus, next);
>         RTE_LOG(DEBUG, EAL, "Registered [%s] bus.\n", bus->name);
> diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h
> index 9aa047d..8c2d19c 100644
> --- a/lib/librte_eal/common/include/rte_bus.h
> +++ b/lib/librte_eal/common/include/rte_bus.h
> @@ -109,6 +109,35 @@ typedef struct rte_device *
>                          const struct rte_device *start);
>
>  /**
> + * Implementation specific probe function which is responsible for linking
> + * devices on that bus with applicable drivers.
> + * The plugged device might already have been used previously by the bus,
> + * in which case some buses might prefer to detect and re-use the relevant
> + * information pertaining to this device.
> + *
> + * @param da
> + *     Device declaration.
> + *
> + * @return
> + *     The pointer to a valid rte_device usable by the bus on success.
> + *     NULL on error. rte_errno is then set.
> + */
> +typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);

Shouldn't this be orthogonal to unplug() and take a rte_device. You
should only be able to plug devices that have been found by scan
before.

Jan

> +
> +/**
> + * Implementation specific remove function which is responsible for unlinking
> + * devices on that bus from assigned driver.
> + *
> + * @param dev
> + *     Device pointer that was returned by a previous device plug call.
> + *
> + * @return
> + *     0 on success.
> + *     !0 on error. rte_errno is then set.
> + */
> +typedef int (*rte_bus_unplug_t)(struct rte_device *dev);
> +
> +/**
>   * A structure describing a generic bus.
>   */
>  struct rte_bus {
> @@ -117,6 +146,8 @@ struct rte_bus {
>         rte_bus_scan_t scan;         /**< Scan for devices attached to bus */
>         rte_bus_probe_t probe;       /**< Probe devices on bus */
>         rte_bus_find_device_t find_device; /**< Find a device on the bus */
> +       rte_bus_plug_t plug;         /**< Probe single device for drivers */
> +       rte_bus_unplug_t unplug;     /**< Remove single device from driver */
>  };
>
>  /**
> --
> 2.1.4
>
  

27/06/2017 21:03, Jan Blunck:
> On Tue, Jun 27, 2017 at 6:11 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> > --- a/lib/librte_eal/common/include/rte_bus.h
> > +++ b/lib/librte_eal/common/include/rte_bus.h
> >  /**
> > + * Implementation specific probe function which is responsible for linking
> > + * devices on that bus with applicable drivers.
> > + * The plugged device might already have been used previously by the bus,
> > + * in which case some buses might prefer to detect and re-use the relevant
> > + * information pertaining to this device.
> > + *
> > + * @param da
> > + *     Device declaration.
> > + *
> > + * @return
> > + *     The pointer to a valid rte_device usable by the bus on success.
> > + *     NULL on error. rte_errno is then set.
> > + */
> > +typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
> 
> Shouldn't this be orthogonal to unplug() and take a rte_device. You
> should only be able to plug devices that have been found by scan
> before.

Plugging a device that has been scanned before is a special case.
In a true hotplug scenario, we could use this plug function passing
a devargs.
I don't see any issue passing rte_devargs to plug and rte_device to unplug.
  

On Wed, Jun 28, 2017 at 1:44 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> 27/06/2017 21:03, Jan Blunck:
>> On Tue, Jun 27, 2017 at 6:11 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
>> > --- a/lib/librte_eal/common/include/rte_bus.h
>> > +++ b/lib/librte_eal/common/include/rte_bus.h
>> >  /**
>> > + * Implementation specific probe function which is responsible for linking
>> > + * devices on that bus with applicable drivers.
>> > + * The plugged device might already have been used previously by the bus,
>> > + * in which case some buses might prefer to detect and re-use the relevant
>> > + * information pertaining to this device.
>> > + *
>> > + * @param da
>> > + *     Device declaration.
>> > + *
>> > + * @return
>> > + *     The pointer to a valid rte_device usable by the bus on success.
>> > + *     NULL on error. rte_errno is then set.
>> > + */
>> > +typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
>>
>> Shouldn't this be orthogonal to unplug() and take a rte_device. You
>> should only be able to plug devices that have been found by scan
>> before.
>
> Plugging a device that has been scanned before is a special case.
> In a true hotplug scenario, we could use this plug function passing
> a devargs.
> I don't see any issue passing rte_devargs to plug and rte_device to unplug.

What do you mean by "true hotplug"?

The problem with this is that passing just rte_devargs to plug()
requires the bus to parse and enrich the rte_devargs with bus
specifics. From there it gets folded into the to-be-created bus
specific rte_XXX_device. This makes it unnecessarily complicated and
even worse it adds a second code path how devices come alive.

When we get notified about a hotplug event we already know which bus
this event belongs to:

1. scan the bus for incoming devices
2. plug single device with devargs and probe for drivers

Makes sense?
  

28/06/2017 13:58, Jan Blunck:
> On Wed, Jun 28, 2017 at 1:44 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> > 27/06/2017 21:03, Jan Blunck:
> >> On Tue, Jun 27, 2017 at 6:11 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> >> > --- a/lib/librte_eal/common/include/rte_bus.h
> >> > +++ b/lib/librte_eal/common/include/rte_bus.h
> >> >  /**
> >> > + * Implementation specific probe function which is responsible for linking
> >> > + * devices on that bus with applicable drivers.
> >> > + * The plugged device might already have been used previously by the bus,
> >> > + * in which case some buses might prefer to detect and re-use the relevant
> >> > + * information pertaining to this device.
> >> > + *
> >> > + * @param da
> >> > + *     Device declaration.
> >> > + *
> >> > + * @return
> >> > + *     The pointer to a valid rte_device usable by the bus on success.
> >> > + *     NULL on error. rte_errno is then set.
> >> > + */
> >> > +typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
> >>
> >> Shouldn't this be orthogonal to unplug() and take a rte_device. You
> >> should only be able to plug devices that have been found by scan
> >> before.
> >
> > Plugging a device that has been scanned before is a special case.
> > In a true hotplug scenario, we could use this plug function passing
> > a devargs.
> > I don't see any issue passing rte_devargs to plug and rte_device to unplug.
> 
> What do you mean by "true hotplug"?

I mean a kernel notification of a new device.

> The problem with this is that passing just rte_devargs to plug()
> requires the bus to parse and enrich the rte_devargs with bus
> specifics. From there it gets folded into the to-be-created bus
> specific rte_XXX_device. This makes it unnecessarily complicated and
> even worse it adds a second code path how devices come alive.

Just after the notification, the rte_device does not exist yet.
So the plug function could share the same code as the scan function
to get the metadata and create the device instance.

> When we get notified about a hotplug event we already know which bus
> this event belongs to:
> 
> 1. scan the bus for incoming devices

No need to scan every devices here.

> 2. plug single device with devargs and probe for drivers
> 
> Makes sense?

I want to make sure there is no misunderstanding first :)
  

On Wed, Jun 28, 2017 at 2:11 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> 28/06/2017 13:58, Jan Blunck:
>> On Wed, Jun 28, 2017 at 1:44 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
>> > 27/06/2017 21:03, Jan Blunck:
>> >> On Tue, Jun 27, 2017 at 6:11 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
>> >> > --- a/lib/librte_eal/common/include/rte_bus.h
>> >> > +++ b/lib/librte_eal/common/include/rte_bus.h
>> >> >  /**
>> >> > + * Implementation specific probe function which is responsible for linking
>> >> > + * devices on that bus with applicable drivers.
>> >> > + * The plugged device might already have been used previously by the bus,
>> >> > + * in which case some buses might prefer to detect and re-use the relevant
>> >> > + * information pertaining to this device.
>> >> > + *
>> >> > + * @param da
>> >> > + *     Device declaration.
>> >> > + *
>> >> > + * @return
>> >> > + *     The pointer to a valid rte_device usable by the bus on success.
>> >> > + *     NULL on error. rte_errno is then set.
>> >> > + */
>> >> > +typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
>> >>
>> >> Shouldn't this be orthogonal to unplug() and take a rte_device. You
>> >> should only be able to plug devices that have been found by scan
>> >> before.
>> >
>> > Plugging a device that has been scanned before is a special case.
>> > In a true hotplug scenario, we could use this plug function passing
>> > a devargs.
>> > I don't see any issue passing rte_devargs to plug and rte_device to unplug.
>>
>> What do you mean by "true hotplug"?
>
> I mean a kernel notification of a new device.
>

Does a "false hotplug" exist, too? :)

>> The problem with this is that passing just rte_devargs to plug()
>> requires the bus to parse and enrich the rte_devargs with bus
>> specifics. From there it gets folded into the to-be-created bus
>> specific rte_XXX_device. This makes it unnecessarily complicated and
>> even worse it adds a second code path how devices come alive.
>
> Just after the notification, the rte_device does not exist yet.
> So the plug function could share the same code as the scan function
> to get the metadata and create the device instance.
>

Exactly this is what I want to avoid. The plug() function would become
a "scan-one and probe". From my point of view plug() and unplug()
should be orthogonal. The plug() and unplug() should only be
responsible for adding drivers with optional arguments. The EAL should
allow the drivers to get unplugged/re-plugged at run-time. I want to
be able to change arguments ... or even drivers :)

>> When we get notified about a hotplug event we already know which bus
>> this event belongs to:
>>
>> 1. scan the bus for incoming devices
>
> No need to scan every devices here.
>

This is a readdir followed by open+read+close for any new device. This
code belongs here anyway. Its lightweight if nothing changed. The scan
itself should be idempotent anyway.

>> 2. plug single device with devargs and probe for drivers
>>
>> Makes sense?
>
> I want to make sure there is no misunderstanding first :)

Which makes sense. That is probably my fault due to being too
distracted with other things and not communicating well enough while
Gaetan consumed my code.
  

28/06/2017 15:09, Jan Blunck:
> On Wed, Jun 28, 2017 at 2:11 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> > 28/06/2017 13:58, Jan Blunck:
> >> On Wed, Jun 28, 2017 at 1:44 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> >> > 27/06/2017 21:03, Jan Blunck:
> >> >> On Tue, Jun 27, 2017 at 6:11 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> >> >> > --- a/lib/librte_eal/common/include/rte_bus.h
> >> >> > +++ b/lib/librte_eal/common/include/rte_bus.h
> >> >> >  /**
> >> >> > + * Implementation specific probe function which is responsible for linking
> >> >> > + * devices on that bus with applicable drivers.
> >> >> > + * The plugged device might already have been used previously by the bus,
> >> >> > + * in which case some buses might prefer to detect and re-use the relevant
> >> >> > + * information pertaining to this device.
> >> >> > + *
> >> >> > + * @param da
> >> >> > + *     Device declaration.
> >> >> > + *
> >> >> > + * @return
> >> >> > + *     The pointer to a valid rte_device usable by the bus on success.
> >> >> > + *     NULL on error. rte_errno is then set.
> >> >> > + */
> >> >> > +typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
> >> >>
> >> >> Shouldn't this be orthogonal to unplug() and take a rte_device. You
> >> >> should only be able to plug devices that have been found by scan
> >> >> before.
> >> >
> >> > Plugging a device that has been scanned before is a special case.
> >> > In a true hotplug scenario, we could use this plug function passing
> >> > a devargs.
> >> > I don't see any issue passing rte_devargs to plug and rte_device to unplug.
> >>
> >> What do you mean by "true hotplug"?
> >
> > I mean a kernel notification of a new device.
> 
> Does a "false hotplug" exist, too? :)

The false hotplug was the original attach function which was just
adding a new ethdev interface.

> >> The problem with this is that passing just rte_devargs to plug()
> >> requires the bus to parse and enrich the rte_devargs with bus
> >> specifics. From there it gets folded into the to-be-created bus
> >> specific rte_XXX_device. This makes it unnecessarily complicated and
> >> even worse it adds a second code path how devices come alive.
> >
> > Just after the notification, the rte_device does not exist yet.
> > So the plug function could share the same code as the scan function
> > to get the metadata and create the device instance.
> 
> Exactly this is what I want to avoid. 

Why do you want to avoid that?
I think you mean it is not what you had in mind.

> The plug() function would become a "scan-one and probe".

Yes

> From my point of view plug() and unplug() should be orthogonal.
> The plug() and unplug() should only be responsible for adding drivers
> with optional arguments. The EAL should allow the drivers to get
> unplugged/re-plugged at run-time. I want to be able to change arguments
> ... or even drivers :)

It is a totally different thing.
We are talking about hotplug of a device,
and you are talking about changing drivers dynamically.

So I still don't understand what is the issue with the plug/unplug
functions proposed here.

> >> When we get notified about a hotplug event we already know which bus
> >> this event belongs to:
> >>
> >> 1. scan the bus for incoming devices
> >
> > No need to scan every devices here.
> 
> This is a readdir followed by open+read+close for any new device. This
> code belongs here anyway. Its lightweight if nothing changed. The scan
> itself should be idempotent anyway.
> 
> >> 2. plug single device with devargs and probe for drivers
> >>
> >> Makes sense?
> >
> > I want to make sure there is no misunderstanding first :)
> 
> Which makes sense. That is probably my fault due to being too
> distracted with other things and not communicating well enough while
> Gaetan consumed my code.

Your ideas are probably interesting, and I want to understand them.
In the meantime, we need to progress on 17.08-rc1 which must be done
in following days. Please let's separate the ideas which are not
yet implemented from what we are already able to deliver.
  

On Wed, Jun 28, 2017 at 3:30 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> 28/06/2017 15:09, Jan Blunck:
>> On Wed, Jun 28, 2017 at 2:11 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
>> > 28/06/2017 13:58, Jan Blunck:
>> >> On Wed, Jun 28, 2017 at 1:44 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
>> >> > 27/06/2017 21:03, Jan Blunck:
>> >> >> On Tue, Jun 27, 2017 at 6:11 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
>> >> >> > --- a/lib/librte_eal/common/include/rte_bus.h
>> >> >> > +++ b/lib/librte_eal/common/include/rte_bus.h
>> >> >> >  /**
>> >> >> > + * Implementation specific probe function which is responsible for linking
>> >> >> > + * devices on that bus with applicable drivers.
>> >> >> > + * The plugged device might already have been used previously by the bus,
>> >> >> > + * in which case some buses might prefer to detect and re-use the relevant
>> >> >> > + * information pertaining to this device.
>> >> >> > + *
>> >> >> > + * @param da
>> >> >> > + *     Device declaration.
>> >> >> > + *
>> >> >> > + * @return
>> >> >> > + *     The pointer to a valid rte_device usable by the bus on success.
>> >> >> > + *     NULL on error. rte_errno is then set.
>> >> >> > + */
>> >> >> > +typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
>> >> >>
>> >> >> Shouldn't this be orthogonal to unplug() and take a rte_device. You
>> >> >> should only be able to plug devices that have been found by scan
>> >> >> before.
>> >> >
>> >> > Plugging a device that has been scanned before is a special case.
>> >> > In a true hotplug scenario, we could use this plug function passing
>> >> > a devargs.
>> >> > I don't see any issue passing rte_devargs to plug and rte_device to unplug.
>> >>
>> >> What do you mean by "true hotplug"?
>> >
>> > I mean a kernel notification of a new device.
>>
>> Does a "false hotplug" exist, too? :)
>
> The false hotplug was the original attach function which was just
> adding a new ethdev interface.
>
>> >> The problem with this is that passing just rte_devargs to plug()
>> >> requires the bus to parse and enrich the rte_devargs with bus
>> >> specifics. From there it gets folded into the to-be-created bus
>> >> specific rte_XXX_device. This makes it unnecessarily complicated and
>> >> even worse it adds a second code path how devices come alive.
>> >
>> > Just after the notification, the rte_device does not exist yet.
>> > So the plug function could share the same code as the scan function
>> > to get the metadata and create the device instance.
>>
>> Exactly this is what I want to avoid.
>
> Why do you want to avoid that?
> I think you mean it is not what you had in mind.
>
>> The plug() function would become a "scan-one and probe".
>
> Yes
>
>> From my point of view plug() and unplug() should be orthogonal.
>> The plug() and unplug() should only be responsible for adding drivers
>> with optional arguments. The EAL should allow the drivers to get
>> unplugged/re-plugged at run-time. I want to be able to change arguments
>> ... or even drivers :)
>
> It is a totally different thing.
> We are talking about hotplug of a device,
> and you are talking about changing drivers dynamically.
>
> So I still don't understand what is the issue with the plug/unplug
> functions proposed here.
>

I don't agree with the notion that plug() means "scan-one and probe".

>> >> When we get notified about a hotplug event we already know which bus
>> >> this event belongs to:
>> >>
>> >> 1. scan the bus for incoming devices
>> >
>> > No need to scan every devices here.
>>
>> This is a readdir followed by open+read+close for any new device. This
>> code belongs here anyway. Its lightweight if nothing changed. The scan
>> itself should be idempotent anyway.
>>
>> >> 2. plug single device with devargs and probe for drivers
>> >>
>> >> Makes sense?
>> >
>> > I want to make sure there is no misunderstanding first :)
>>
>> Which makes sense. That is probably my fault due to being too
>> distracted with other things and not communicating well enough while
>> Gaetan consumed my code.
>
> Your ideas are probably interesting, and I want to understand them.
> In the meantime, we need to progress on 17.08-rc1 which must be done
> in following days. Please let's separate the ideas which are not
> yet implemented from what we are already able to deliver.

Sorry, if you got the impression that this is taken from thin air but
this is how my original patch series looked like and I'm just
providing the rational behind it.
  

On Wed, Jun 28, 2017 at 05:11:46PM +0200, Jan Blunck wrote:
> On Wed, Jun 28, 2017 at 3:30 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> > 28/06/2017 15:09, Jan Blunck:
> >> On Wed, Jun 28, 2017 at 2:11 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> >> > 28/06/2017 13:58, Jan Blunck:
> >> >> On Wed, Jun 28, 2017 at 1:44 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> >> >> > 27/06/2017 21:03, Jan Blunck:
> >> >> >> On Tue, Jun 27, 2017 at 6:11 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> >> >> >> > --- a/lib/librte_eal/common/include/rte_bus.h
> >> >> >> > +++ b/lib/librte_eal/common/include/rte_bus.h
> >> >> >> >  /**
> >> >> >> > + * Implementation specific probe function which is responsible for linking
> >> >> >> > + * devices on that bus with applicable drivers.
> >> >> >> > + * The plugged device might already have been used previously by the bus,
> >> >> >> > + * in which case some buses might prefer to detect and re-use the relevant
> >> >> >> > + * information pertaining to this device.
> >> >> >> > + *
> >> >> >> > + * @param da
> >> >> >> > + *     Device declaration.
> >> >> >> > + *
> >> >> >> > + * @return
> >> >> >> > + *     The pointer to a valid rte_device usable by the bus on success.
> >> >> >> > + *     NULL on error. rte_errno is then set.
> >> >> >> > + */
> >> >> >> > +typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
> >> >> >>
> >> >> >> Shouldn't this be orthogonal to unplug() and take a rte_device. You
> >> >> >> should only be able to plug devices that have been found by scan
> >> >> >> before.
> >> >> >
> >> >> > Plugging a device that has been scanned before is a special case.
> >> >> > In a true hotplug scenario, we could use this plug function passing
> >> >> > a devargs.
> >> >> > I don't see any issue passing rte_devargs to plug and rte_device to unplug.
> >> >>
> >> >> What do you mean by "true hotplug"?
> >> >
> >> > I mean a kernel notification of a new device.
> >>
> >> Does a "false hotplug" exist, too? :)
> >
> > The false hotplug was the original attach function which was just
> > adding a new ethdev interface.
> >
> >> >> The problem with this is that passing just rte_devargs to plug()
> >> >> requires the bus to parse and enrich the rte_devargs with bus
> >> >> specifics. From there it gets folded into the to-be-created bus
> >> >> specific rte_XXX_device. This makes it unnecessarily complicated and
> >> >> even worse it adds a second code path how devices come alive.
> >> >
> >> > Just after the notification, the rte_device does not exist yet.
> >> > So the plug function could share the same code as the scan function
> >> > to get the metadata and create the device instance.
> >>
> >> Exactly this is what I want to avoid.
> >
> > Why do you want to avoid that?
> > I think you mean it is not what you had in mind.
> >
> >> The plug() function would become a "scan-one and probe".
> >
> > Yes
> >
> >> From my point of view plug() and unplug() should be orthogonal.
> >> The plug() and unplug() should only be responsible for adding drivers
> >> with optional arguments. The EAL should allow the drivers to get
> >> unplugged/re-plugged at run-time. I want to be able to change arguments
> >> ... or even drivers :)
> >
> > It is a totally different thing.
> > We are talking about hotplug of a device,
> > and you are talking about changing drivers dynamically.
> >
> > So I still don't understand what is the issue with the plug/unplug
> > functions proposed here.
> >
> 
> I don't agree with the notion that plug() means "scan-one and probe".

What do you see as plug doing then? What do you see as the use-case and
then logic flow for hotplug?
  

On Wed, Jun 28, 2017 at 04:33:20PM +0100, Bruce Richardson wrote:
> On Wed, Jun 28, 2017 at 05:11:46PM +0200, Jan Blunck wrote:
> > On Wed, Jun 28, 2017 at 3:30 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> > > 28/06/2017 15:09, Jan Blunck:
> > >> On Wed, Jun 28, 2017 at 2:11 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> > >> > 28/06/2017 13:58, Jan Blunck:
> > >> >> On Wed, Jun 28, 2017 at 1:44 PM, Thomas Monjalon <thomas@monjalon.net> wrote:
> > >> >> > 27/06/2017 21:03, Jan Blunck:
> > >> >> >> On Tue, Jun 27, 2017 at 6:11 PM, Gaetan Rivet <gaetan.rivet@6wind.com> wrote:
> > >> >> >> > --- a/lib/librte_eal/common/include/rte_bus.h
> > >> >> >> > +++ b/lib/librte_eal/common/include/rte_bus.h
> > >> >> >> >  /**
> > >> >> >> > + * Implementation specific probe function which is responsible for linking
> > >> >> >> > + * devices on that bus with applicable drivers.
> > >> >> >> > + * The plugged device might already have been used previously by the bus,
> > >> >> >> > + * in which case some buses might prefer to detect and re-use the relevant
> > >> >> >> > + * information pertaining to this device.
> > >> >> >> > + *
> > >> >> >> > + * @param da
> > >> >> >> > + *     Device declaration.
> > >> >> >> > + *
> > >> >> >> > + * @return
> > >> >> >> > + *     The pointer to a valid rte_device usable by the bus on success.
> > >> >> >> > + *     NULL on error. rte_errno is then set.
> > >> >> >> > + */
> > >> >> >> > +typedef struct rte_device * (*rte_bus_plug_t)(struct rte_devargs *da);
> > >> >> >>
> > >> >> >> Shouldn't this be orthogonal to unplug() and take a rte_device. You
> > >> >> >> should only be able to plug devices that have been found by scan
> > >> >> >> before.
> > >> >> >
> > >> >> > Plugging a device that has been scanned before is a special case.
> > >> >> > In a true hotplug scenario, we could use this plug function passing
> > >> >> > a devargs.
> > >> >> > I don't see any issue passing rte_devargs to plug and rte_device to unplug.
> > >> >>
> > >> >> What do you mean by "true hotplug"?
> > >> >
> > >> > I mean a kernel notification of a new device.
> > >>
> > >> Does a "false hotplug" exist, too? :)
> > >
> > > The false hotplug was the original attach function which was just
> > > adding a new ethdev interface.
> > >
> > >> >> The problem with this is that passing just rte_devargs to plug()
> > >> >> requires the bus to parse and enrich the rte_devargs with bus
> > >> >> specifics. From there it gets folded into the to-be-created bus
> > >> >> specific rte_XXX_device. This makes it unnecessarily complicated and
> > >> >> even worse it adds a second code path how devices come alive.
> > >> >
> > >> > Just after the notification, the rte_device does not exist yet.
> > >> > So the plug function could share the same code as the scan function
> > >> > to get the metadata and create the device instance.
> > >>
> > >> Exactly this is what I want to avoid.
> > >
> > > Why do you want to avoid that?
> > > I think you mean it is not what you had in mind.
> > >
> > >> The plug() function would become a "scan-one and probe".
> > >
> > > Yes
> > >
> > >> From my point of view plug() and unplug() should be orthogonal.
> > >> The plug() and unplug() should only be responsible for adding drivers
> > >> with optional arguments. The EAL should allow the drivers to get
> > >> unplugged/re-plugged at run-time. I want to be able to change arguments
> > >> ... or even drivers :)
> > >
> > > It is a totally different thing.
> > > We are talking about hotplug of a device,
> > > and you are talking about changing drivers dynamically.
> > >
> > > So I still don't understand what is the issue with the plug/unplug
> > > functions proposed here.
> > >
> > 
> > I don't agree with the notion that plug() means "scan-one and probe".
> 
> What do you see as plug doing then? What do you see as the use-case and
> then logic flow for hotplug?
> 

Hi all,

We are all for having "true" hotplug support in DPDK.
By true hotplug, it means that at some point, a new device exists on the
system, while the DPDK bus on which it should be probed does not yet
have its metadata. Something needs to be done to retrieve these metadata,
one way or another.

What I see as a solution to this:

- An interrupt framework integrated to rte_bus, allowing drivers to
  describe interrupt sources (Kernel UEVENT, custom FDs, ...), to their
  buses.

- Applications should be able to pilot these interrupts for rte_bus
  (either in describing expected devices, or allowing actions to be
  taken upon device removal).

- Buses could take the responsibility of plugging in and out their own
  devices, once properly configured.

In this context, it is possible to imagine triggering a bus-rescan upon
device ADD, iff we explicitly define scan() as being idempotent (a thing
that is not part of its API yet and that we cannot expect from buses at
this point).

Then, we can limit bus->plug() to a probe, once we are sure that
metadatas for the bus are (almost) always in sync with the system.

Where we are:

- Intel is proposing a UEVENT API[1]. It might be interesting to help them
  make it respect a generic framework.

- A first plug / unplug implementation is being proposed. Plug() is
  currently effectively defined as (scan-one + probe).

What can be done to go forward:

- Define the API for rte_bus interrupt sources, configuration and
  triggers to allow the development of the needed subsystems.

- Each device event sources should offer an event parser to transform
  them in device descriptions. Depending on the specifics of the source,
  different levels of info are available.

- Redefine the requirements for bus->scan() to allow hotplugging.

- Reduce the scope of plug() from (scan-one + probe) to (probe), as
  everything is now in place.

- Further hotplugging developments are then possible: add INTR_ADD
  support, with flexible device definition for example... A thing that
  is not yet possible with the current architecture but seemed to
  interest a few people.

If we can agree that this is a way forward, do you think Jan that having
the current plug() API hinders this plan? We can reduce its scope later,
without changing current implementations as, as you said, a proper
scan should be idempotent. The future API as envisionned is already
respected, but right now the hotplug support in buses is a little more
involved. Applications that will start using plug() right now would not
have to be rewritten, as the requirements for plugging would still be
fullfilled.

We can support already hotplugging in DPDK. We can refine this support
later to make it more generic and easier to implement for PMD
maintainers. But I do not see this as a reason to block this support
from being integrated right now.

[1]: http://dpdk.org/ml/archives/dev/2017-June/069057.html
  

On Thu, Jun 29, 2017 at 2:59 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
>
> Hi all,
>
> We are all for having "true" hotplug support in DPDK.
> By true hotplug, it means that at some point, a new device exists on the
> system, while the DPDK bus on which it should be probed does not yet
> have its metadata. Something needs to be done to retrieve these metadata,
> one way or another.
>
> What I see as a solution to this:
>
> - An interrupt framework integrated to rte_bus, allowing drivers to
>   describe interrupt sources (Kernel UEVENT, custom FDs, ...), to their
>   buses.
>
> - Applications should be able to pilot these interrupts for rte_bus
>   (either in describing expected devices, or allowing actions to be
>   taken upon device removal).
>
> - Buses could take the responsibility of plugging in and out their own
>   devices, once properly configured.
>

This is highly application dependent and it is up to the application
developer to decide when such events are getting processed. There is a
major difference between the data path functionality that support
interrupts and the processing of hotplug events. So from my
perspective it needs to be left as an exercise to the programmer to
add the polling of the /sysfs files into the event loop. We might
offer an example of how to do this though.

> In this context, it is possible to imagine triggering a bus-rescan upon
> device ADD, iff we explicitly define scan() as being idempotent (a thing
> that is not part of its API yet and that we cannot expect from buses at
> this point).

Hmm, so from what I can tell the PCI bus offers an idempotent scan()
and if I haven't added any bugs this is true for the virtual bus too.

> Then, we can limit bus->plug() to a probe, once we are sure that
> metadatas for the bus are (almost) always in sync with the system.
>
> Where we are:
>
> - Intel is proposing a UEVENT API[1]. It might be interesting to help them
>   make it respect a generic framework.

Just because you can do it and someone invested time doesn't mean its
a good idea. What problem is this solving? You now have a DPDK native
library that reads uevents ... Where is the benefit for the
application developer?

Although we replicate some components of an operating system in
userspace it doesn't mean that we are building one. If we don't push
back on things that don't belong here we will have a big pile of
average code soon instead of focusing on the technical problems that
need to get addressed.

> - A first plug / unplug implementation is being proposed. Plug() is
>   currently effectively defined as (scan-one + probe).
>
> What can be done to go forward:
>
> - Define the API for rte_bus interrupt sources, configuration and
>   triggers to allow the development of the needed subsystems.
>
> - Each device event sources should offer an event parser to transform
>   them in device descriptions. Depending on the specifics of the source,
>   different levels of info are available.
>
> - Redefine the requirements for bus->scan() to allow hotplugging.
>
> - Reduce the scope of plug() from (scan-one + probe) to (probe), as
>   everything is now in place.
>

Also see the series that I send out today. From my point of view we
are here already.

> - Further hotplugging developments are then possible: add INTR_ADD
>   support, with flexible device definition for example... A thing that
>   is not yet possible with the current architecture but seemed to
>   interest a few people.
>
> If we can agree that this is a way forward, do you think Jan that having
> the current plug() API hinders this plan? We can reduce its scope later,
> without changing current implementations as, as you said, a proper
> scan should be idempotent. The future API as envisionned is already
> respected, but right now the hotplug support in buses is a little more
> involved. Applications that will start using plug() right now would not
> have to be rewritten, as the requirements for plugging would still be
> fullfilled.
>
> We can support already hotplugging in DPDK. We can refine this support
> later to make it more generic and easier to implement for PMD
> maintainers. But I do not see this as a reason to block this support
> from being integrated right now.

Indeed. I had hotplug support in the version of DPDK that I'm working
with for the last years. Don't get me wrong: I'm not arguing against
the inclusion of hotplug code. I just don't understand the reasoning
behind the implementation you proposed (with my original SOB)
especially since some of the things that you listed as going forward
are already there, are easy to fix or don't necessarily need to be
part of the DPDK EAL.

So lets try to get this right from the beginning. What is missing:
1. document and verify that existing scan() implementations are idempotent
2. example app with udev based hotplug
3. check that the symbols are in the correct libraries (bus, pci, vdev)

What am I missing?

> [1]: http://dpdk.org/ml/archives/dev/2017-June/069057.html
>
> --
> Gaėtan Rivet
> 6WIND
  

On Thu, Jun 29, 2017 at 09:20:30PM +0200, Jan Blunck wrote:
> On Thu, Jun 29, 2017 at 2:59 PM, Gaëtan Rivet <gaetan.rivet@6wind.com> wrote:
> > What can be done to go forward:
> >
> > - Define the API for rte_bus interrupt sources, configuration and
> >   triggers to allow the development of the needed subsystems.
> >
> > - Each device event sources should offer an event parser to transform
> >   them in device descriptions. Depending on the specifics of the source,
> >   different levels of info are available.
> >
> > - Redefine the requirements for bus->scan() to allow hotplugging.
> >
> > - Reduce the scope of plug() from (scan-one + probe) to (probe), as
> >   everything is now in place.
> >
> 
> Also see the series that I send out today. From my point of view we
> are here already.
> 

Yes, your series is only a superficial departure from the v6. In the end,
I see that your critics were pretty much only on interfaces and that you
simply wanted it to be your way.

You did not expose your reason for disagreeing, thus I threw a few ideas
to at least make you either explicit your view or accept the current
proposal.

> > - Further hotplugging developments are then possible: add INTR_ADD
> >   support, with flexible device definition for example... A thing that
> >   is not yet possible with the current architecture but seemed to
> >   interest a few people.
> >
> > If we can agree that this is a way forward, do you think Jan that having
> > the current plug() API hinders this plan? We can reduce its scope later,
> > without changing current implementations as, as you said, a proper
> > scan should be idempotent. The future API as envisionned is already
> > respected, but right now the hotplug support in buses is a little more
> > involved. Applications that will start using plug() right now would not
> > have to be rewritten, as the requirements for plugging would still be
> > fullfilled.
> >
> > We can support already hotplugging in DPDK. We can refine this support
> > later to make it more generic and easier to implement for PMD
> > maintainers. But I do not see this as a reason to block this support
> > from being integrated right now.
> 
> Indeed. I had hotplug support in the version of DPDK that I'm working
> with for the last years. Don't get me wrong: I'm not arguing against
> the inclusion of hotplug code. I just don't understand the reasoning
> behind the implementation you proposed (with my original SOB)
> especially since some of the things that you listed as going forward
> are already there, are easy to fix or don't necessarily need to be
> part of the DPDK EAL.
> 

Don't get me wrong, I am not personally a proponent of these changes,
but I wanted the current implementation to at least leave things open as
I think a few people will push for this in a near future.

But as far as I'm concerned, the hotplug support in DPDK will be
sufficient in v17.08.

> So lets try to get this right from the beginning. What is missing:
> 1. document and verify that existing scan() implementations are idempotent
> 2. example app with udev based hotplug
> 3. check that the symbols are in the correct libraries (bus, pci, vdev)
> 
> What am I missing?
> 

Nothing on the technical side.