Re: Re: Re: Re: [[PATCH v2 for-next]] RDMA/siw: Fix SQ/RQ drain logic

[Leon Romanovsky <leon@kernel.org>]

On Mon, Oct 28, 2019 at 12:37:38PM +0000, Bernard Metzler wrote:
> -----"Leon Romanovsky" <leon@kernel.org> wrote: -----
>
> >To: "Bernard Metzler" <BMT@zurich.ibm.com>
> >From: "Leon Romanovsky" <leon@kernel.org>
> >Date: 10/27/2019 06:21AM
> >Cc: "Jason Gunthorpe" <jgg@ziepe.ca>, linux-rdma@vger.kernel.org,
> >bharat@chelsio.com, nirranjan@chelsio.com, krishna2@chelsio.com,
> >bvanassche@acm.org
> >Subject: [EXTERNAL] Re: Re: Re: [[PATCH v2 for-next]] RDMA/siw: Fix
> >SQ/RQ drain logic
> >
> >On Fri, Oct 25, 2019 at 12:11:16PM +0000, Bernard Metzler wrote:
> >> -----"Jason Gunthorpe" <jgg@ziepe.ca> wrote: -----
> >>
> >> >To: "Bernard Metzler" <BMT@zurich.ibm.com>
> >> >From: "Jason Gunthorpe" <jgg@ziepe.ca>
> >> >Date: 10/04/2019 07:48PM
> >> >Cc: "Leon Romanovsky" <leon@kernel.org>,
> >linux-rdma@vger.kernel.org,
> >> >bharat@chelsio.com, nirranjan@chelsio.com, krishna2@chelsio.com,
> >> >bvanassche@acm.org
> >> >Subject: [EXTERNAL] Re: Re: [[PATCH v2 for-next]] RDMA/siw: Fix
> >SQ/RQ
> >> >drain logic
> >> >
> >> >On Fri, Oct 04, 2019 at 02:09:57PM +0000, Bernard Metzler wrote:
> >> >> <...>
> >> >>
> >> >> >>   *
> >> >> >> @@ -705,6 +746,12 @@ int siw_post_send(struct ib_qp *base_qp,
> >> >const
> >> >> >struct ib_send_wr *wr,
> >> >> >>  	unsigned long flags;
> >> >> >>  	int rv = 0;
> >> >> >>
> >> >> >> +	if (wr && !qp->kernel_verbs) {
> >> >> >
> >> >> >It is not related to this specific patch, but all siw
> >> >"kernel_verbs"
> >> >> >should go, we have standard way to distinguish between kernel
> >and
> >> >> >user
> >> >> >verbs.
> >> >> >
> >> >> >Thanks
> >> >> >
> >> >> Understood. I think we touched on that already.
> >> >> rdma core objects have a uobject pointer which
> >> >> is valid only if it belongs to a user land
> >> >> application. We might better use that.
> >> >
> >> >No, the uobject pointer is not to be touched by drivers
> >> >
> >> Now what would be the appropriate way of remembering/
> >> detecting user level nature of endpoint resources?
> >> I see drivers _are_ doing 'if (!ibqp->uobject)' ...
> >
> >IMHO, you should rely in "udata" to distinguish user/kernel objects.
> >
>
> right, we already do that at resource creation time, when
> 'udata' is available.  But there is no such parameter
> around during resource access (post_send/post_recv/poll_cq/...),
> when user land or kernel land application specific
> code might be required.
> That's why siw currently saves that info to a resource
> (QP/CQ/SRQ) specific parameter 'kernel_verbs'. I agree
> this parameter is redundant, if the rdma core object
> provides that information as well. The only way I see
> it provided is the validity of the uobject pointer of
> all those resources.
> Either (1) we use that uobject pointer as an indication
> of application location, or (2) we remember it from
> resource creation time when udata was available, or
> (3) we have the rdma core exporting that information
> explicitly.
> siw, and other drivers, are currently implementing (2).
> Some drivers implement (1). I'd be happy to change siw
> to implement (1) - to get rid of 'kernel_verbs'.

Many if not all "kernel_verbs" variables are copy/paste.
The usual way to handle difference in internal flows is to
rely on having pointer initialized, e.g.
if (siw_device->some_specific_kernel_pointer)
 do_kernelwork(siw_device->some_specific_kernel_pointer->extra)

Thanks

>
> Thanks and best regards,
> Bernard.
>
>
>
>
> >>
> >> Other drivers keep it with the private state, like iw40,
> >> but I learned we shall get rid of it.
> >>
> >> We may export an inline query from RDMA core, or simply
> >> #define is_usermode(ib_obj *) (ib_obj->uobject != NULL)
> >> ?
> >>
> >> Thanks and best regards,
> >> Bernard
> >>
> >
> >
>