RE: [EXT] Re: [PATCH v7 rdma-next 2/7] RDMA/core: Create mmap database and cookie helper functions

[Michal Kalderon <mkalderon@marvell.com>]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Wednesday, August 21, 2019 7:51 PM
> 
> On Wed, Aug 21, 2019 at 04:47:47PM +0000, Michal Kalderon wrote:
> > > From: linux-rdma-owner@vger.kernel.org <linux-rdma-
> > > owner@vger.kernel.org> On Behalf Of Michal Kalderon
> > >
> > > > From: Jason Gunthorpe <jgg@ziepe.ca>
> > > > Sent: Tuesday, August 20, 2019 4:21 PM
> > > >
> > > > On Tue, Aug 20, 2019 at 03:18:42PM +0300, Michal Kalderon wrote:
> > > > > Create some common API's for adding entries to a xa_mmap.
> > > > > Searching for an entry and freeing one.
> > > > >
> > > > > +
> > > > > +/**
> > > > > + * rdma_user_mmap_entry_insert() - Allocate and insert an entry
> > > > > +to the
> > > > mmap_xa.
> > > > > + *
> > > > > + * @ucontext: associated user context.
> > > > > + * @obj: opaque driver object that will be stored in the entry.
> > > > > + * @address: The address that will be mmapped to the user
> > > > > + * @length: Length of the address that will be mmapped
> > > > > + * @mmap_flag: opaque driver flags related to the address (For
> > > > > + *           example could be used for cachability)
> > > > > + *
> > > > > + * This function should be called by drivers that use the
> > > > > +rdma_user_mmap
> > > > > + * interface for handling user mmapped addresses. The database
> > > > > +is handled in
> > > > > + * the core and helper functions are provided to insert entries
> > > > > +into the
> > > > > + * database and extract entries when the user call mmap with
> > > > > +the given
> > > > key.
> > > > > + * The function returns a unique key that should be provided to
> > > > > +user, the user
> > > > > + * will use the key to map the given address.
> > > > > + *
> > > > > + * Return: unique key or RDMA_USER_MMAP_INVALID if entry was
> > > > > +not
> > > > added.
> > > > > + */
> > > > > +u64 rdma_user_mmap_entry_insert(struct ib_ucontext *ucontext,
> > > > > +void
> > > > *obj,
> > > > > +				u64 address, u64 length, u8
> mmap_flag) {
> > > > > +	XA_STATE(xas, &ucontext->mmap_xa, 0);
> > > > > +	struct rdma_user_mmap_entry *entry;
> > > > > +	unsigned long index = 0, index_max;
> > > > > +	u32 xa_first, xa_last, npages;
> > > > > +	int err, i;
> > > > > +	void *ent;
> > > > > +
> > > > > +	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> > > > > +	if (!entry)
> > > > > +		return RDMA_USER_MMAP_INVALID;
> > > > > +
> > > > > +	entry->obj = obj;
> > > >
> > > > It is more a kernel pattern to have the driver allocate a
> > > > rdma_user_mmap_entry and extend it with its 'priv', then use
> > > > container_of
> > > then would we also want the driver to free the memory ?
> > > Or will it be ok to free it using the kref put callback ?
> >
> > Jason, I looked into this deeper today, it seems that since the Core
> > is the one handling the reference counting, and eventually Freeing the
> > object that it makes more sense to keep the allocation In core and not
> > in the drivers, since the driver won't be able to free The entry
> > without providing yet an additional callback function to the Core to
> > be called once the reference count reaches zero.
> 
> This already added a callback to free the xa_entry, why can't it free all the
> memory too when kref goes to 0?
True, could free it there. I just think we'll have a bit more duplication code
Between the drivers defining a very similar private structure and adding
Allocation calls before each of the rdma_user_mmap_insert function calls. 
 And just to make sure I follow, 
Do you mean creating the following structure per driver: 
Struct <driver>_user_mmap_entry {
	struct rdma_user_mmap_entry umap_entry;
              ... <private fields> ...
}

Thanks again, 
Michal

> 
> Jason