On Mon, 2019-06-17 at 18:54 +0000, Jason Gunthorpe wrote:
> On Fri, Jun 14, 2019 at 03:00:32PM -0400, Doug Ledford wrote:
> > On Thu, 2019-06-13 at 21:38 -0300, Jason Gunthorpe wrote:
> > > + if (ibdev)
> > > + ret = __ib_get_client_nl_info(ibdev, client_name,
> > > res);
> > > + else
> > > + ret = __ib_get_global_client_nl_info(client_name,
> > > res);
> > > +#ifdef CONFIG_MODULES
> > > + if (ret == -ENOENT) {
> > > + request_module("rdma-client-%s", client_name);
> > > + if (ibdev)
> > > + ret = __ib_get_client_nl_info(ibdev,
> > > client_name, res);
> > > + else
> > > + ret =
> > > __ib_get_global_client_nl_info(client_name, res);
> > > + }
> > > +#endif
> >
> > I was trying to put my finger on something yesterday while reading
> > the
> > code, and this change makes it more clear for me. Do we really
> > want to
> > limit the info type based on ibdev? It seems to me that all global
> > info retrieval should work whether you open a specific ibdev or
> > not.
>
> Each chardev name has a specified query protocol - global chardevs
> must not specify a ibdev, and local ones must. Each name can only be
> global or ibdev - no mixing. Too confusing.
I can see where that's the uapi as envisioned, my point though is would
it be better to allow opening of an ibdev, retrieval of device specific
data, and also retrieval of the available global data? It just
prevents having to open two files to get information that isn't device
specific. But, it's not a big deal either.
> It is uapi so we should be strict, if the ibdev is not allowed then
> it
> should be checked to be absent in case we do something different
> later.
>
> > The other thing I was wondering about was the module
> > loading. Every
> > attempt to load a module is a fork/exec cycle and a context switch
> > over
> > to modprobe and back, and we make no attempt here to keep each
>
> It is a common pattern in the kernel, ie we did exactly this code to
> load the ib netlink module in the netlink core.
>
> If there is a problem then it should be addressed globally..
Yeah, I agree. I'm not sure there is a problem.
> > indicate we've attempted to load that module, and on -ENOENT, we
> > check
> > the table for a match to our passed in client_name, and only if we
> > have
> > a match, and it's load count is 0, do we call request_module() and
> > increment the load count. Thoughts?
>
> I assume it becomes single threaded and batched at some point, so it
> is not so bad...
Thanks, series applied.
--
Doug Ledford <dledford@xxxxxxxxxx>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57
2FDD
Attachment:
signature.asc
Description: This is a digitally signed message part
