Re: [PATCH v12 rdma-next 2/8] RDMA/core: Create mmap database and cookie helper functions

[Jason Gunthorpe <jgg@ziepe.ca>]

On Wed, Oct 30, 2019 at 11:44:11AM +0200, Michal Kalderon wrote:
> diff --git a/drivers/infiniband/core/ib_core_uverbs.c b/drivers/infiniband/core/ib_core_uverbs.c
> index b74d2a2fb342..1ffc89fd5d94 100644
> +++ b/drivers/infiniband/core/ib_core_uverbs.c
> @@ -71,3 +71,204 @@ int rdma_user_mmap_io(struct ib_ucontext *ucontext, struct vm_area_struct *vma,
>  	return 0;
>  }
>  EXPORT_SYMBOL(rdma_user_mmap_io);
> +
> +/**
> + * rdma_user_mmap_entry_get() - Get an entry from the mmap_xa.
> + *
> + * @ucontext: associated user context.
> + * @key: the key received from rdma_user_mmap_entry_insert which
> + *     is provided by user as the address to map.
> + * @vma: the vma related to the current mmap call.
> + *
> + * This function is called when a user tries to mmap a key it
> + * initially received from the driver. The key was created by
> + * the function rdma_user_mmap_entry_insert.
> + * This function increases the refcnt of the entry so that it won't
> + * be deleted from the xa in the meantime.
> + *
> + * Return an entry if exists or NULL if there is no match.
> + */
> +struct rdma_user_mmap_entry *
> +rdma_user_mmap_entry_get(struct ib_ucontext *ucontext, u64 key,
> +			 struct vm_area_struct *vma)

This should just accept 'key' (but called pgoff for clarity), but
since everyone has a vma a static inline wrapper should take in the
vma

> +{
> +	struct rdma_user_mmap_entry *entry;
> +	u64 mmap_page;
> +
> +	mmap_page = key >> PAGE_SHIFT;

It isn't even really 'key' here if it was shifted as that is called
'offset'

> +	if (mmap_page > U32_MAX)
> +		return NULL;
> +
> +	xa_lock(&ucontext->mmap_xa);
> +
> +	entry = xa_load(&ucontext->mmap_xa, mmap_page);

Since each xarray entry in the range stores the same pointer this
needs to check that mmap_page is actually the right entry, attempting
to directly mmap the 2nd page should fail as we don't have the rest of
the infrastructure to make that work.


> +/**
> + * rdma_user_mmap_entry_put() - Drop reference to the mmap entry
> + *
> + * @ucontext: associated user context.
> + * @entry: an entry in the mmap_xa.
> + *
> + * This function is called when the mapping is closed if it was
> + * an io mapping or when the driver is done with the entry for
> + * some other reason.
> + * Should be called after rdma_user_mmap_entry_get was called
> + * and entry is no longer needed. This function will erase the
> + * entry and free it if its refcnt reaches zero.
> + */
> +void rdma_user_mmap_entry_put(struct ib_ucontext *ucontext,
> +			      struct rdma_user_mmap_entry *entry)
> +{

ucontext is not needed

> +	kref_put(&entry->ref, rdma_user_mmap_entry_free);
> +}
> +EXPORT_SYMBOL(rdma_user_mmap_entry_put);
> +
> +/**
> + * rdma_user_mmap_entry_remove() - Drop reference to entry and
> + *				   mark it as invalid.
> + *
> + * @ucontext: associated user context.
> + * @entry: the entry to insert into the mmap_xa
> + */
> +void rdma_user_mmap_entry_remove(struct ib_ucontext *ucontext,
> +				 struct rdma_user_mmap_entry *entry)
> +{

ucontext is not needed

> +/**
> + * rdma_user_mmap_entry_insert() - 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 calls mmap with the given key.
> + * The function allocates 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: 0 on success and -ENOMEM on failure
> + */
> +int rdma_user_mmap_entry_insert(struct ib_ucontext *ucontext,
> +				struct rdma_user_mmap_entry *entry,
> +				size_t length)
> +{
> +	struct ib_uverbs_file *ufile = ucontext->ufile;
> +	XA_STATE(xas, &ucontext->mmap_xa, 0);
> +	u32 xa_first, xa_last, npages;
> +	int err, i;

'i' should be u32

> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 6a47ba85c54c..8a87c9d442bc 100644
> +++ b/include/rdma/ib_verbs.h
> @@ -1471,6 +1471,7 @@ struct ib_ucontext {
>  	 * Implementation details of the RDMA core, don't use in drivers:
>  	 */
>  	struct rdma_restrack_entry res;
> +	struct xarray mmap_xa;
>  };
>  
>  struct ib_uobject {
> @@ -2251,6 +2252,20 @@ struct iw_cm_conn_param;
>  
>  #define DECLARE_RDMA_OBJ_SIZE(ib_struct) size_t size_##ib_struct
>  
> +struct rdma_user_mmap_entry {
> +	struct kref ref;
> +	struct ib_ucontext *ucontext;
> +	u32 npages;
> +	u32 mmap_page;
> +	bool invalid;

These names are confusing, lets use:

       unsigned long start_pgoff;
       size_t npages;
       bool driver_removed;

> +};
> +
> +static inline u64
> +rdma_user_mmap_get_key(const struct rdma_user_mmap_entry *entry)
> +{
> +	return (u64)entry->mmap_page << PAGE_SHIFT;
> +}

This is offset not key, in fact lets not use the word 'key' at
all. Either 'pgoff' or 'offset'

There are also a number of fixables in comments, grammer, indentation
and style..

Jason