Re: [PATCH v6 05/11] bus: introduce hotplug functionality

[Jan Blunck <jblunck@infradead.org>]

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