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

[Gaëtan Rivet]

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 at monjalon.net> wrote:
> > > 28/06/2017 15:09, Jan Blunck:
> > >> On Wed, Jun 28, 2017 at 2:11 PM, Thomas Monjalon <thomas at monjalon.net> wrote:
> > >> > 28/06/2017 13:58, Jan Blunck:
> > >> >> On Wed, Jun 28, 2017 at 1:44 PM, Thomas Monjalon <thomas at monjalon.net> wrote:
> > >> >> > 27/06/2017 21:03, Jan Blunck:
> > >> >> >> On Tue, Jun 27, 2017 at 6:11 PM, Gaetan Rivet <gaetan.rivet at 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

-- 
Gaëtan Rivet
6WIND