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

[Jan Blunck]

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".

>> >> 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.