[openib-general] [PATCH] sa_query: require SA query registration

[Michael S. Tsirkin]

Quoting r. Sean Hefty <sean.hefty at intel.com>:
> Subject: [PATCH] sa_query: require SA query registration
> 
> Require registration with SA module, to prevent module text
> from going away while sa query callback is still running,
> and update all users.
> 
> Signed-off-by: Michael S. Tsirkin <mst at mellanox.co.il>
> Signed-off-by: Sean Hefty <sean.hefty at intel.com>
> ---
> Changes to the previous post include:
> 
> * Move struct ib_sa_client definition internal to SA module to
>   better encapsulate future extensions.

Documentation/stable_api_nonsense.txt says:
"You think you want a stable kernel interface, but you really do not"

>    We can debate whether this is good or bad.

I liked the init/destroy API better.

It seems your API has a (small) extra cost, as ib_sa_register_client will now
remain in-memory even if it is only used at module load time, plus all calls to
SA routines have to load the pointer to structure from memory instead of the
address being computed by linker.

Error handling also seems to be trickier with your patch.

I also liked the fact that the original patch put the name into
the structure, I think ability to figure out who's using the query
will come handy for debugging.

Some more questions on the matter:

We allocate stuff in ib_verbs mainly because there are multiple providers
inheriting structs like ib_qp. But why would you want different
types of clients?

Could you explain how does allocating structure from heap this better
encapsulate future extensions, please?
Why is
	ipoib_sa_client = ib_sa_register_client
	ret = PTR_ERR(ipoib_sa_client)
a better encapsulation than
	ret = ib_sa_register_client(&ipoib_sa_client)

Further, why should this API be different from ib_register_client?
Isn't this confusing?

Isn't it better to let user control where does the memory come from,
rather than forcing GFP_KERNEL?

Take care,

-- 
MST