On 3/1/2017 4:47 PM, Alex Rosenbaum wrote:
> On Wed, Mar 1, 2017 at 8:00 PM, Hefty, Sean <sean.hefty@xxxxxxxxx> wrote:
>>>>> I don't think that we should introduce an asych context into
>>> libibverbs.
>>>>
>>>> Why not?
>>>
>>> Generally, I dislike the idea of running threads from libraries,
>>> particularly libraries like ibverbs. So many apps get no benefit from
>>> the thread, but it sits there connected to udev..
>>
>> I thought Doug was referring to reporting the device add/remove event through some event reporting interface, like what the librdmacm already does. So no threads for that are needed. IMO, this makes sense.
>>
>> I didn't follow his idea for how the device list would be updated and kept in sync between the app and the library.
>>
>> If you had device add/remove events, the get_event call could intercept that event and update the device list there. But I don't know that you try to sync a shared list between the library and the app.
>
> In order to provide 'out-of-ibv_context' libibverbs events we'll need
> a new interface, something like ibv_get_system_event(), to report
> device changes.
Yep.
> Every time the application calls this API, libibverbs must process all
> events in queue in order to have an up to date ibv_device list with
> they way it appears in sysfs.
This is not necessarily true. It is entirely possible that the library
has already processed the events themselves internally and the device
list is already consistent. You're tying the act of the user calling
into the library with the library processing work that needs processed.
These two ought not be tied.
> Then, do we force all these popped events on the user? or can we force
> him to loop on ibv_get_system_event() until EAGAIN so we're sure the
> device list is updated?
If the user wants to drain that queue, then that would be just fine. On
the other hand, as I wrote in my original implementation proposal, the
user could also simply call ibv_get_device_list and get the updated
list, which directly implies that the queue of events need not be popped
clean in order for the processing work to have already been done.
> Is this hiding yet another fd for the application to block on?
Personally I would use a thread in the library that would block on
inotify events from the sysfs directory. Any time it got woke up, it
would process the remove/add event, update the device list, create a
notification if the app had registered a notification handler and queue
that to the app, then go back to sleep on the inotify events in the
sysfs dir. The processing of the event and the notification would be
totally disjoint. The app need not process events, even if it
registered and event handler, in order to get the new device list.
> Verbs is built around a cmd_fd per ibv_context. Reporting of new
> devices hotplug needs a separate channel. It does not seem correct to
> add yet another such cdev to verbs.
I would have no intention of adding a cdev or anything else kernel
related to this. This can be done 100% in user space, and should be.
That also makes the change backward compatible with earlier kernels.
> I agree the adding this hotplug report capabilities into librdmacm
> sounds more appropriate.
> But libibvers will still need to provide the latest ibv_device list
> snapshot as it appears in sysfs.
Under my proposed implementation it would.
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG Key ID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD
Attachment:
signature.asc
Description: OpenPGP digital signature
