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

[Jason Gunthorpe <jgg@ziepe.ca>]

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?

Jason