On 2/28/2017 11:32 AM, Liran Liss wrote: >> From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma- >> owner@xxxxxxxxxxxxxxx] On Behalf Of Doug Ledford > >>> not part of this RFC. User space applications should monitor >>> respectful changes >>> >>> in the system according to its specific logic to detect plugout or >>> plugin of >>> >>> hardware devices. This can be does by means of: netlink, udev or other >>> inputs. >> >> On this point I disagree. I see no reason that we should implement hotplug >> detection in every application when the library can get it right just once and >> make it available to everyone. >> > > This is definitely something we should consider doing, but orthogonal to > obtaining an up-to-date device list. Not really. Half of the logic in the original RFC can be considered hacks to work around not doing this properly in the first place. >>> Here I suggest to modify the implementation of ibv_get_device_list() >>> so that >>> >>> consecutive calls will re-scan the sysfs in the same manner as done >>> today in >>> >>> order to create a fresh ibv_device list each time. We will remove >>> caching of >>> >>> devices that support plugout, while keeping the ibv_device cache for >>> devices >>> >>> which do not support plugout. >> >> I would argue that a better solution is to, on first call into >> ibv_get_device_list() (which doubles as the library's init() entry), we scan the >> system for whatever devices there are, build a permanent list of these devices, >> then register the necessary hooks ourselves to get updates on both plug out and >> plug in, and on those events we update the list as appropriate (on plug out, >> notify the application via async event, after the application has processed the >> event, if the device is unused, remove the device entirely, if the device is still in >> use, then move it to a defunct list and wait for the application to be able to >> release all of its references to it, and only then remove it; on plug in, add a new >> device to the device list and notify the application via async event at which point >> the application can decide if it wants to use the device). This might actually >> involve a new async notifier added to the libibverbs API that is not tied to a >> device and which an application would need to be modified to register with the >> library. >> But since any applicaiton wanting to support this is going to need to be modified >> anyway, a new entry point isn't the end of the world. >> > > I don't think that we should introduce an asych context into libibverbs. Why not? > Also, not all apps would like to process such asynch events - some merely want to > use the RDMA subsystem at some time and get an up-to-date device list. They don't have to. If they want to register an event handler and get notified, then they can. If they don't want to, then they don't need to. Behind the scenes, if libibverbs simply does the right thing, then an app that gets async events will work fine, and an app that wants to simply call ibv_get_device_list and scan for new devices also works fine. > > In the future, I would introduce some ibv_create_notifier_channel(), which > apps can ibv_get_notifier_event() from it that informs that the device list has changed... > > This is a simpler API that maintains backward compatibility with ibv_get_device_list(). I couldn't care less that this is simpler. If it means maintaining two implementations, or implementation hacks instead of a single proper implementation, then it's not worth it. Just do it right the first time, especially since we aren't talking about some huge implementation that would take months to get correct. Taking baby steps and going one piece at a time is, IMO, reserved for trickier things to implement than this. This is straight forward enough to go from start to final implementation in one shot. -- 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
