Re: RE: RE: [EXT] Re: [PATCH v8 rdma-next 4/7] RDMA/siw: Use the common mmap_xa helpers

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

-----"Michal Kalderon" <mkalderon@marvell.com> wrote: -----

>To: "Bernard Metzler" <BMT@zurich.ibm.com>
>From: "Michal Kalderon" <mkalderon@marvell.com>
>Date: 09/02/2019 10:01AM
>Cc: "Ariel Elior" <aelior@marvell.com>, "jgg@ziepe.ca"
><jgg@ziepe.ca>, "dledford@redhat.com" <dledford@redhat.com>,
>"galpress@amazon.com" <galpress@amazon.com>, "sleybo@amazon.com"
><sleybo@amazon.com>, "leon@kernel.org" <leon@kernel.org>,
>"linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>
>Subject: [EXTERNAL] RE: RE: [EXT] Re: [PATCH v8 rdma-next 4/7]
>RDMA/siw: Use the common mmap_xa helpers
>
>> From: linux-rdma-owner@vger.kernel.org <linux-rdma-
>> owner@vger.kernel.org> On Behalf Of Bernard Metzler
>> 
>> -----"Michal Kalderon" <mkalderon@marvell.com> wrote: -----
>> 
>> >To: "Bernard Metzler" <BMT@zurich.ibm.com>
>> >From: "Michal Kalderon" <mkalderon@marvell.com>
>> >Date: 08/30/2019 02:42PM
>> >Cc: "Ariel Elior" <aelior@marvell.com>, "jgg@ziepe.ca"
>> ><jgg@ziepe.ca>, "dledford@redhat.com" <dledford@redhat.com>,
>> >"galpress@amazon.com" <galpress@amazon.com>,
>> "sleybo@amazon.com"
>> ><sleybo@amazon.com>, "leon@kernel.org" <leon@kernel.org>,
>> >"linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>, "Ariel
>> >Elior" <aelior@marvell.com>
>> >Subject: [EXTERNAL] RE: [EXT] Re: [PATCH v8 rdma-next 4/7]
>RDMA/siw:
>> >Use the common mmap_xa helpers
>> >
>> >> From: Bernard Metzler <BMT@zurich.ibm.com>
>> >> Sent: Friday, August 30, 2019 3:08 PM
>> >>
>> >> External Email
>> >>
>> >>
>>
>>--------------------------------------------------------------------
>-
>> >-
>> >> -----"Michal Kalderon" <michal.kalderon@marvell.com> wrote:
>-----
>> >>
>> >> Hi Michael,
>> >>
>> >> I tried this patch. It unfortunately panics immediately when siw
>> >gets used. I'll
>> >> investigate further. Some comments in line.
>> >Thanks for testing,
>> >
>> >>
>> >> Thanks
>> >> Bernard.
>> >>
>> >> >To: <mkalderon@marvell.com>, <aelior@marvell.com>,
>> <jgg@ziepe.ca>,
>> >> ><dledford@redhat.com>, <bmt@zurich.ibm.com>,
>> >> <galpress@amazon.com>,
>> >> ><sleybo@amazon.com>, <leon@kernel.org>
>> >> >From: "Michal Kalderon" <michal.kalderon@marvell.com>
>> >> >Date: 08/27/2019 03:31PM
>> >> >Cc: <linux-rdma@vger.kernel.org>, "Michal Kalderon"
>> >> ><michal.kalderon@marvell.com>, "Ariel Elior"
>> >> ><ariel.elior@marvell.com>
>> >> >Subject: [EXTERNAL] [PATCH v8 rdma-next 4/7] RDMA/siw: Use the
>> >> common
>> >> >mmap_xa helpers
>> >> >
>> >> >Remove the functions related to managing the mmap_xa database.
>> >> >This code is now common in ib_core. Use the common API's
>instead.
>> >> >
>> >> >Signed-off-by: Ariel Elior <ariel.elior@marvell.com>
>> >> >Signed-off-by: Michal Kalderon <michal.kalderon@marvell.com>
>> >> >---
>> >> > drivers/infiniband/sw/siw/siw.h       |  20 ++-
>> >> > drivers/infiniband/sw/siw/siw_main.c  |   1 +
>> >> > drivers/infiniband/sw/siw/siw_verbs.c | 223
>> >> >+++++++++++++++++++---------------
>> >> > drivers/infiniband/sw/siw/siw_verbs.h |   1 +
>> >> > 4 files changed, 144 insertions(+), 101 deletions(-)
>> >> >
>> >> >+			/* If entry was inserted successfully, qp->sendq
>> >> >+			 * will be freed by siw_mmap_free
>> >> >+			 */
>> >> >+			qp->sendq = NULL;
>> >>
>> >> qp->sendq points to the SQ array. Zeroing this pointer will
>leave
>> >> siw with no idea where the WQE's are. It will panic
>de-referencing
>> >[NULL +
>> >> current position in ring buffer]. Same for RQ, SRQ and CQ.
>> >Qp->sendq is only used in kernel mode, and only set to NULL is
>> >user-space mode
>> >Where it is allocated and mapped in user, so the user will be the
>one
>> >accessing the rings And not kernel, unless I'm missing something.
>> 
>> These pointers are pointing to the allocated work queues.
>> These queues/arrays are holding the actual send/recv/complete/srq
>work
>> queue elements. It is a shared array. That is why we need mmap at
>all.
>> 
>> e.g.,
>> 
>> struct siw_sqe *sqe = &qp->sendq[qp->sq_get % qp->attrs.sq_size];
>> 
>> 
>Ok got it, I'm HW oriented... so user chains and kernel are totally
>separated. 
>Will add a flag whether to free or not and remove setting to NULL. 
>Thanks,
>Michal
>
Michael,
I even think there is an underlying design decision I am
questioning. The provider allocates the resource to be mmapped,
and there is no reason to have the mmapping service freeing
that resource at any point in time. I simply don't understand
why the mmap service wants to free the mmapped resource at all,
if it did not allocate it. The aim of the service
should be limited to provide mmapping bookkeeping.

The new mmap helper framework already defines a callback
for unmapping. The provider may use it to free the
resource, if appropriate. E.g., siw will not destroy a 
currently used CQ, only because the user land application
calls munmap on it.

Best regards,
Bernard.