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