-----"Michal Kalderon" <mkalderon@xxxxxxxxxxx> wrote: -----
>To: "Bernard Metzler" <BMT@xxxxxxxxxxxxxx>
>From: "Michal Kalderon" <mkalderon@xxxxxxxxxxx>
>Date: 09/02/2019 03:16PM
>Cc: "Ariel Elior" <aelior@xxxxxxxxxxx>, "jgg@xxxxxxxx"
><jgg@xxxxxxxx>, "dledford@xxxxxxxxxx" <dledford@xxxxxxxxxx>,
>"galpress@xxxxxxxxxx" <galpress@xxxxxxxxxx>, "sleybo@xxxxxxxxxx"
><sleybo@xxxxxxxxxx>, "leon@xxxxxxxxxx" <leon@xxxxxxxxxx>,
>"linux-rdma@xxxxxxxxxxxxxxx" <linux-rdma@xxxxxxxxxxxxxxx>
>Subject: [EXTERNAL] RE: RE: [EXT] Re: [PATCH v8 rdma-next 4/7]
>RDMA/siw: Use the common mmap_xa helpers
>
>> From: linux-rdma-owner@xxxxxxxxxxxxxxx <linux-rdma-
>> owner@xxxxxxxxxxxxxxx> On Behalf Of Bernard Metzler
>>
>> -----"Michal Kalderon" <mkalderon@xxxxxxxxxxx> wrote: -----
>>
>> >To: "Bernard Metzler" <BMT@xxxxxxxxxxxxxx>
>> >From: "Michal Kalderon" <mkalderon@xxxxxxxxxxx>
>> >Date: 09/02/2019 10:01AM
>> >Cc: "Ariel Elior" <aelior@xxxxxxxxxxx>, "jgg@xxxxxxxx"
>> ><jgg@xxxxxxxx>, "dledford@xxxxxxxxxx" <dledford@xxxxxxxxxx>,
>> >"galpress@xxxxxxxxxx" <galpress@xxxxxxxxxx>,
>> "sleybo@xxxxxxxxxx"
>> ><sleybo@xxxxxxxxxx>, "leon@xxxxxxxxxx" <leon@xxxxxxxxxx>,
>> >"linux-rdma@xxxxxxxxxxxxxxx" <linux-rdma@xxxxxxxxxxxxxxx>
>> >Subject: [EXTERNAL] RE: RE: [EXT] Re: [PATCH v8 rdma-next 4/7]
>> >RDMA/siw: Use the common mmap_xa helpers
>> >
>> >> From: linux-rdma-owner@xxxxxxxxxxxxxxx <linux-rdma-
>> >> owner@xxxxxxxxxxxxxxx> On Behalf Of Bernard Metzler
>> >>
>> >> -----"Michal Kalderon" <mkalderon@xxxxxxxxxxx> wrote: -----
>> >>
>> >> >To: "Bernard Metzler" <BMT@xxxxxxxxxxxxxx>
>> >> >From: "Michal Kalderon" <mkalderon@xxxxxxxxxxx>
>> >> >Date: 08/30/2019 02:42PM
>> >> >Cc: "Ariel Elior" <aelior@xxxxxxxxxxx>, "jgg@xxxxxxxx"
>> >> ><jgg@xxxxxxxx>, "dledford@xxxxxxxxxx" <dledford@xxxxxxxxxx>,
>> >> >"galpress@xxxxxxxxxx" <galpress@xxxxxxxxxx>,
>> >> "sleybo@xxxxxxxxxx"
>> >> ><sleybo@xxxxxxxxxx>, "leon@xxxxxxxxxx" <leon@xxxxxxxxxx>,
>> >> >"linux-rdma@xxxxxxxxxxxxxxx" <linux-rdma@xxxxxxxxxxxxxxx>,
>"Ariel
>> >> >Elior" <aelior@xxxxxxxxxxx>
>> >> >Subject: [EXTERNAL] RE: [EXT] Re: [PATCH v8 rdma-next 4/7]
>> >RDMA/siw:
>> >> >Use the common mmap_xa helpers
>> >> >
>> >> >> From: Bernard Metzler <BMT@xxxxxxxxxxxxxx>
>> >> >> Sent: Friday, August 30, 2019 3:08 PM
>> >> >>
>> >> >> External Email
>> >> >>
>> >> >>
>> >>
>>
>>>-------------------------------------------------------------------
>-
>> >-
>> >> >-
>> >> >> -----"Michal Kalderon" <michal.kalderon@xxxxxxxxxxx> 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@xxxxxxxxxxx>, <aelior@xxxxxxxxxxx>,
>> >> <jgg@xxxxxxxx>,
>> >> >> ><dledford@xxxxxxxxxx>, <bmt@xxxxxxxxxxxxxx>,
>> >> >> <galpress@xxxxxxxxxx>,
>> >> >> ><sleybo@xxxxxxxxxx>, <leon@xxxxxxxxxx>
>> >> >> >From: "Michal Kalderon" <michal.kalderon@xxxxxxxxxxx>
>> >> >> >Date: 08/27/2019 03:31PM
>> >> >> >Cc: <linux-rdma@xxxxxxxxxxxxxxx>, "Michal Kalderon"
>> >> >> ><michal.kalderon@xxxxxxxxxxx>, "Ariel Elior"
>> >> >> ><ariel.elior@xxxxxxxxxxx>
>> >> >> >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@xxxxxxxxxxx>
>> >> >> >Signed-off-by: Michal Kalderon <michal.kalderon@xxxxxxxxxxx>
>> >> >> >---
>> >> >> > 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.
