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

[Michal Kalderon <mkalderon@marvell.com>]

> From: Bernard Metzler <BMT@zurich.ibm.com>
> Sent: Monday, September 2, 2019 1:55 PM
> 
> -----"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,
Michal  (not Michael) thanks 😊 

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

The function that frees the memory is the siw_mmap_free in the provider , not the 
Mapping service. 
This is called when provider calls mmap_remove and ALL maps to the address were unmapped or 
Zapped, we don't want to free the memory before the mapping was removed. 
Once the provider calls mmap_entry_insert the refcnt is set to '1' and can
Only reach zero after provider calls mmap_entry_remove. 

Thanks,
Michal

> 
> Best regards,
> Bernard.