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

Michal Kalderon
2019-09-23T09:36:12Z

> 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