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

From: Michal Kalderon <mkalderon@marvell.com>
To: Jason Gunthorpe <jgg@ziepe.ca>
Cc: Ariel Elior <aelior@marvell.com>,
	"dledford@redhat.com" <dledford@redhat.com>,
	"bmt@zurich.ibm.com" <bmt@zurich.ibm.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: RE: [EXT] Re: [PATCH v11 rdma-next 2/7] RDMA/core: Create mmap database and cookie helper functions
Date: Mon, 23 Sep 2019 09:36:01 +0000	[thread overview]
Message-ID: <MN2PR18MB31822DCFCA312D6A455A000BA1850@MN2PR18MB3182.namprd18.prod.outlook.com> (raw)
In-Reply-To: <20190919180746.GF4132@ziepe.ca>

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Thursday, September 19, 2019 9:08 PM
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Thu, Sep 05, 2019 at 01:01:12PM +0300, Michal Kalderon wrote:
> > +/**
> > + * rdma_user_mmap_entry_remove() - Remove a key's entry from the
> > +mmap_xa
> > + *
> > + * @ucontext: associated user context.
> > + * @key: the key to be deleted
> > + *
> > + * This function will find if there is an entry matching the key and
> > +if so
> > + * decrease its refcnt, which will in turn delete the entry if
> > + * its refcount reaches zero.
> > + */
> > +void rdma_user_mmap_entry_remove(struct ib_ucontext *ucontext,
> u64
> > +key)
> 
> Since the struct rdma_user_mmap_entry already has both of these, doesn't
> it make more sense to pass in the struct pointer to _remove than store the
> key?
> 
> Ie replace things like ctx->db_key with a pointer and make the
> rdma_user_mmap_get_key() into a header inline
> 
> 
> > +/**
> > + * rdma_user_mmap_entry_insert() - Allocate and insert an entry to the
> mmap_xa.
> > + *
> > + * @ucontext: associated user context.
> > + * @entry: the entry to insert into the mmap_xa
> > + * @length: length of the address that will be mmapped
> > + *
> > + * 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 retrieve information such as address to
> > + * be mapped and how.
> > + *
> > + * Return: unique key or RDMA_USER_MMAP_INVALID if entry was not
> added.
> > + */
> > +u64 rdma_user_mmap_entry_insert(struct ib_ucontext *ucontext,
> > +				struct rdma_user_mmap_entry *entry,
> > +				size_t length)
> 
> The similarly we don't need to return a u64 here and the sort of ugly
> RDMA_USER_MMAP_INVALID can go away

So you mean add a return code here ? and on failure driver will delete the 
Allocated xx_user_mmap_entry and not store it in xx_key ? 

> 
> Jason