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

[Gal Pressman <galpress@amazon.com>]

On 20/08/2019 15:18, Michal Kalderon wrote:
> -/*
> - * Each time we map IO memory into user space this keeps track of the mapping.
> - * When the device is hot-unplugged we 'zap' the mmaps in user space to point
> - * to the zero page and allow the hot unplug to proceed.
> +/**
> + * rdma_umap_priv_init() - Initialize the private data of a vma
> + *
> + * @vma: The vm area struct that needs private data
> + * @entry: entry into the mmap_xa that needs to be linked with
> + *       this vma
> + *
> + * Each time we map IO memory into user space this keeps track
> + * of the mapping. When the device is hot-unplugged we 'zap' the
> + * mmaps in user space to point to the zero page and allow the
> + * hot unplug to proceed.
>   *
>   * This is necessary for cases like PCI physical hot unplug as the actual BAR
>   * memory may vanish after this and access to it from userspace could MCE.
>   *
>   * RDMA drivers supporting disassociation must have their user space designed
>   * to cope in some way with their IO pages going to the zero page.
> + *
> + * We extended the umap list usage to track all memory that was mapped by
> + * user space and not only the IO memory. This will occur for drivers that use
> + * the mmap_xa database and helper functions
> + *
> + * Return 0 on success or -ENOMEM if out of memory
>   */
> -void rdma_umap_priv_init(struct rdma_umap_priv *priv,
> -			 struct vm_area_struct *vma)
> +int rdma_umap_priv_init(struct vm_area_struct *vma,
> +			struct rdma_user_mmap_entry *entry)
>  {
>  	struct ib_uverbs_file *ufile = vma->vm_file->private_data;
> +	struct rdma_umap_priv *priv;
> +
> +	/* If the xa_mmap is used, private data will already be initialized.
> +	 * this is required for the cases that rdma_user_mmap_io is called
> +	 * from drivers that don't use the xa_mmap database yet

Yet? I don't think all drivers are going to use it.

> +	 */
> +	if (vma->vm_private_data)
> +		return 0;
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
>  
>  	priv->vma = vma;
> +	priv->entry = entry;
>  	vma->vm_private_data = priv;
>  
>  	mutex_lock(&ufile->umap_lock);
>  	list_add(&priv->list, &ufile->umaps);
>  	mutex_unlock(&ufile->umap_lock);
> +
> +	return 0;
>  }
>  EXPORT_SYMBOL(rdma_umap_priv_init);
>  
> +void rdma_user_mmap_entry_free(struct kref *kref)
> +{
> +	struct rdma_user_mmap_entry *entry =
> +		container_of(kref, struct rdma_user_mmap_entry, ref);
> +	unsigned long i, npages = (u32)DIV_ROUND_UP(entry->length, PAGE_SIZE);
> +	struct ib_ucontext *ucontext = entry->ucontext;
> +
> +	/* need to erase all entries occupied... */
> +	for (i = 0; i < npages; i++) {
> +		xa_erase(&ucontext->mmap_xa, entry->mmap_page + i);
> +
> +		ibdev_dbg(ucontext->device,
> +			  "mmap: obj[0x%p] key[%#llx] addr[%#llx] len[%#llx] npages[%#lx] removed\n",
> +			  entry->obj, rdma_user_mmap_get_key(entry),
> +			  entry->address, entry->length, npages);
> +
> +		if (ucontext->device->ops.mmap_free)
> +			ucontext->device->ops.mmap_free(entry);
> +	}

Should this loop be surrounded with a lock? What happens with concurrent insertions?

> +	kfree(entry);
> +}
> +
> +/**
> + * rdma_user_mmap_entry_put() - drop reference to the mmap entry
> + *
> + * @ucontext: associated user context.
> + * @entry: An entry in the mmap_xa.

Nit: sometimes a capital letter is used and sometimes not.

> + *
> + * This function is called when the mapping is closed 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 it's refcnt reaches zero.

"it's" -> "its".

> + */
> +void rdma_user_mmap_entry_put(struct ib_ucontext *ucontext,
> +			      struct rdma_user_mmap_entry *entry)
> +{
> +	WARN_ON(!kref_read(&entry->ref));
> +	kref_put(&entry->ref, rdma_user_mmap_entry_free);
> +}
> +EXPORT_SYMBOL(rdma_user_mmap_entry_put);
> +
> +/**
> + * 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 it's refcnt, which will in turn delete the entry if its refcount

Same.

> + * reaches zero.
> + */
> +void rdma_user_mmap_entry_remove(struct ib_ucontext *ucontext, u64 key)
> +{
> +	struct rdma_user_mmap_entry *entry;
> +	u32 mmap_page;
> +
> +	if (key == RDMA_USER_MMAP_INVALID)
> +		return;

How could this happen?

> +
> +	mmap_page = key >> PAGE_SHIFT;
> +	if (mmap_page > U32_MAX)
> +		return;
> +
> +	entry = xa_load(&ucontext->mmap_xa, mmap_page);
> +	if (!entry)
> +		return;
> +
> +	rdma_user_mmap_entry_put(ucontext, entry);
> +}
> +EXPORT_SYMBOL(rdma_user_mmap_entry_remove);
> +
> +/**
> + * 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;
> +	entry->address = address;
> +	entry->length = length;
> +	kref_init(&entry->ref);
> +	entry->mmap_flag = mmap_flag;
> +	entry->ucontext = ucontext;
> +
> +	xa_lock(&ucontext->mmap_xa);
> +
> +	/* We want to find an empty range */
> +	npages = (u32)DIV_ROUND_UP(length, PAGE_SIZE);
> +	do {
> +		/* First find an empty index */
> +		xas_find_marked(&xas, U32_MAX, XA_FREE_MARK);
> +		if (xas.xa_node == XAS_RESTART)
> +			goto err_unlock;
> +
> +		xa_first = xas.xa_index;
> +
> +		/* Is there enough room to have the range? */
> +		if (check_add_overflow(xa_first, npages, &xa_last))
> +			goto err_unlock;
> +
> +		/* Iterate over all present entries in the range. If a present
> +		 * entry exists we will finish this with the largest index
> +		 * occupied in the range which will serve as the start of the
> +		 * new search
> +		 */
> +		index_max = xa_last;
> +		xa_for_each_start(&ucontext->mmap_xa, index, ent, xa_first)
> +			if (index < xa_last)
> +				index_max = index;
> +			else
> +				break;
> +		if (index_max == xa_last) /* range is free */
> +			break;
> +		/* o/w start again from largest index found in range */
> +		xas_set(&xas, index_max);
> +	} while (true);
> +
> +	for (i = xa_first; i < xa_last; i++) {
> +		err = __xa_insert(&ucontext->mmap_xa, i, entry, GFP_KERNEL);
> +		if (err)
> +			goto err_undo;
> +	}
> +
> +	entry->mmap_page = xa_first;
> +	xa_unlock(&ucontext->mmap_xa);
> +
> +	ibdev_dbg(ucontext->device,
> +		  "mmap: obj[0x%p] addr[%#llx], len[%#llx], key[%#llx] npages[%#x] inserted\n",
> +		  entry->obj, entry->address, entry->length,
> +		  rdma_user_mmap_get_key(entry), npages);
> +
> +	return rdma_user_mmap_get_key(entry);
> +
> +err_undo:
> +	for (; i > xa_first; i--)
> +		__xa_erase(&ucontext->mmap_xa, i - 1);

Personal taste, but I find this clearer:
	for (i--; i >= xa_first; i--)
		__xa_erase(&ucontext->mmap_xa, i);

> +
> +err_unlock:
> +	xa_unlock(&ucontext->mmap_xa);
> +	kfree(entry);
> +	return RDMA_USER_MMAP_INVALID;
> +}
> +EXPORT_SYMBOL(rdma_user_mmap_entry_insert);
> +
> +/**
> + * rdma_user_mmap_entries_remove_free() - Free remaining entries
> + * in mmap_xa.
> + *
> + * @ucontext: associated user context
> + *
> + * This is only called when the ucontext is destroyed and there
> + * can be no concurrent query via mmap or allocate on the
> + * xarray, thus we can be sure no other thread is using the
> + * entry pointer. We also know that all the BAR pages have
> + * either been zap'd or munmaped at this point. Normal pages are
> + * refcounted and will be freed at the proper time.
> + */
> +void rdma_user_mmap_entries_remove_free(struct ib_ucontext *ucontext)
> +{
> +	struct rdma_user_mmap_entry *entry;
> +	unsigned long mmap_page;
> +
> +	WARN_ON(!xa_empty(&ucontext->mmap_xa));
> +	xa_for_each(&ucontext->mmap_xa, mmap_page, entry) {
> +		ibdev_dbg(ucontext->device,
> +			  "mmap: obj[0x%p] key[%#llx] addr[%#llx] len[%#llx] removed\n",
> +			  entry->obj, rdma_user_mmap_get_key(entry),
> +			  entry->address, entry->length);
> +
> +		/* override the refcnt to make sure entry is deleted */
> +		kref_init(&entry->ref);
> +		rdma_user_mmap_entry_put(ucontext, entry);
> +	}
> +}
> +EXPORT_SYMBOL(rdma_user_mmap_entries_remove_free);
> diff --git a/drivers/infiniband/core/rdma_core.c b/drivers/infiniband/core/rdma_core.c
> index ccf4d069c25c..7166741834c8 100644
> --- a/drivers/infiniband/core/rdma_core.c
> +++ b/drivers/infiniband/core/rdma_core.c
> @@ -817,6 +817,7 @@ static void ufile_destroy_ucontext(struct ib_uverbs_file *ufile,
>  	rdma_restrack_del(&ucontext->res);
>  
>  	ib_dev->ops.dealloc_ucontext(ucontext);
> +	rdma_user_mmap_entries_remove_free(ucontext);

Why did you switch the order again?

>  	kfree(ucontext);
>  
>  	ufile->ucontext = NULL;
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 391499008a22..b66c197a7079 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -1479,6 +1479,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 {
> @@ -2259,6 +2260,19 @@ struct iw_cm_conn_param;
>  
>  #define DECLARE_RDMA_OBJ_SIZE(ib_struct) size_t size_##ib_struct
>  
> +#define RDMA_USER_MMAP_FLAG_SHIFT 56
> +#define RDMA_USER_MMAP_PAGE_MASK GENMASK(EFA_MMAP_FLAG_SHIFT - 1, 0)

These are unused.

> +#define RDMA_USER_MMAP_INVALID U64_MAX
> +struct rdma_user_mmap_entry {
> +	struct kref ref;
> +	struct ib_ucontext *ucontext;
> +	void *obj;
> +	u64 address;
> +	u64 length;
> +	u32 mmap_page;
> +	u8 mmap_flag;
> +};