On Mon, Jun 17, 2019 at 03:27:22PM -0400, Doug Ledford wrote:
> 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,
Please drop it.
Thanks
>
>
> --
> Doug Ledford <dledford@xxxxxxxxxx>
> GPG KeyID: B826A3330E572FDD
> Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57
> 2FDD
