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 03:16PM
>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: 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.
>> 
>> Forget my last reply. I was under the impression the RDMA core
>mmap_xa
>> helper stuff would free the resource. Sorry about that. So just do
>not free
>> the resource in siw_mmap_free().
>OK, will leave the logic exactly as it was before, just use the
>common mmap helper
>Functions. 
>thanks
>
Hi Michal,

Let me send a short patch to the list which should
fix it for siw. There were a few more nits:

- make the siw_user_mmap_entry.address a void *, which
  naturally fits with remap_vmalloc_range. also avoids
  other casting during resource address assignment.
    
- do not kfree SQ/RQ/CQ/SRQ in preparation of mmap.
  Those resources are always further needed ;)
    
- Fix check for correct mmap range in siw_mmap().
  - entry->length is the object length. We have to
    expand to PAGE_ALIGN(entry->length), since mmap
    comes with complete page(s) containing the
    object.
  - put mmap_entry if that check fails. Otherwise
    entry object ref counting screws up, and later
    crashes during context close.
    
- simplify siw_mmap_free() - it must just free
  the entry.


I think we shall further really change the size of
the to be mmapped object to size_t. That perfectly
reflects the user API mmap() call and is the right
type for both 64bit and 32bit architectures.

Best regards
Bernard.