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

["Bernard Metzler" <BMT@zurich.ibm.com>]

-----"Leon Romanovsky" <leon@kernel.org> wrote: -----

>To: "Bernard Metzler" <BMT@zurich.ibm.com>
>From: "Leon Romanovsky" <leon@kernel.org>
>Date: 10/29/2019 05:55AM
>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: Re: [[PATCH v2 for-next]] RDMA/siw:
>Fix SQ/RQ drain logic
>
>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)
>

The conditional code is always rather short:

- a few lines for extra checks during post_sq,
  post_rq and post_srq to forbid writing queue
  entries via syscall.

- write the qp kernel pointer to a CQE only
  if it is a kernel application. Don't expose
  it to user land.

IMO, these checks do not qualify for a change
to a function indirection, which would establish
two very similar functions, differing only in
very few lines. It would also decrease readability.
The function pointer would have to be part of
the resource itself (QP/SRQ/CQ), as the flag
is now.

Let me limit the usage of this obviously unliked
flag to its possible minimum. During resource
construction/destruction I do not really need
it (except setting it), since 'udata' is there.
It would appear only in the fast path for
meentioned checks.
I still prefer that siw private flag to
avoid to rely on potentially changing semantics
of rdma core private structures the driver
IMHO better should not interpret. But I am
completely open to do it that way, if preferred
by the maintainers.

Thanks and best regards,
Bernard.

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