Re: [PATCH RFC rdma-core 4/5] mlx5: Add support for ibv_parent domain and its related verbs

[Jason Gunthorpe <jgg-uk2M96/98Pc@public.gmane.org>]

On Tue, Nov 14, 2017 at 12:29:14PM +0200, Yishai Hadas wrote:

> It's not enough, there might be cases that protection_domain is NULL but
> still the object is a parent domain as of in the CQ case where td may be
> applicable but not the protection_domain. Better having here clear
> indication which is set upon the creation API rather than some semantic.

Ugh, the idea of a ibv_pd without a ibv_pd is a ugly. That is a whole
bunch more checking and failure scenarios.

I think I'd rather have the CQ receive a PD it doesn't need than do
that..

> >>+struct ibv_pd *mlx5_alloc_parent_domain(struct ibv_context *context,
> >>+			struct ibv_parent_domain_init_attr *attr)

> >>+	pd->protection_domain= attr->pd;
> >
> >Don't we need some kind of ref counting here?
> >
> 
> We thought about it as well, however reference counting can't prevent race
> conditions when more than one thread uses the API.

Not really concerned about races. More concerned about helping the
user use it properly..

> Similar behavior exists also in current code which puts the responsibility
> on the application to correctly use the API instead of using some reference
> counting /locks inside the libraries.

We have refcounts in the kernel side for many things and generate
errors if the user does stuff in the wrong order.

> >What is the intention for the final version of this patch?
> 
> The next patch of this RFC around QP creation introduces the
> expected usage.

Well, this transformation would have to ultimately be done in this
patch or before.

Looking more, this isn't good enough, at the very least I think you
need to memcpy the struct ibv_pd portion so context and handle are
set, as those are public ABIs.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html