Re: [PATCH v2 2/8] ib_umem: Add a new, more generic ib_umem_get_attrs

From: Knut Omang
Date: Wed Apr 05 2017 - 11:21:43 EST


Dough,Â

This patch and the next in this set is IMHO together a generic enhancement/cleanup,
is it something you would consider to take just as that?

I am looking at suggesting a simplified, less target specific version of this set
to get it off my chest,

Thanks,
Knut

On Fri, 2016-09-16 at 20:31 +0200, Knut Omang wrote:
> This call allows a full range of DMA attributes and also
> DMA direction to be supplied and is just a refactor of the old ib_umem_get.
> Reimplement ib_umem_get using the new generic call,
> now a trivial implementation.
> ---
> Âdrivers/infiniband/core/umem.c | 23 +++++++++++++++--------
> Âinclude/rdma/ib_umem.hÂÂÂÂÂÂÂÂÂ| 15 ++++++++++++++-
> Â2 files changed, 29 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
> index c68746c..699a0f7 100644
> --- a/drivers/infiniband/core/umem.c
> +++ b/drivers/infiniband/core/umem.c
> @@ -52,7 +52,7 @@ static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int d
> Â if (umem->nmap > 0)
> Â ib_dma_unmap_sg(dev, umem->sg_head.sgl,
> Â umem->nmap,
> - DMA_BIDIRECTIONAL);
> + umem->dir);
> Â
> Â for_each_sg(umem->sg_head.sgl, sg, umem->npages, i) {
> Â
> @@ -82,6 +82,17 @@ static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int d
> Âstruct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
> Â ÂÂÂÂsize_t size, int access, int dmasync)
> Â{
> + unsigned long dma_attrs = 0;
> + if (dmasync)
> + dma_attrs |= DMA_ATTR_WRITE_BARRIER;
> + return ib_umem_get_attrs(context, addr, size, access, DMA_BIDIRECTIONAL, dma_attrs);
> +}
> +EXPORT_SYMBOL(ib_umem_get);
> +
> +struct ib_umem *ib_umem_get_attrs(struct ib_ucontext *context, unsigned long addr,
> + ÂÂsize_t size, int access, enum dma_data_direction dir,
> + ÂÂunsigned long dma_attrs)
> +{
> Â struct ib_umem *umem;
> Â struct page **page_list;
> Â struct vm_area_struct **vma_list;
> @@ -91,16 +102,11 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
> Â unsigned long npages;
> Â int ret;
> Â int i;
> - unsigned long dma_attrs = 0;
> Â struct scatterlist *sg, *sg_list_start;
> Â int need_release = 0;
> Â
> - if (dmasync)
> - dma_attrs |= DMA_ATTR_WRITE_BARRIER;
> -
> Â if (!size)
> Â return ERR_PTR(-EINVAL);
> -
> Â /*
> Â Â* If the combination of the addr and size requested for this memory
> Â Â* region causes an integer overflow, return error.
> @@ -121,6 +127,7 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
> Â umem->addressÂÂÂ= addr;
> Â umem->page_size = PAGE_SIZE;
> Â umem->pidÂÂÂÂÂÂÂ= get_task_pid(current, PIDTYPE_PID);
> + umem->dir = dir;
> Â /*
> Â Â* We ask for writable memory if any of the following
> Â Â* access flags are set.ÂÂ"Local write" and "remote write"
> @@ -213,7 +220,7 @@ struct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
> Â umem->nmap = ib_dma_map_sg_attrs(context->device,
> Â ÂÂumem->sg_head.sgl,
> Â ÂÂumem->npages,
> - ÂÂDMA_BIDIRECTIONAL,
> + ÂÂdir,
> Â ÂÂdma_attrs);
> Â
> Â if (umem->nmap <= 0) {
> @@ -239,7 +246,7 @@ out:
> Â
> Â return ret < 0 ? ERR_PTR(ret) : umem;
> Â}
> -EXPORT_SYMBOL(ib_umem_get);
> +EXPORT_SYMBOL(ib_umem_get_attrs);
> Â
> Âstatic void ib_umem_account(struct work_struct *work)
> Â{
> diff --git a/include/rdma/ib_umem.h b/include/rdma/ib_umem.h
> index 2d83cfd..2876679 100644
> --- a/include/rdma/ib_umem.h
> +++ b/include/rdma/ib_umem.h
> @@ -36,6 +36,7 @@
> Â#include <linux/list.h>
> Â#include <linux/scatterlist.h>
> Â#include <linux/workqueue.h>
> +#include <linux/dma-direction.h>
> Â
> Âstruct ib_ucontext;
> Âstruct ib_umem_odp;
> @@ -47,6 +48,7 @@ struct ib_umem {
> Â int page_size;
> Â intÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂwritable;
> Â intÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂhugetlb;
> + enum dma_data_direction dir;
> Â struct work_struct work;
> Â struct pidÂÂÂÂÂÂÂÂÂÂÂÂÂ*pid;
> Â struct mm_structÂÂÂÂÂÂÂ*mm;
> @@ -81,9 +83,12 @@ static inline size_t ib_umem_num_pages(struct ib_umem *umem)
> Â}
> Â
> Â#ifdef CONFIG_INFINIBAND_USER_MEM
> -
> Âstruct ib_umem *ib_umem_get(struct ib_ucontext *context, unsigned long addr,
> Â ÂÂÂÂsize_t size, int access, int dmasync);
> +struct ib_umem *ib_umem_get_attrs(struct ib_ucontext *context, unsigned long addr,
> + ÂÂsize_t size, int access,
> + ÂÂenum dma_data_direction dir,
> + ÂÂunsigned long dma_attrs);
> Âvoid ib_umem_release(struct ib_umem *umem);
> Âint ib_umem_page_count(struct ib_umem *umem);
> Âint ib_umem_copy_from(void *dst, struct ib_umem *umem, size_t offset,
> @@ -98,6 +103,14 @@ static inline struct ib_umem *ib_umem_get(struct ib_ucontext *context,
> Â ÂÂint access, int dmasync) {
> Â return ERR_PTR(-EINVAL);
> Â}
> +static inline struct ib_umem *ib_umem_get_attrs(struct ib_ucontext *context,
> + unsigned long addr,
> + size_t size, int access,
> + enum dma_data_direction dir,
> + unsigned long dma_attrs)
> +{
> + return ERR_PTR(-EINVAL);
> +}
> Âstatic inline void ib_umem_release(struct ib_umem *umem) { }
> Âstatic inline int ib_umem_page_count(struct ib_umem *umem) { return 0; }
> Âstatic inline int ib_umem_copy_from(void *dst, struct ib_umem *umem, size_t offset,