Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Jerome Glisse]

On Wed, May 9, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com> wrote:
> this feature is used to import user space region allocated by malloc() or
> mmaped into a gem. and to guarantee the pages to user space not to be
> swapped out, the VMAs within the user space would be locked and then unlocked
> when the pages are released.
>
> but this lock might result in significant degradation of system performance
> because the pages couldn't be swapped out so we limit user-desired userptr
> size to pre-defined.
>
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>


Again i would like feedback from mm people (adding cc). I am not sure
locking the vma is the right anwser as i said in my previous mail,
userspace can munlock it in your back, maybe VM_RESERVED is better.
Anyway even not considering that you don't check at all that process
don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK
for how it's done. Also you mlock complete vma but the userptr you get
might be inside say 16M vma and you only care about 1M of userptr, if
you mark the whole vma as locked than anytime a new page is fault in
the vma else where than in the buffer you are interested then it got
allocated for ever until the gem buffer is destroy, i am not sure of
what happen to the vma on next malloc if it grows or not (i would
think it won't grow at it would have different flags than new
anonymous memory).

The whole business of directly using malloced memory for gpu is fishy
and i would really like to get it right rather than relying on never
hitting strange things like page migration, vma merging, or worse
things like over locking pages and stealing memory.

Cheers,
Jerome


> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c |    2 +
>  drivers/gpu/drm/exynos/exynos_drm_gem.c |  334 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/exynos/exynos_drm_gem.h |   17 ++-
>  include/drm/exynos_drm.h                |   26 +++-
>  4 files changed, 376 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 1e68ec2..e8ae3f1 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -220,6 +220,8 @@ static struct drm_ioctl_desc exynos_ioctls[] = {
>        DRM_IOCTL_DEF_DRV(EXYNOS_GEM_MAP_OFFSET,
>                        exynos_drm_gem_map_offset_ioctl, DRM_UNLOCKED |
>                        DRM_AUTH),
> +       DRM_IOCTL_DEF_DRV(EXYNOS_GEM_USERPTR,
> +                       exynos_drm_gem_userptr_ioctl, DRM_UNLOCKED),
>        DRM_IOCTL_DEF_DRV(EXYNOS_GEM_MMAP,
>                        exynos_drm_gem_mmap_ioctl, DRM_UNLOCKED | DRM_AUTH),
>        DRM_IOCTL_DEF_DRV(EXYNOS_GEM_GET,
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index e6abb66..ccc6e3d 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -68,6 +68,83 @@ static int check_gem_flags(unsigned int flags)
>        return 0;
>  }
>
> +static struct vm_area_struct *get_vma(struct vm_area_struct *vma)
> +{
> +       struct vm_area_struct *vma_copy;
> +
> +       vma_copy = kmalloc(sizeof(*vma_copy), GFP_KERNEL);
> +       if (!vma_copy)
> +               return NULL;
> +
> +       if (vma->vm_ops && vma->vm_ops->open)
> +               vma->vm_ops->open(vma);
> +
> +       if (vma->vm_file)
> +               get_file(vma->vm_file);
> +
> +       memcpy(vma_copy, vma, sizeof(*vma));
> +
> +       vma_copy->vm_mm = NULL;
> +       vma_copy->vm_next = NULL;
> +       vma_copy->vm_prev = NULL;
> +
> +       return vma_copy;
> +}
> +
> +
> +static void put_vma(struct vm_area_struct *vma)
> +{
> +       if (!vma)
> +               return;
> +
> +       if (vma->vm_ops && vma->vm_ops->close)
> +               vma->vm_ops->close(vma);
> +
> +       if (vma->vm_file)
> +               fput(vma->vm_file);
> +
> +       kfree(vma);
> +}
> +
> +/*
> + * lock_userptr_vma - lock VMAs within user address space
> + *
> + * this function locks vma within user address space to avoid pages
> + * to the userspace from being swapped out.
> + * if this vma isn't locked, the pages to the userspace could be swapped out
> + * so unprivileged user might access different pages and dma of any device
> + * could access physical memory region not intended once swap-in.
> + */
> +static int lock_userptr_vma(struct exynos_drm_gem_buf *buf, unsigned int lock)
> +{
> +       struct vm_area_struct *vma;
> +       unsigned long start, end;
> +
> +       start = buf->userptr;
> +       end = buf->userptr + buf->size - 1;
> +
> +       down_write(&current->mm->mmap_sem);
> +
> +       do {
> +               vma = find_vma(current->mm, start);
> +               if (!vma) {
> +                       up_write(&current->mm->mmap_sem);
> +                       return -EFAULT;
> +               }
> +
> +               if (lock)
> +                       vma->vm_flags |= VM_LOCKED;
> +               else
> +                       vma->vm_flags &= ~VM_LOCKED;
> +
> +               start = vma->vm_end + 1;
> +       } while (vma->vm_end < end);
> +
> +       up_write(&current->mm->mmap_sem);
> +
> +       return 0;
> +}
> +
>  static void update_vm_cache_attr(struct exynos_drm_gem_obj *obj,
>                                        struct vm_area_struct *vma)
>  {
> @@ -256,6 +333,44 @@ static void exynos_drm_gem_put_pages(struct drm_gem_object *obj)
>        /* add some codes for UNCACHED type here. TODO */
>  }
>
> +static void exynos_drm_put_userptr(struct drm_gem_object *obj)
> +{
> +       struct exynos_drm_gem_obj *exynos_gem_obj;
> +       struct exynos_drm_gem_buf *buf;
> +       struct vm_area_struct *vma;
> +       int npages;
> +
> +       exynos_gem_obj = to_exynos_gem_obj(obj);
> +       buf = exynos_gem_obj->buffer;
> +       vma = exynos_gem_obj->vma;
> +
> +       if (vma && (vma->vm_flags & VM_PFNMAP) && (vma->vm_pgoff)) {
> +               put_vma(exynos_gem_obj->vma);
> +               goto out;
> +       }
> +
> +       npages = buf->size >> PAGE_SHIFT;
> +
> +       if (exynos_gem_obj->flags & EXYNOS_BO_USERPTR && !buf->pfnmap)
> +               lock_userptr_vma(buf, 0);
> +
> +       npages--;
> +       while (npages >= 0) {
> +               if (buf->write)
> +                       set_page_dirty_lock(buf->pages[npages]);
> +
> +               put_page(buf->pages[npages]);
> +               npages--;
> +       }
> +
> +out:
> +       kfree(buf->pages);
> +       buf->pages = NULL;
> +
> +       kfree(buf->sgt);
> +       buf->sgt = NULL;
> +}
> +
>  static int exynos_drm_gem_handle_create(struct drm_gem_object *obj,
>                                        struct drm_file *file_priv,
>                                        unsigned int *handle)
> @@ -295,6 +410,8 @@ void exynos_drm_gem_destroy(struct exynos_drm_gem_obj *exynos_gem_obj)
>
>        if (exynos_gem_obj->flags & EXYNOS_BO_NONCONTIG)
>                exynos_drm_gem_put_pages(obj);
> +       else if (exynos_gem_obj->flags & EXYNOS_BO_USERPTR)
> +               exynos_drm_put_userptr(obj);
>        else
>                exynos_drm_free_buf(obj->dev, exynos_gem_obj->flags, buf);
>
> @@ -606,6 +723,223 @@ int exynos_drm_gem_mmap_ioctl(struct drm_device *dev, void *data,
>        return 0;
>  }
>
> +static int exynos_drm_get_userptr(struct drm_device *dev,
> +                               struct exynos_drm_gem_obj *obj,
> +                               unsigned long userptr,
> +                               unsigned int write)
> +{
> +       unsigned int get_npages;
> +       unsigned long npages = 0;
> +       struct vm_area_struct *vma;
> +       struct exynos_drm_gem_buf *buf = obj->buffer;
> +       int ret;
> +
> +       down_read(&current->mm->mmap_sem);
> +       vma = find_vma(current->mm, userptr);
> +
> +       /* the memory region mmaped with VM_PFNMAP. */
> +       if (vma && (vma->vm_flags & VM_PFNMAP) && (vma->vm_pgoff)) {
> +               unsigned long this_pfn, prev_pfn, pa;
> +               unsigned long start, end, offset;
> +               struct scatterlist *sgl;
> +               int ret;
> +
> +               start = userptr;
> +               offset = userptr & ~PAGE_MASK;
> +               end = start + buf->size;
> +               sgl = buf->sgt->sgl;
> +
> +               for (prev_pfn = 0; start < end; start += PAGE_SIZE) {
> +                       ret = follow_pfn(vma, start, &this_pfn);
> +                       if (ret)
> +                               goto err;
> +
> +                       if (prev_pfn == 0) {
> +                               pa = this_pfn << PAGE_SHIFT;
> +                               buf->dma_addr = pa + offset;
> +                       } else if (this_pfn != prev_pfn + 1) {
> +                               ret = -EINVAL;
> +                               goto err;
> +                       }
> +
> +                       sg_dma_address(sgl) = (pa + offset);
> +                       sg_dma_len(sgl) = PAGE_SIZE;
> +                       prev_pfn = this_pfn;
> +                       pa += PAGE_SIZE;
> +                       npages++;
> +                       sgl = sg_next(sgl);
> +               }
> +
> +               obj->vma = get_vma(vma);
> +               if (!obj->vma) {
> +                       ret = -ENOMEM;
> +                       goto err;
> +               }
> +
> +               up_read(&current->mm->mmap_sem);
> +               buf->pfnmap = true;
> +
> +               return npages;
> +err:
> +               buf->dma_addr = 0;
> +               up_read(&current->mm->mmap_sem);
> +
> +               return ret;
> +       }
> +
> +       up_read(&current->mm->mmap_sem);
> +
> +       /*
> +        * lock the vma within userptr to avoid userspace buffer
> +        * from being swapped out.
> +        */
> +       ret = lock_userptr_vma(buf, 1);
> +       if (ret < 0) {
> +               DRM_ERROR("failed to lock vma for userptr.\n");
> +               lock_userptr_vma(buf, 0);
> +               return 0;
> +       }
> +
> +       buf->write = write;
> +       npages = buf->size >> PAGE_SHIFT;
> +
> +       down_read(&current->mm->mmap_sem);
> +       get_npages = get_user_pages(current, current->mm, userptr,
> +                                       npages, write, 1, buf->pages, NULL);
> +       up_read(&current->mm->mmap_sem);
> +       if (get_npages != npages)
> +               DRM_ERROR("failed to get user_pages.\n");
> +
> +       buf->userptr = userptr;
> +       buf->pfnmap = false;
> +
> +       return get_npages;
> +}
> +
> +int exynos_drm_gem_userptr_ioctl(struct drm_device *dev, void *data,
> +                                     struct drm_file *file_priv)
> +{
> +       struct exynos_drm_private *priv = dev->dev_private;
> +       struct exynos_drm_gem_obj *exynos_gem_obj;
> +       struct drm_exynos_gem_userptr *args = data;
> +       struct exynos_drm_gem_buf *buf;
> +       struct scatterlist *sgl;
> +       unsigned long size, userptr;
> +       unsigned int npages;
> +       int ret, get_npages;
> +
> +       DRM_DEBUG_KMS("%s\n", __FILE__);
> +
> +       if (!args->size) {
> +               DRM_ERROR("invalid size.\n");
> +               return -EINVAL;
> +       }
> +
> +       ret = check_gem_flags(args->flags);
> +       if (ret)
> +               return ret;
> +
> +       size = roundup_gem_size(args->size, EXYNOS_BO_USERPTR);
> +
> +       if (size > priv->userptr_limit) {
> +               DRM_ERROR("excessed maximum size of userptr.\n");
> +               return -EINVAL;
> +       }
> +
> +       userptr = args->userptr;
> +
> +       buf = exynos_drm_init_buf(dev, size);
> +       if (!buf)
> +               return -ENOMEM;
> +
> +       exynos_gem_obj = exynos_drm_gem_init(dev, size);
> +       if (!exynos_gem_obj) {
> +               ret = -ENOMEM;
> +               goto err_free_buffer;
> +       }
> +
> +       buf->sgt = kzalloc(sizeof(struct sg_table), GFP_KERNEL);
> +       if (!buf->sgt) {
> +               DRM_ERROR("failed to allocate buf->sgt.\n");
> +               ret = -ENOMEM;
> +               goto err_release_gem;
> +       }
> +
> +       npages = size >> PAGE_SHIFT;
> +
> +       ret = sg_alloc_table(buf->sgt, npages, GFP_KERNEL);
> +       if (ret < 0) {
> +               DRM_ERROR("failed to initailize sg table.\n");
> +               goto err_free_sgt;
> +       }
> +
> +       buf->pages = kzalloc(npages * sizeof(struct page *), GFP_KERNEL);
> +       if (!buf->pages) {
> +               DRM_ERROR("failed to allocate buf->pages\n");
> +               ret = -ENOMEM;
> +               goto err_free_table;
> +       }
> +
> +       exynos_gem_obj->buffer = buf;
> +
> +       get_npages = exynos_drm_get_userptr(dev, exynos_gem_obj, userptr, 1);
> +       if (get_npages != npages) {
> +               DRM_ERROR("failed to get user_pages.\n");
> +               ret = get_npages;
> +               goto err_release_userptr;
> +       }
> +
> +       ret = exynos_drm_gem_handle_create(&exynos_gem_obj->base, file_priv,
> +                                               &args->handle);
> +       if (ret < 0) {
> +               DRM_ERROR("failed to create gem handle.\n");
> +               goto err_release_userptr;
> +       }
> +
> +       sgl = buf->sgt->sgl;
> +
> +       /*
> +        * if buf->pfnmap is true then update sgl of sgt with pages but
> +        * if buf->pfnmap is false then it means the sgl was updated already
> +        * so it doesn't need to update the sgl.
> +        */
> +       if (!buf->pfnmap) {
> +               unsigned int i = 0;
> +
> +               /* set all pages to sg list. */
> +               while (i < npages) {
> +                       sg_set_page(sgl, buf->pages[i], PAGE_SIZE, 0);
> +                       sg_dma_address(sgl) = page_to_phys(buf->pages[i]);
> +                       i++;
> +                       sgl = sg_next(sgl);
> +               }
> +       }
> +
> +       /* always use EXYNOS_BO_USERPTR as memory type for userptr. */
> +       exynos_gem_obj->flags |= EXYNOS_BO_USERPTR;
> +
> +       return 0;
> +
> +err_release_userptr:
> +       get_npages--;
> +       while (get_npages >= 0)
> +               put_page(buf->pages[get_npages--]);
> +       kfree(buf->pages);
> +       buf->pages = NULL;
> +err_free_table:
> +       sg_free_table(buf->sgt);
> +err_free_sgt:
> +       kfree(buf->sgt);
> +       buf->sgt = NULL;
> +err_release_gem:
> +       drm_gem_object_release(&exynos_gem_obj->base);
> +       kfree(exynos_gem_obj);
> +       exynos_gem_obj = NULL;
> +err_free_buffer:
> +       exynos_drm_free_buf(dev, 0, buf);
> +       return ret;
> +}
> +
>  int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data,
>                                      struct drm_file *file_priv)
>  {      struct exynos_drm_gem_obj *exynos_gem_obj;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> index 3334c9f..72bd993 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> @@ -29,27 +29,35 @@
>  #define to_exynos_gem_obj(x)   container_of(x,\
>                        struct exynos_drm_gem_obj, base)
>
> -#define IS_NONCONTIG_BUFFER(f)         (f & EXYNOS_BO_NONCONTIG)
> +#define IS_NONCONTIG_BUFFER(f) ((f & EXYNOS_BO_NONCONTIG) ||\
> +                                       (f & EXYNOS_BO_USERPTR))
>
>  /*
>  * exynos drm gem buffer structure.
>  *
>  * @kvaddr: kernel virtual address to allocated memory region.
> + * @userptr: user space address.
>  * @dma_addr: bus address(accessed by dma) to allocated memory region.
>  *     - this address could be physical address without IOMMU and
>  *     device address with IOMMU.
> + * @write: whether pages will be written to by the caller.
>  * @sgt: sg table to transfer page data.
>  * @pages: contain all pages to allocated memory region.
>  * @page_size: could be 4K, 64K or 1MB.
>  * @size: size of allocated memory region.
> + * @pfnmap: indicate whether memory region from userptr is mmaped with
> + *     VM_PFNMAP or not.
>  */
>  struct exynos_drm_gem_buf {
>        void __iomem            *kvaddr;
> +       unsigned long           userptr;
>        dma_addr_t              dma_addr;
> +       unsigned int            write;
>        struct sg_table         *sgt;
>        struct page             **pages;
>        unsigned long           page_size;
>        unsigned long           size;
> +       bool                    pfnmap;
>  };
>
>  /*
> @@ -64,6 +72,8 @@ struct exynos_drm_gem_buf {
>  *     continuous memory region allocated by user request
>  *     or at framebuffer creation.
>  * @size: total memory size to physically non-continuous memory region.
> + * @vma: a pointer to the vma to user address space and used to release
> + *     the pages to user space.
>  * @flags: indicate memory type to allocated buffer and cache attruibute.
>  *
>  * P.S. this object would be transfered to user as kms_bo.handle so
> @@ -73,6 +83,7 @@ struct exynos_drm_gem_obj {
>        struct drm_gem_object           base;
>        struct exynos_drm_gem_buf       *buffer;
>        unsigned long                   size;
> +       struct vm_area_struct           *vma;
>        unsigned int                    flags;
>  };
>
> @@ -130,6 +141,10 @@ int exynos_drm_gem_map_offset_ioctl(struct drm_device *dev, void *data,
>  int exynos_drm_gem_mmap_ioctl(struct drm_device *dev, void *data,
>                              struct drm_file *file_priv);
>
> +/* map user space allocated by malloc to pages. */
> +int exynos_drm_gem_userptr_ioctl(struct drm_device *dev, void *data,
> +                                     struct drm_file *file_priv);
> +
>  /* get buffer information to memory region allocated by gem. */
>  int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data,
>                                      struct drm_file *file_priv);
> diff --git a/include/drm/exynos_drm.h b/include/drm/exynos_drm.h
> index 52465dc..33fa1e2 100644
> --- a/include/drm/exynos_drm.h
> +++ b/include/drm/exynos_drm.h
> @@ -77,6 +77,23 @@ struct drm_exynos_gem_mmap {
>  };
>
>  /**
> + * User-requested user space importing structure
> + *
> + * @userptr: user space address allocated by malloc.
> + * @size: size to the buffer allocated by malloc.
> + * @flags: indicate user-desired cache attribute to map the allocated buffer
> + *     to kernel space.
> + * @handle: a returned handle to created gem object.
> + *     - this handle will be set by gem module of kernel side.
> + */
> +struct drm_exynos_gem_userptr {
> +       uint64_t userptr;
> +       uint64_t size;
> +       unsigned int flags;
> +       unsigned int handle;
> +};
> +
> +/**
>  * A structure to gem information.
>  *
>  * @handle: a handle to gem object created.
> @@ -135,8 +152,10 @@ enum e_drm_exynos_gem_mem_type {
>        EXYNOS_BO_CACHABLE      = 1 << 1,
>        /* write-combine mapping. */
>        EXYNOS_BO_WC            = 1 << 2,
> +       /* user space memory allocated by malloc. */
> +       EXYNOS_BO_USERPTR       = 1 << 3,
>        EXYNOS_BO_MASK          = EXYNOS_BO_NONCONTIG | EXYNOS_BO_CACHABLE |
> -                                       EXYNOS_BO_WC
> +                                       EXYNOS_BO_WC | EXYNOS_BO_USERPTR
>  };
>
>  struct drm_exynos_g2d_get_ver {
> @@ -173,7 +192,7 @@ struct drm_exynos_g2d_exec {
>  #define DRM_EXYNOS_GEM_CREATE          0x00
>  #define DRM_EXYNOS_GEM_MAP_OFFSET      0x01
>  #define DRM_EXYNOS_GEM_MMAP            0x02
> -/* Reserved 0x03 ~ 0x05 for exynos specific gem ioctl */
> +#define DRM_EXYNOS_GEM_USERPTR         0x03
>  #define DRM_EXYNOS_GEM_GET             0x04
>  #define DRM_EXYNOS_USER_LIMIT          0x05
>  #define DRM_EXYNOS_PLANE_SET_ZPOS      0x06
> @@ -193,6 +212,9 @@ struct drm_exynos_g2d_exec {
>  #define DRM_IOCTL_EXYNOS_GEM_MMAP      DRM_IOWR(DRM_COMMAND_BASE + \
>                DRM_EXYNOS_GEM_MMAP, struct drm_exynos_gem_mmap)
>
> +#define DRM_IOCTL_EXYNOS_GEM_USERPTR   DRM_IOWR(DRM_COMMAND_BASE + \
> +               DRM_EXYNOS_GEM_USERPTR, struct drm_exynos_gem_userptr)
> +
>  #define DRM_IOCTL_EXYNOS_GEM_GET       DRM_IOWR(DRM_COMMAND_BASE + \
>                DRM_EXYNOS_GEM_GET,     struct drm_exynos_gem_info)
>
> --
> 1.7.4.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Jerome Glisse]

On Wed, May 9, 2012 at 10:45 AM, Jerome Glisse <j.glisse@gmail.com> wrote:
> On Wed, May 9, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com> wrote:
>> this feature is used to import user space region allocated by malloc() or
>> mmaped into a gem. and to guarantee the pages to user space not to be
>> swapped out, the VMAs within the user space would be locked and then unlocked
>> when the pages are released.
>>
>> but this lock might result in significant degradation of system performance
>> because the pages couldn't be swapped out so we limit user-desired userptr
>> size to pre-defined.
>>
>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>
>
> Again i would like feedback from mm people (adding cc). I am not sure
> locking the vma is the right anwser as i said in my previous mail,
> userspace can munlock it in your back, maybe VM_RESERVED is better.
> Anyway even not considering that you don't check at all that process
> don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK
> for how it's done. Also you mlock complete vma but the userptr you get
> might be inside say 16M vma and you only care about 1M of userptr, if
> you mark the whole vma as locked than anytime a new page is fault in
> the vma else where than in the buffer you are interested then it got
> allocated for ever until the gem buffer is destroy, i am not sure of
> what happen to the vma on next malloc if it grows or not (i would
> think it won't grow at it would have different flags than new
> anonymous memory).
>
> The whole business of directly using malloced memory for gpu is fishy
> and i would really like to get it right rather than relying on never
> hitting strange things like page migration, vma merging, or worse
> things like over locking pages and stealing memory.
>
> Cheers,
> Jerome

I had a lengthy discussion with mm people (thx a lot for that). I
think we should split 2 different use case. The zero-copy upload case
ie :
app:
    ptr = malloc()
    ...
    glTex/VBO/UBO/...(ptr)
    free(ptr) or reuse it for other things
For which i guess you want to avoid having to do a memcpy inside the
gl library (could be anything else than gl that have same useage
pattern).

ie after the upload happen you don't care about those page they can
removed from the vma or marked as cow so that anything messing with
those page after the upload won't change what you uploaded. Of course
this is assuming that the tlb cost of doing such thing is smaller than
the cost of memcpy the data.

Two way to do that, either you assume app can't not read back data
after gl can and you do an unmap_mapping_range (make sure you only
unmap fully covered page and that you copy non fully covered page) or
you want to allow userspace to still read data or possibly overwrite
them

Second use case is something more like for the opencl case of
CL_MEM_USE_HOST_PTR, in which you want to use the same page in the gpu
and keep the userspace vma pointing to those page. I think the
agreement on this case is that there is no way right now to do it
sanely inside linux kernel. mlocking will need proper accounting
against rtlimit but this limit might be low. Also the fork case might
be problematic.

For the fork case the memory is anonymous so it should be COWed in the
fork child but relative to cl context that means the child could not
use the cl context with that memory or at least if the child write to
this memory the cl will not see those change. I guess the answer to
that one is that you really need to use the cl api to read the object
or get proper ptr to read it.

Anyway in all case, implementing this userptr thing need a lot more
code. You have to check to that the vma you are trying to use is
anonymous and only handle this case and fallback to alloc new page and
copy otherwise..

Cheers,
Jerome

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Inki Dae]

Hi Jerome,

Thank you again.

> -----Original Message-----
> From: Jerome Glisse [mailto:j.glisse@gmail.com]
> Sent: Thursday, May 10, 2012 3:33 AM
> To: Inki Dae
> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
> 
> On Wed, May 9, 2012 at 10:45 AM, Jerome Glisse <j.glisse@gmail.com> wrote:
> > On Wed, May 9, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com> wrote:
> >> this feature is used to import user space region allocated by malloc()
> or
> >> mmaped into a gem. and to guarantee the pages to user space not to be
> >> swapped out, the VMAs within the user space would be locked and then
> unlocked
> >> when the pages are released.
> >>
> >> but this lock might result in significant degradation of system
> performance
> >> because the pages couldn't be swapped out so we limit user-desired
> userptr
> >> size to pre-defined.
> >>
> >> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >
> >
> > Again i would like feedback from mm people (adding cc). I am not sure
> > locking the vma is the right anwser as i said in my previous mail,
> > userspace can munlock it in your back, maybe VM_RESERVED is better.
> > Anyway even not considering that you don't check at all that process
> > don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK
> > for how it's done. Also you mlock complete vma but the userptr you get
> > might be inside say 16M vma and you only care about 1M of userptr, if
> > you mark the whole vma as locked than anytime a new page is fault in
> > the vma else where than in the buffer you are interested then it got
> > allocated for ever until the gem buffer is destroy, i am not sure of
> > what happen to the vma on next malloc if it grows or not (i would
> > think it won't grow at it would have different flags than new
> > anonymous memory).
> >
> > The whole business of directly using malloced memory for gpu is fishy
> > and i would really like to get it right rather than relying on never
> > hitting strange things like page migration, vma merging, or worse
> > things like over locking pages and stealing memory.
> >
> > Cheers,
> > Jerome
> 
> I had a lengthy discussion with mm people (thx a lot for that). I
> think we should split 2 different use case. The zero-copy upload case
> ie :
> app:
>     ptr = malloc()
>     ...
>     glTex/VBO/UBO/...(ptr)
>     free(ptr) or reuse it for other things
> For which i guess you want to avoid having to do a memcpy inside the
> gl library (could be anything else than gl that have same useage
> pattern).
> 

Right, in this case, we are using the userptr feature as pixman and evas
backend to use 2d accelerator.

> ie after the upload happen you don't care about those page they can
> removed from the vma or marked as cow so that anything messing with
> those page after the upload won't change what you uploaded. Of course

I'm not sure that I understood your mentions but could the pages be removed
from vma with VM_LOCKED or VM_RESERVED? once glTex/VBO/UBO/..., the VMAs to
user space would be locked. if cpu accessed significant part of all the
pages in user mode then pages to the part would be allocated by page fault
handler, after that, through userptr, the VMAs to user address space would
be locked(at this time, the remaining pages would be allocated also by
get_user_pages by calling page fault handler) I'd be glad to give me any
comments and advices if there is my missing point.

> this is assuming that the tlb cost of doing such thing is smaller than
> the cost of memcpy the data.
> 

yes, in our test case, the tlb cost(incurred by tlb miss) was smaller than
the cost of memcpy also cpu usage. of course, this would be depended on gpu
performance.

> Two way to do that, either you assume app can't not read back data
> after gl can and you do an unmap_mapping_range (make sure you only
> unmap fully covered page and that you copy non fully covered page) or
> you want to allow userspace to still read data or possibly overwrite
> them
> 
> Second use case is something more like for the opencl case of
> CL_MEM_USE_HOST_PTR, in which you want to use the same page in the gpu
> and keep the userspace vma pointing to those page. I think the
> agreement on this case is that there is no way right now to do it
> sanely inside linux kernel. mlocking will need proper accounting
> against rtlimit but this limit might be low. Also the fork case might
> be problematic.
> 
> For the fork case the memory is anonymous so it should be COWed in the
> fork child but relative to cl context that means the child could not
> use the cl context with that memory or at least if the child write to
> this memory the cl will not see those change. I guess the answer to
> that one is that you really need to use the cl api to read the object
> or get proper ptr to read it.
> 
> Anyway in all case, implementing this userptr thing need a lot more
> code. You have to check to that the vma you are trying to use is
> anonymous and only handle this case and fallback to alloc new page and
> copy otherwise..
> 

I'd like to say thank you again you gave me comments and advices in detail.
there may be my missing points but I will check it again.

Thanks,
Inki Dae

> Cheers,
> Jerome

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Jerome Glisse]

On Wed, May 9, 2012 at 10:44 PM, Inki Dae <inki.dae@samsung.com> wrote:
> Hi Jerome,
>
> Thank you again.
>
>> -----Original Message-----
>> From: Jerome Glisse [mailto:j.glisse@gmail.com]
>> Sent: Thursday, May 10, 2012 3:33 AM
>> To: Inki Dae
>> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
>> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
>> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
>>
>> On Wed, May 9, 2012 at 10:45 AM, Jerome Glisse <j.glisse@gmail.com> wrote:
>> > On Wed, May 9, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com> wrote:
>> >> this feature is used to import user space region allocated by malloc()
>> or
>> >> mmaped into a gem. and to guarantee the pages to user space not to be
>> >> swapped out, the VMAs within the user space would be locked and then
>> unlocked
>> >> when the pages are released.
>> >>
>> >> but this lock might result in significant degradation of system
>> performance
>> >> because the pages couldn't be swapped out so we limit user-desired
>> userptr
>> >> size to pre-defined.
>> >>
>> >> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> >
>> >
>> > Again i would like feedback from mm people (adding cc). I am not sure
>> > locking the vma is the right anwser as i said in my previous mail,
>> > userspace can munlock it in your back, maybe VM_RESERVED is better.
>> > Anyway even not considering that you don't check at all that process
>> > don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK
>> > for how it's done. Also you mlock complete vma but the userptr you get
>> > might be inside say 16M vma and you only care about 1M of userptr, if
>> > you mark the whole vma as locked than anytime a new page is fault in
>> > the vma else where than in the buffer you are interested then it got
>> > allocated for ever until the gem buffer is destroy, i am not sure of
>> > what happen to the vma on next malloc if it grows or not (i would
>> > think it won't grow at it would have different flags than new
>> > anonymous memory).
>> >
>> > The whole business of directly using malloced memory for gpu is fishy
>> > and i would really like to get it right rather than relying on never
>> > hitting strange things like page migration, vma merging, or worse
>> > things like over locking pages and stealing memory.
>> >
>> > Cheers,
>> > Jerome
>>
>> I had a lengthy discussion with mm people (thx a lot for that). I
>> think we should split 2 different use case. The zero-copy upload case
>> ie :
>> app:
>>     ptr = malloc()
>>     ...
>>     glTex/VBO/UBO/...(ptr)
>>     free(ptr) or reuse it for other things
>> For which i guess you want to avoid having to do a memcpy inside the
>> gl library (could be anything else than gl that have same useage
>> pattern).
>>
>
> Right, in this case, we are using the userptr feature as pixman and evas
> backend to use 2d accelerator.
>
>> ie after the upload happen you don't care about those page they can
>> removed from the vma or marked as cow so that anything messing with
>> those page after the upload won't change what you uploaded. Of course
>
> I'm not sure that I understood your mentions but could the pages be removed
> from vma with VM_LOCKED or VM_RESERVED? once glTex/VBO/UBO/..., the VMAs to
> user space would be locked. if cpu accessed significant part of all the
> pages in user mode then pages to the part would be allocated by page fault
> handler, after that, through userptr, the VMAs to user address space would
> be locked(at this time, the remaining pages would be allocated also by
> get_user_pages by calling page fault handler) I'd be glad to give me any
> comments and advices if there is my missing point.
>
>> this is assuming that the tlb cost of doing such thing is smaller than
>> the cost of memcpy the data.
>>
>
> yes, in our test case, the tlb cost(incurred by tlb miss) was smaller than
> the cost of memcpy also cpu usage. of course, this would be depended on gpu
> performance.
>
>> Two way to do that, either you assume app can't not read back data
>> after gl can and you do an unmap_mapping_range (make sure you only
>> unmap fully covered page and that you copy non fully covered page) or
>> you want to allow userspace to still read data or possibly overwrite
>> them
>>
>> Second use case is something more like for the opencl case of
>> CL_MEM_USE_HOST_PTR, in which you want to use the same page in the gpu
>> and keep the userspace vma pointing to those page. I think the
>> agreement on this case is that there is no way right now to do it
>> sanely inside linux kernel. mlocking will need proper accounting
>> against rtlimit but this limit might be low. Also the fork case might
>> be problematic.
>>
>> For the fork case the memory is anonymous so it should be COWed in the
>> fork child but relative to cl context that means the child could not
>> use the cl context with that memory or at least if the child write to
>> this memory the cl will not see those change. I guess the answer to
>> that one is that you really need to use the cl api to read the object
>> or get proper ptr to read it.
>>
>> Anyway in all case, implementing this userptr thing need a lot more
>> code. You have to check to that the vma you are trying to use is
>> anonymous and only handle this case and fallback to alloc new page and
>> copy otherwise..
>>
>
> I'd like to say thank you again you gave me comments and advices in detail.
> there may be my missing points but I will check it again.
>
> Thanks,
> Inki Dae
>
>> Cheers,
>> Jerome
>

I think to sumup things there is 2 use case:
1: we want to steal anonymous page and move them to a gem/gpu object.
So call get_user_pages on the rand and then unmap_mapping_range on the
range we want to still page are the function you would want. That
assume of course that the api case your are trying to accelerate is ok
with having the user process loosing the content of this buffer. If
it's not the other solution is to mark the range as COW and steal the
page, if userspace try to write something new to the range it will get
new page (from copy of old one).

Let call it  drm_gem_from_userptr

2: you want to be able to use malloced area over long period of time
(several minute) with the gpu and you want that userspace vma still
point to those page. It the most problematic case, as i said just
mlocking the vma is troublesome as malicious userspace can munlock it
or try to abuse you to mlock too much. I believe right now there is no
good way to handle this case. That means that page can't be swaped out
or moved as regular anonymous page but on fork or exec this area still
need to behave like an anonymous vma.

Let call drm_gem_backed_by_userptr

Inki i really think you should split this 2 usecase, and do only the
drm_gem_from_userptr if it's enough for what you are trying to do. As
the second case look that too many things can go wrong.

Cheers,
Jerome

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Daniel Vetter]

On Thu, May 10, 2012 at 11:05:07AM -0400, Jerome Glisse wrote:
> On Wed, May 9, 2012 at 10:44 PM, Inki Dae <inki.dae@samsung.com> wrote:
> > Hi Jerome,
> >
> > Thank you again.
> >
> >> -----Original Message-----
> >> From: Jerome Glisse [mailto:j.glisse@gmail.com]
> >> Sent: Thursday, May 10, 2012 3:33 AM
> >> To: Inki Dae
> >> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
> >> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
> >> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
> >>
> >> On Wed, May 9, 2012 at 10:45 AM, Jerome Glisse <j.glisse@gmail.com> wrote:
> >> > On Wed, May 9, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com> wrote:
> >> >> this feature is used to import user space region allocated by malloc()
> >> or
> >> >> mmaped into a gem. and to guarantee the pages to user space not to be
> >> >> swapped out, the VMAs within the user space would be locked and then
> >> unlocked
> >> >> when the pages are released.
> >> >>
> >> >> but this lock might result in significant degradation of system
> >> performance
> >> >> because the pages couldn't be swapped out so we limit user-desired
> >> userptr
> >> >> size to pre-defined.
> >> >>
> >> >> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> >> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> >
> >> >
> >> > Again i would like feedback from mm people (adding cc). I am not sure
> >> > locking the vma is the right anwser as i said in my previous mail,
> >> > userspace can munlock it in your back, maybe VM_RESERVED is better.
> >> > Anyway even not considering that you don't check at all that process
> >> > don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK
> >> > for how it's done. Also you mlock complete vma but the userptr you get
> >> > might be inside say 16M vma and you only care about 1M of userptr, if
> >> > you mark the whole vma as locked than anytime a new page is fault in
> >> > the vma else where than in the buffer you are interested then it got
> >> > allocated for ever until the gem buffer is destroy, i am not sure of
> >> > what happen to the vma on next malloc if it grows or not (i would
> >> > think it won't grow at it would have different flags than new
> >> > anonymous memory).
> >> >
> >> > The whole business of directly using malloced memory for gpu is fishy
> >> > and i would really like to get it right rather than relying on never
> >> > hitting strange things like page migration, vma merging, or worse
> >> > things like over locking pages and stealing memory.
> >> >
> >> > Cheers,
> >> > Jerome
> >>
> >> I had a lengthy discussion with mm people (thx a lot for that). I
> >> think we should split 2 different use case. The zero-copy upload case
> >> ie :
> >> app:
> >>     ptr = malloc()
> >>     ...
> >>     glTex/VBO/UBO/...(ptr)
> >>     free(ptr) or reuse it for other things
> >> For which i guess you want to avoid having to do a memcpy inside the
> >> gl library (could be anything else than gl that have same useage
> >> pattern).
> >>
> >
> > Right, in this case, we are using the userptr feature as pixman and evas
> > backend to use 2d accelerator.
> >
> >> ie after the upload happen you don't care about those page they can
> >> removed from the vma or marked as cow so that anything messing with
> >> those page after the upload won't change what you uploaded. Of course
> >
> > I'm not sure that I understood your mentions but could the pages be removed
> > from vma with VM_LOCKED or VM_RESERVED? once glTex/VBO/UBO/..., the VMAs to
> > user space would be locked. if cpu accessed significant part of all the
> > pages in user mode then pages to the part would be allocated by page fault
> > handler, after that, through userptr, the VMAs to user address space would
> > be locked(at this time, the remaining pages would be allocated also by
> > get_user_pages by calling page fault handler) I'd be glad to give me any
> > comments and advices if there is my missing point.
> >
> >> this is assuming that the tlb cost of doing such thing is smaller than
> >> the cost of memcpy the data.
> >>
> >
> > yes, in our test case, the tlb cost(incurred by tlb miss) was smaller than
> > the cost of memcpy also cpu usage. of course, this would be depended on gpu
> > performance.
> >
> >> Two way to do that, either you assume app can't not read back data
> >> after gl can and you do an unmap_mapping_range (make sure you only
> >> unmap fully covered page and that you copy non fully covered page) or
> >> you want to allow userspace to still read data or possibly overwrite
> >> them
> >>
> >> Second use case is something more like for the opencl case of
> >> CL_MEM_USE_HOST_PTR, in which you want to use the same page in the gpu
> >> and keep the userspace vma pointing to those page. I think the
> >> agreement on this case is that there is no way right now to do it
> >> sanely inside linux kernel. mlocking will need proper accounting
> >> against rtlimit but this limit might be low. Also the fork case might
> >> be problematic.
> >>
> >> For the fork case the memory is anonymous so it should be COWed in the
> >> fork child but relative to cl context that means the child could not
> >> use the cl context with that memory or at least if the child write to
> >> this memory the cl will not see those change. I guess the answer to
> >> that one is that you really need to use the cl api to read the object
> >> or get proper ptr to read it.
> >>
> >> Anyway in all case, implementing this userptr thing need a lot more
> >> code. You have to check to that the vma you are trying to use is
> >> anonymous and only handle this case and fallback to alloc new page and
> >> copy otherwise..
> >>
> >
> > I'd like to say thank you again you gave me comments and advices in detail.
> > there may be my missing points but I will check it again.
> >
> > Thanks,
> > Inki Dae
> >
> >> Cheers,
> >> Jerome
> >
> 
> I think to sumup things there is 2 use case:
> 1: we want to steal anonymous page and move them to a gem/gpu object.
> So call get_user_pages on the rand and then unmap_mapping_range on the
> range we want to still page are the function you would want. That
> assume of course that the api case your are trying to accelerate is ok
> with having the user process loosing the content of this buffer. If
> it's not the other solution is to mark the range as COW and steal the
> page, if userspace try to write something new to the range it will get
> new page (from copy of old one).
> 
> Let call it  drm_gem_from_userptr
> 
> 2: you want to be able to use malloced area over long period of time
> (several minute) with the gpu and you want that userspace vma still
> point to those page. It the most problematic case, as i said just
> mlocking the vma is troublesome as malicious userspace can munlock it
> or try to abuse you to mlock too much. I believe right now there is no
> good way to handle this case. That means that page can't be swaped out
> or moved as regular anonymous page but on fork or exec this area still
> need to behave like an anonymous vma.
> 
> Let call drm_gem_backed_by_userptr
> 
> Inki i really think you should split this 2 usecase, and do only the
> drm_gem_from_userptr if it's enough for what you are trying to do. As
> the second case look that too many things can go wrong.

Jumping into the discussion late: Chris Wilson stitched together a userptr
feature for i915. Iirc he started with your 1st usecase but quickly
noticed that doing all this setup stuff (get_user_pages alone, he didn't
include your proposed cow trick) is too expensive and it's cheaper to just
upload things with the cpu.

So he needs to keep around these mappings for essentially forever to
amortize the setup cost, which boils down to your 2nd use-case. I've
refused to merge that code since, like you point out, too much stuff can
go wrong when we pin arbitrary ranges of userspace.

One example which would only affect ARM platforms is that this would
horribly break CMA: Userspace malloc is allocated as GFP_MOVEABLE, and CMA
relies on migrating moveable pages out of the CMA region to handle large
contigious allocations. Currently the page migrate code simply backs down
and waits a bit if it encounters a page locked by get_user_pages, assuming
that this is due to io and will complete shortly. If you hold onto such
pages for a long time, that retry will eventually fail and break CMA.

There are other problems affecting also desktop machines, but I've figured
I'll pick something that really hurts on arm ;-)

Yours, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Jerome Glisse]

On Thu, May 10, 2012 at 11:31 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, May 10, 2012 at 11:05:07AM -0400, Jerome Glisse wrote:
>> On Wed, May 9, 2012 at 10:44 PM, Inki Dae <inki.dae@samsung.com> wrote:
>> > Hi Jerome,
>> >
>> > Thank you again.
>> >
>> >> -----Original Message-----
>> >> From: Jerome Glisse [mailto:j.glisse@gmail.com]
>> >> Sent: Thursday, May 10, 2012 3:33 AM
>> >> To: Inki Dae
>> >> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
>> >> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
>> >> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
>> >>
>> >> On Wed, May 9, 2012 at 10:45 AM, Jerome Glisse <j.glisse@gmail.com> wrote:
>> >> > On Wed, May 9, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com> wrote:
>> >> >> this feature is used to import user space region allocated by malloc()
>> >> or
>> >> >> mmaped into a gem. and to guarantee the pages to user space not to be
>> >> >> swapped out, the VMAs within the user space would be locked and then
>> >> unlocked
>> >> >> when the pages are released.
>> >> >>
>> >> >> but this lock might result in significant degradation of system
>> >> performance
>> >> >> because the pages couldn't be swapped out so we limit user-desired
>> >> userptr
>> >> >> size to pre-defined.
>> >> >>
>> >> >> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>> >> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> >> >
>> >> >
>> >> > Again i would like feedback from mm people (adding cc). I am not sure
>> >> > locking the vma is the right anwser as i said in my previous mail,
>> >> > userspace can munlock it in your back, maybe VM_RESERVED is better.
>> >> > Anyway even not considering that you don't check at all that process
>> >> > don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK
>> >> > for how it's done. Also you mlock complete vma but the userptr you get
>> >> > might be inside say 16M vma and you only care about 1M of userptr, if
>> >> > you mark the whole vma as locked than anytime a new page is fault in
>> >> > the vma else where than in the buffer you are interested then it got
>> >> > allocated for ever until the gem buffer is destroy, i am not sure of
>> >> > what happen to the vma on next malloc if it grows or not (i would
>> >> > think it won't grow at it would have different flags than new
>> >> > anonymous memory).
>> >> >
>> >> > The whole business of directly using malloced memory for gpu is fishy
>> >> > and i would really like to get it right rather than relying on never
>> >> > hitting strange things like page migration, vma merging, or worse
>> >> > things like over locking pages and stealing memory.
>> >> >
>> >> > Cheers,
>> >> > Jerome
>> >>
>> >> I had a lengthy discussion with mm people (thx a lot for that). I
>> >> think we should split 2 different use case. The zero-copy upload case
>> >> ie :
>> >> app:
>> >>     ptr = malloc()
>> >>     ...
>> >>     glTex/VBO/UBO/...(ptr)
>> >>     free(ptr) or reuse it for other things
>> >> For which i guess you want to avoid having to do a memcpy inside the
>> >> gl library (could be anything else than gl that have same useage
>> >> pattern).
>> >>
>> >
>> > Right, in this case, we are using the userptr feature as pixman and evas
>> > backend to use 2d accelerator.
>> >
>> >> ie after the upload happen you don't care about those page they can
>> >> removed from the vma or marked as cow so that anything messing with
>> >> those page after the upload won't change what you uploaded. Of course
>> >
>> > I'm not sure that I understood your mentions but could the pages be removed
>> > from vma with VM_LOCKED or VM_RESERVED? once glTex/VBO/UBO/..., the VMAs to
>> > user space would be locked. if cpu accessed significant part of all the
>> > pages in user mode then pages to the part would be allocated by page fault
>> > handler, after that, through userptr, the VMAs to user address space would
>> > be locked(at this time, the remaining pages would be allocated also by
>> > get_user_pages by calling page fault handler) I'd be glad to give me any
>> > comments and advices if there is my missing point.
>> >
>> >> this is assuming that the tlb cost of doing such thing is smaller than
>> >> the cost of memcpy the data.
>> >>
>> >
>> > yes, in our test case, the tlb cost(incurred by tlb miss) was smaller than
>> > the cost of memcpy also cpu usage. of course, this would be depended on gpu
>> > performance.
>> >
>> >> Two way to do that, either you assume app can't not read back data
>> >> after gl can and you do an unmap_mapping_range (make sure you only
>> >> unmap fully covered page and that you copy non fully covered page) or
>> >> you want to allow userspace to still read data or possibly overwrite
>> >> them
>> >>
>> >> Second use case is something more like for the opencl case of
>> >> CL_MEM_USE_HOST_PTR, in which you want to use the same page in the gpu
>> >> and keep the userspace vma pointing to those page. I think the
>> >> agreement on this case is that there is no way right now to do it
>> >> sanely inside linux kernel. mlocking will need proper accounting
>> >> against rtlimit but this limit might be low. Also the fork case might
>> >> be problematic.
>> >>
>> >> For the fork case the memory is anonymous so it should be COWed in the
>> >> fork child but relative to cl context that means the child could not
>> >> use the cl context with that memory or at least if the child write to
>> >> this memory the cl will not see those change. I guess the answer to
>> >> that one is that you really need to use the cl api to read the object
>> >> or get proper ptr to read it.
>> >>
>> >> Anyway in all case, implementing this userptr thing need a lot more
>> >> code. You have to check to that the vma you are trying to use is
>> >> anonymous and only handle this case and fallback to alloc new page and
>> >> copy otherwise..
>> >>
>> >
>> > I'd like to say thank you again you gave me comments and advices in detail.
>> > there may be my missing points but I will check it again.
>> >
>> > Thanks,
>> > Inki Dae
>> >
>> >> Cheers,
>> >> Jerome
>> >
>>
>> I think to sumup things there is 2 use case:
>> 1: we want to steal anonymous page and move them to a gem/gpu object.
>> So call get_user_pages on the rand and then unmap_mapping_range on the
>> range we want to still page are the function you would want. That
>> assume of course that the api case your are trying to accelerate is ok
>> with having the user process loosing the content of this buffer. If
>> it's not the other solution is to mark the range as COW and steal the
>> page, if userspace try to write something new to the range it will get
>> new page (from copy of old one).
>>
>> Let call it  drm_gem_from_userptr
>>
>> 2: you want to be able to use malloced area over long period of time
>> (several minute) with the gpu and you want that userspace vma still
>> point to those page. It the most problematic case, as i said just
>> mlocking the vma is troublesome as malicious userspace can munlock it
>> or try to abuse you to mlock too much. I believe right now there is no
>> good way to handle this case. That means that page can't be swaped out
>> or moved as regular anonymous page but on fork or exec this area still
>> need to behave like an anonymous vma.
>>
>> Let call drm_gem_backed_by_userptr
>>
>> Inki i really think you should split this 2 usecase, and do only the
>> drm_gem_from_userptr if it's enough for what you are trying to do. As
>> the second case look that too many things can go wrong.
>
> Jumping into the discussion late: Chris Wilson stitched together a userptr
> feature for i915. Iirc he started with your 1st usecase but quickly
> noticed that doing all this setup stuff (get_user_pages alone, he didn't
> include your proposed cow trick) is too expensive and it's cheaper to just
> upload things with the cpu.

I think use case 1 can still be usefull on desktop x86 but the object
need to be big something like bigger 16M or probably even bigger, so
that the cost of memcpy is bigger than the cost of tlb trashing. I am
sure than once we are seeing 1G dataset of opencl we will want to use
the stealing code path rather than memcpy things. Sadly API like CL
and GL don't make provision saying that the data ptr user supplied
might not have the content anymore so it makes the cow trick kind of
needed but it kills usecase such as :
scratch = malloc()
for (i =0; i<numtex; i++){
   readtexture(scratch, texfilename[i])
   glteximage(scratch)
}
free(scratch)

Or anything with similar access, here obviously the page backing the
scratch area can be stole at each glteximage call.

Anyway if you can define your api and provision that after call the
data you provided is no longer available then use case 1 sounds doable
and worth it to me.

> So he needs to keep around these mappings for essentially forever to
> amortize the setup cost, which boils down to your 2nd use-case. I've
> refused to merge that code since, like you point out, too much stuff can
> go wrong when we pin arbitrary ranges of userspace.
>
> One example which would only affect ARM platforms is that this would
> horribly break CMA: Userspace malloc is allocated as GFP_MOVEABLE, and CMA
> relies on migrating moveable pages out of the CMA region to handle large
> contigious allocations. Currently the page migrate code simply backs down
> and waits a bit if it encounters a page locked by get_user_pages, assuming
> that this is due to io and will complete shortly. If you hold onto such
> pages for a long time, that retry will eventually fail and break CMA.
>
> There are other problems affecting also desktop machines, but I've figured
> I'll pick something that really hurts on arm ;-)
>
> Yours, Daniel
> --
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48

Yes, what i also wanted to stress is that get_user_pages is not
enough, we are not doing something over the period of an ioctl in
which case everything is fine and doable, but are stealing page and
expect to use them at random point in the future with no kind of
synchronization with userspace. Anyway i think we agree that this
second use case is way complex and many things can go wrong, i still
think that for opencl we might want to able to do it but by them i am
expecting we will take advantage of iommu being able to pagefault from
process pagetable like the next AMD iommu can.

Cheers,
Jerome

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Inki Dae]

> -----Original Message-----
> From: Jerome Glisse [mailto:j.glisse@gmail.com]
> Sent: Friday, May 11, 2012 12:53 AM
> To: Jerome Glisse; Inki Dae; linux-mm@kvack.org;
kyungmin.park@samsung.com;
> sw0312.kim@samsung.com; dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
> 
> On Thu, May 10, 2012 at 11:31 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, May 10, 2012 at 11:05:07AM -0400, Jerome Glisse wrote:
> >> On Wed, May 9, 2012 at 10:44 PM, Inki Dae <inki.dae@samsung.com> wrote:
> >> > Hi Jerome,
> >> >
> >> > Thank you again.
> >> >
> >> >> -----Original Message-----
> >> >> From: Jerome Glisse [mailto:j.glisse@gmail.com]
> >> >> Sent: Thursday, May 10, 2012 3:33 AM
> >> >> To: Inki Dae
> >> >> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
> >> >> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-
> mm@kvack.org
> >> >> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
> >> >>
> >> >> On Wed, May 9, 2012 at 10:45 AM, Jerome Glisse <j.glisse@gmail.com>
> wrote:
> >> >> > On Wed, May 9, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com>
> wrote:
> >> >> >> this feature is used to import user space region allocated by
> malloc()
> >> >> or
> >> >> >> mmaped into a gem. and to guarantee the pages to user space not
> to be
> >> >> >> swapped out, the VMAs within the user space would be locked and
> then
> >> >> unlocked
> >> >> >> when the pages are released.
> >> >> >>
> >> >> >> but this lock might result in significant degradation of system
> >> >> performance
> >> >> >> because the pages couldn't be swapped out so we limit user-
> desired
> >> >> userptr
> >> >> >> size to pre-defined.
> >> >> >>
> >> >> >> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> >> >> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> >> >
> >> >> >
> >> >> > Again i would like feedback from mm people (adding cc). I am not
> sure
> >> >> > locking the vma is the right anwser as i said in my previous mail,
> >> >> > userspace can munlock it in your back, maybe VM_RESERVED is
better.
> >> >> > Anyway even not considering that you don't check at all that
> process
> >> >> > don't go over the limit of locked page see mm/mlock.c
> RLIMIT_MEMLOCK
> >> >> > for how it's done. Also you mlock complete vma but the userptr you
> get
> >> >> > might be inside say 16M vma and you only care about 1M of userptr,
> if
> >> >> > you mark the whole vma as locked than anytime a new page is fault
> in
> >> >> > the vma else where than in the buffer you are interested then it
> got
> >> >> > allocated for ever until the gem buffer is destroy, i am not sure
> of
> >> >> > what happen to the vma on next malloc if it grows or not (i would
> >> >> > think it won't grow at it would have different flags than new
> >> >> > anonymous memory).
> >> >> >
> >> >> > The whole business of directly using malloced memory for gpu is
> fishy
> >> >> > and i would really like to get it right rather than relying on
> never
> >> >> > hitting strange things like page migration, vma merging, or worse
> >> >> > things like over locking pages and stealing memory.
> >> >> >
> >> >> > Cheers,
> >> >> > Jerome
> >> >>
> >> >> I had a lengthy discussion with mm people (thx a lot for that). I
> >> >> think we should split 2 different use case. The zero-copy upload
> case
> >> >> ie :
> >> >> app:
> >> >>     ptr = malloc()
> >> >>     ...
> >> >>     glTex/VBO/UBO/...(ptr)
> >> >>     free(ptr) or reuse it for other things
> >> >> For which i guess you want to avoid having to do a memcpy inside the
> >> >> gl library (could be anything else than gl that have same useage
> >> >> pattern).
> >> >>
> >> >
> >> > Right, in this case, we are using the userptr feature as pixman and
> evas
> >> > backend to use 2d accelerator.
> >> >
> >> >> ie after the upload happen you don't care about those page they can
> >> >> removed from the vma or marked as cow so that anything messing with
> >> >> those page after the upload won't change what you uploaded. Of
> course
> >> >
> >> > I'm not sure that I understood your mentions but could the pages be
> removed
> >> > from vma with VM_LOCKED or VM_RESERVED? once glTex/VBO/UBO/..., the
> VMAs to
> >> > user space would be locked. if cpu accessed significant part of all
> the
> >> > pages in user mode then pages to the part would be allocated by page
> fault
> >> > handler, after that, through userptr, the VMAs to user address space
> would
> >> > be locked(at this time, the remaining pages would be allocated also
> by
> >> > get_user_pages by calling page fault handler) I'd be glad to give me
> any
> >> > comments and advices if there is my missing point.
> >> >
> >> >> this is assuming that the tlb cost of doing such thing is smaller
> than
> >> >> the cost of memcpy the data.
> >> >>
> >> >
> >> > yes, in our test case, the tlb cost(incurred by tlb miss) was smaller
> than
> >> > the cost of memcpy also cpu usage. of course, this would be depended
> on gpu
> >> > performance.
> >> >
> >> >> Two way to do that, either you assume app can't not read back data
> >> >> after gl can and you do an unmap_mapping_range (make sure you only
> >> >> unmap fully covered page and that you copy non fully covered page)
> or
> >> >> you want to allow userspace to still read data or possibly overwrite
> >> >> them
> >> >>
> >> >> Second use case is something more like for the opencl case of
> >> >> CL_MEM_USE_HOST_PTR, in which you want to use the same page in the
> gpu
> >> >> and keep the userspace vma pointing to those page. I think the
> >> >> agreement on this case is that there is no way right now to do it
> >> >> sanely inside linux kernel. mlocking will need proper accounting
> >> >> against rtlimit but this limit might be low. Also the fork case
> might
> >> >> be problematic.
> >> >>
> >> >> For the fork case the memory is anonymous so it should be COWed in
> the
> >> >> fork child but relative to cl context that means the child could not
> >> >> use the cl context with that memory or at least if the child write
> to
> >> >> this memory the cl will not see those change. I guess the answer to
> >> >> that one is that you really need to use the cl api to read the
> object
> >> >> or get proper ptr to read it.
> >> >>
> >> >> Anyway in all case, implementing this userptr thing need a lot more
> >> >> code. You have to check to that the vma you are trying to use is
> >> >> anonymous and only handle this case and fallback to alloc new page
> and
> >> >> copy otherwise..
> >> >>
> >> >
> >> > I'd like to say thank you again you gave me comments and advices in
> detail.
> >> > there may be my missing points but I will check it again.
> >> >
> >> > Thanks,
> >> > Inki Dae
> >> >
> >> >> Cheers,
> >> >> Jerome
> >> >
> >>
> >> I think to sumup things there is 2 use case:
> >> 1: we want to steal anonymous page and move them to a gem/gpu object.
> >> So call get_user_pages on the rand and then unmap_mapping_range on the
> >> range we want to still page are the function you would want. That
> >> assume of course that the api case your are trying to accelerate is ok
> >> with having the user process loosing the content of this buffer. If
> >> it's not the other solution is to mark the range as COW and steal the
> >> page, if userspace try to write something new to the range it will get
> >> new page (from copy of old one).
> >>
> >> Let call it  drm_gem_from_userptr
> >>
> >> 2: you want to be able to use malloced area over long period of time
> >> (several minute) with the gpu and you want that userspace vma still
> >> point to those page. It the most problematic case, as i said just
> >> mlocking the vma is troublesome as malicious userspace can munlock it
> >> or try to abuse you to mlock too much. I believe right now there is no
> >> good way to handle this case. That means that page can't be swaped out
> >> or moved as regular anonymous page but on fork or exec this area still
> >> need to behave like an anonymous vma.
> >>
> >> Let call drm_gem_backed_by_userptr
> >>
> >> Inki i really think you should split this 2 usecase, and do only the
> >> drm_gem_from_userptr if it's enough for what you are trying to do. As
> >> the second case look that too many things can go wrong.
> >
> > Jumping into the discussion late: Chris Wilson stitched together a
> userptr
> > feature for i915. Iirc he started with your 1st usecase but quickly
> > noticed that doing all this setup stuff (get_user_pages alone, he didn't
> > include your proposed cow trick) is too expensive and it's cheaper to
> just
> > upload things with the cpu.
> 
> I think use case 1 can still be usefull on desktop x86 but the object
> need to be big something like bigger 16M or probably even bigger, so
> that the cost of memcpy is bigger than the cost of tlb trashing. I am
> sure than once we are seeing 1G dataset of opencl we will want to use
> the stealing code path rather than memcpy things. Sadly API like CL
> and GL don't make provision saying that the data ptr user supplied
> might not have the content anymore so it makes the cow trick kind of
> needed but it kills usecase such as :
> scratch = malloc()
> for (i =0; i<numtex; i++){
>    readtexture(scratch, texfilename[i])
>    glteximage(scratch)
> }
> free(scratch)
> 
> Or anything with similar access, here obviously the page backing the
> scratch area can be stole at each glteximage call.
> 
> Anyway if you can define your api and provision that after call the
> data you provided is no longer available then use case 1 sounds doable
> and worth it to me.
> 

How about forcing VM_DONTCOPY not to copy the vma on fork? this flag may
prevent doing COW. and I was talked that get_user_pages call avoid the pages
from being swapped out, not using mlock. if all the pages from
get_user_pages are MOVABLE then CMA would try to migrate movable pages into
reserved space for DMA once device driver tries allocation through dma api
so if we could prevent the pages from being moved by CMA and we limit
maximum size for userptr(accessed by only root user) then I guess that we
could avoid these issues. there may be many things I don't care so please
give me any comments and advices.

Thanks,
Inki Dae


> > So he needs to keep around these mappings for essentially forever to
> > amortize the setup cost, which boils down to your 2nd use-case. I've
> > refused to merge that code since, like you point out, too much stuff can
> > go wrong when we pin arbitrary ranges of userspace.
> >
> > One example which would only affect ARM platforms is that this would
> > horribly break CMA: Userspace malloc is allocated as GFP_MOVEABLE, and
> CMA
> > relies on migrating moveable pages out of the CMA region to handle large
> > contigious allocations. Currently the page migrate code simply backs
> down
> > and waits a bit if it encounters a page locked by get_user_pages,
> assuming
> > that this is due to io and will complete shortly. If you hold onto such
> > pages for a long time, that retry will eventually fail and break CMA.
> >
> > There are other problems affecting also desktop machines, but I've
> figured
> > I'll pick something that really hurts on arm ;-)
> >
> > Yours, Daniel
> > --
> > Daniel Vetter
> > Mail: daniel@ffwll.ch
> > Mobile: +41 (0)79 365 57 48
> 
> Yes, what i also wanted to stress is that get_user_pages is not
> enough, we are not doing something over the period of an ioctl in
> which case everything is fine and doable, but are stealing page and
> expect to use them at random point in the future with no kind of
> synchronization with userspace. Anyway i think we agree that this
> second use case is way complex and many things can go wrong, i still
> think that for opencl we might want to able to do it but by them i am
> expecting we will take advantage of iommu being able to pagefault from
> process pagetable like the next AMD iommu can.
> 
> Cheers,
> Jerome

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Minchan Kim]

On 05/11/2012 10:47 AM, Inki Dae wrote:

> 
>> -----Original Message-----
>> From: Jerome Glisse [mailto:j.glisse@gmail.com]
>> Sent: Friday, May 11, 2012 12:53 AM
>> To: Jerome Glisse; Inki Dae; linux-mm@kvack.org;
> kyungmin.park@samsung.com;
>> sw0312.kim@samsung.com; dri-devel@lists.freedesktop.org
>> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
>>
>> On Thu, May 10, 2012 at 11:31 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
>>> On Thu, May 10, 2012 at 11:05:07AM -0400, Jerome Glisse wrote:
>>>> On Wed, May 9, 2012 at 10:44 PM, Inki Dae <inki.dae@samsung.com> wrote:
>>>>> Hi Jerome,
>>>>>
>>>>> Thank you again.
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Jerome Glisse [mailto:j.glisse@gmail.com]
>>>>>> Sent: Thursday, May 10, 2012 3:33 AM
>>>>>> To: Inki Dae
>>>>>> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
>>>>>> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-
>> mm@kvack.org
>>>>>> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
>>>>>>
>>>>>> On Wed, May 9, 2012 at 10:45 AM, Jerome Glisse <j.glisse@gmail.com>
>> wrote:
>>>>>>> On Wed, May 9, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com>
>> wrote:
>>>>>>>> this feature is used to import user space region allocated by
>> malloc()
>>>>>> or
>>>>>>>> mmaped into a gem. and to guarantee the pages to user space not
>> to be
>>>>>>>> swapped out, the VMAs within the user space would be locked and
>> then
>>>>>> unlocked
>>>>>>>> when the pages are released.
>>>>>>>>
>>>>>>>> but this lock might result in significant degradation of system
>>>>>> performance
>>>>>>>> because the pages couldn't be swapped out so we limit user-
>> desired
>>>>>> userptr
>>>>>>>> size to pre-defined.
>>>>>>>>
>>>>>>>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>>>>>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>>>>>
>>>>>>>
>>>>>>> Again i would like feedback from mm people (adding cc). I am not
>> sure
>>>>>>> locking the vma is the right anwser as i said in my previous mail,
>>>>>>> userspace can munlock it in your back, maybe VM_RESERVED is
> better.
>>>>>>> Anyway even not considering that you don't check at all that
>> process
>>>>>>> don't go over the limit of locked page see mm/mlock.c
>> RLIMIT_MEMLOCK
>>>>>>> for how it's done. Also you mlock complete vma but the userptr you
>> get
>>>>>>> might be inside say 16M vma and you only care about 1M of userptr,
>> if
>>>>>>> you mark the whole vma as locked than anytime a new page is fault
>> in
>>>>>>> the vma else where than in the buffer you are interested then it
>> got
>>>>>>> allocated for ever until the gem buffer is destroy, i am not sure
>> of
>>>>>>> what happen to the vma on next malloc if it grows or not (i would
>>>>>>> think it won't grow at it would have different flags than new
>>>>>>> anonymous memory).
>>>>>>>
>>>>>>> The whole business of directly using malloced memory for gpu is
>> fishy
>>>>>>> and i would really like to get it right rather than relying on
>> never
>>>>>>> hitting strange things like page migration, vma merging, or worse
>>>>>>> things like over locking pages and stealing memory.
>>>>>>>
>>>>>>> Cheers,
>>>>>>> Jerome
>>>>>>
>>>>>> I had a lengthy discussion with mm people (thx a lot for that). I
>>>>>> think we should split 2 different use case. The zero-copy upload
>> case
>>>>>> ie :
>>>>>> app:
>>>>>>     ptr = malloc()
>>>>>>     ...
>>>>>>     glTex/VBO/UBO/...(ptr)
>>>>>>     free(ptr) or reuse it for other things
>>>>>> For which i guess you want to avoid having to do a memcpy inside the
>>>>>> gl library (could be anything else than gl that have same useage
>>>>>> pattern).
>>>>>>
>>>>>
>>>>> Right, in this case, we are using the userptr feature as pixman and
>> evas
>>>>> backend to use 2d accelerator.
>>>>>
>>>>>> ie after the upload happen you don't care about those page they can
>>>>>> removed from the vma or marked as cow so that anything messing with
>>>>>> those page after the upload won't change what you uploaded. Of
>> course
>>>>>
>>>>> I'm not sure that I understood your mentions but could the pages be
>> removed
>>>>> from vma with VM_LOCKED or VM_RESERVED? once glTex/VBO/UBO/..., the
>> VMAs to
>>>>> user space would be locked. if cpu accessed significant part of all
>> the
>>>>> pages in user mode then pages to the part would be allocated by page
>> fault
>>>>> handler, after that, through userptr, the VMAs to user address space
>> would
>>>>> be locked(at this time, the remaining pages would be allocated also
>> by
>>>>> get_user_pages by calling page fault handler) I'd be glad to give me
>> any
>>>>> comments and advices if there is my missing point.
>>>>>
>>>>>> this is assuming that the tlb cost of doing such thing is smaller
>> than
>>>>>> the cost of memcpy the data.
>>>>>>
>>>>>
>>>>> yes, in our test case, the tlb cost(incurred by tlb miss) was smaller
>> than
>>>>> the cost of memcpy also cpu usage. of course, this would be depended
>> on gpu
>>>>> performance.
>>>>>
>>>>>> Two way to do that, either you assume app can't not read back data
>>>>>> after gl can and you do an unmap_mapping_range (make sure you only
>>>>>> unmap fully covered page and that you copy non fully covered page)
>> or
>>>>>> you want to allow userspace to still read data or possibly overwrite
>>>>>> them
>>>>>>
>>>>>> Second use case is something more like for the opencl case of
>>>>>> CL_MEM_USE_HOST_PTR, in which you want to use the same page in the
>> gpu
>>>>>> and keep the userspace vma pointing to those page. I think the
>>>>>> agreement on this case is that there is no way right now to do it
>>>>>> sanely inside linux kernel. mlocking will need proper accounting
>>>>>> against rtlimit but this limit might be low. Also the fork case
>> might
>>>>>> be problematic.
>>>>>>
>>>>>> For the fork case the memory is anonymous so it should be COWed in
>> the
>>>>>> fork child but relative to cl context that means the child could not
>>>>>> use the cl context with that memory or at least if the child write
>> to
>>>>>> this memory the cl will not see those change. I guess the answer to
>>>>>> that one is that you really need to use the cl api to read the
>> object
>>>>>> or get proper ptr to read it.
>>>>>>
>>>>>> Anyway in all case, implementing this userptr thing need a lot more
>>>>>> code. You have to check to that the vma you are trying to use is
>>>>>> anonymous and only handle this case and fallback to alloc new page
>> and
>>>>>> copy otherwise..
>>>>>>
>>>>>
>>>>> I'd like to say thank you again you gave me comments and advices in
>> detail.
>>>>> there may be my missing points but I will check it again.
>>>>>
>>>>> Thanks,
>>>>> Inki Dae
>>>>>
>>>>>> Cheers,
>>>>>> Jerome
>>>>>
>>>>
>>>> I think to sumup things there is 2 use case:
>>>> 1: we want to steal anonymous page and move them to a gem/gpu object.
>>>> So call get_user_pages on the rand and then unmap_mapping_range on the
>>>> range we want to still page are the function you would want. That
>>>> assume of course that the api case your are trying to accelerate is ok
>>>> with having the user process loosing the content of this buffer. If
>>>> it's not the other solution is to mark the range as COW and steal the
>>>> page, if userspace try to write something new to the range it will get
>>>> new page (from copy of old one).
>>>>
>>>> Let call it  drm_gem_from_userptr
>>>>
>>>> 2: you want to be able to use malloced area over long period of time
>>>> (several minute) with the gpu and you want that userspace vma still
>>>> point to those page. It the most problematic case, as i said just
>>>> mlocking the vma is troublesome as malicious userspace can munlock it
>>>> or try to abuse you to mlock too much. I believe right now there is no
>>>> good way to handle this case. That means that page can't be swaped out
>>>> or moved as regular anonymous page but on fork or exec this area still
>>>> need to behave like an anonymous vma.
>>>>
>>>> Let call drm_gem_backed_by_userptr
>>>>
>>>> Inki i really think you should split this 2 usecase, and do only the
>>>> drm_gem_from_userptr if it's enough for what you are trying to do. As
>>>> the second case look that too many things can go wrong.
>>>
>>> Jumping into the discussion late: Chris Wilson stitched together a
>> userptr
>>> feature for i915. Iirc he started with your 1st usecase but quickly
>>> noticed that doing all this setup stuff (get_user_pages alone, he didn't
>>> include your proposed cow trick) is too expensive and it's cheaper to
>> just
>>> upload things with the cpu.
>>
>> I think use case 1 can still be usefull on desktop x86 but the object
>> need to be big something like bigger 16M or probably even bigger, so
>> that the cost of memcpy is bigger than the cost of tlb trashing. I am
>> sure than once we are seeing 1G dataset of opencl we will want to use
>> the stealing code path rather than memcpy things. Sadly API like CL
>> and GL don't make provision saying that the data ptr user supplied
>> might not have the content anymore so it makes the cow trick kind of
>> needed but it kills usecase such as :
>> scratch = malloc()
>> for (i =0; i<numtex; i++){
>>    readtexture(scratch, texfilename[i])
>>    glteximage(scratch)
>> }
>> free(scratch)
>>
>> Or anything with similar access, here obviously the page backing the
>> scratch area can be stole at each glteximage call.
>>
>> Anyway if you can define your api and provision that after call the
>> data you provided is no longer available then use case 1 sounds doable
>> and worth it to me.
>>
> 
> How about forcing VM_DONTCOPY not to copy the vma on fork? this flag may
> prevent doing COW. and I was talked that get_user_pages call avoid the pages


That was why we introduced MADV_DONTFORK.

> from being swapped out, not using mlock. if all the pages from


True.

> get_user_pages are MOVABLE then CMA would try to migrate movable pages into
> reserved space for DMA once device driver tries allocation through dma api
> so if we could prevent the pages from being moved by CMA and we limit


CMA can't migrate pinning pages which have increased page count by get_user_pages.
So don't worry about it.

> maximum size for userptr(accessed by only root user) then I guess that we
> could avoid these issues. there may be many things I don't care so please
> give me any comments and advices.
> 
> Thanks,
> Inki Dae
> 
> 
>>> So he needs to keep around these mappings for essentially forever to
>>> amortize the setup cost, which boils down to your 2nd use-case. I've
>>> refused to merge that code since, like you point out, too much stuff can
>>> go wrong when we pin arbitrary ranges of userspace.
>>>
>>> One example which would only affect ARM platforms is that this would
>>> horribly break CMA: Userspace malloc is allocated as GFP_MOVEABLE, and
>> CMA
>>> relies on migrating moveable pages out of the CMA region to handle large
>>> contigious allocations. Currently the page migrate code simply backs
>> down
>>> and waits a bit if it encounters a page locked by get_user_pages,
>> assuming
>>> that this is due to io and will complete shortly. If you hold onto such
>>> pages for a long time, that retry will eventually fail and break CMA.
>>>
>>> There are other problems affecting also desktop machines, but I've
>> figured
>>> I'll pick something that really hurts on arm ;-)
>>>
>>> Yours, Daniel
>>> --
>>> Daniel Vetter
>>> Mail: daniel@ffwll.ch
>>> Mobile: +41 (0)79 365 57 48
>>
>> Yes, what i also wanted to stress is that get_user_pages is not
>> enough, we are not doing something over the period of an ioctl in
>> which case everything is fine and doable, but are stealing page and
>> expect to use them at random point in the future with no kind of
>> synchronization with userspace. Anyway i think we agree that this
>> second use case is way complex and many things can go wrong, i still
>> think that for opencl we might want to able to do it but by them i am
>> expecting we will take advantage of iommu being able to pagefault from
>> process pagetable like the next AMD iommu can.
>>
>> Cheers,
>> Jerome
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=ilto:"dont@kvack.org"> email@kvack.org </a>
> 



-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Inki Dae]

Hi Jerome,

> -----Original Message-----
> From: Jerome Glisse [mailto:j.glisse@gmail.com]
> Sent: Wednesday, May 09, 2012 11:46 PM
> To: Inki Dae
> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
> 
> On Wed, May 9, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com> wrote:
> > this feature is used to import user space region allocated by malloc()
> or
> > mmaped into a gem. and to guarantee the pages to user space not to be
> > swapped out, the VMAs within the user space would be locked and then
> unlocked
> > when the pages are released.
> >
> > but this lock might result in significant degradation of system
> performance
> > because the pages couldn't be swapped out so we limit user-desired
> userptr
> > size to pre-defined.
> >
> > Signed-off-by: Inki Dae <inki.dae@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> 
> 
> Again i would like feedback from mm people (adding cc). I am not sure

Thank you, I missed adding mm as cc.

> locking the vma is the right anwser as i said in my previous mail,
> userspace can munlock it in your back, maybe VM_RESERVED is better.

I know that with VM_RESERVED flag, also we can avoid the pages from being
swapped out. but these pages should be unlocked anytime we want because we
could allocate all pages on system and lock them, which in turn, it may
result in significant deterioration of system performance.(maybe other
processes requesting free memory would be blocked) so I used VM_LOCKED flags
instead. but I'm not sure this way is best also.

> Anyway even not considering that you don't check at all that process
> don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK

Thank you for your advices.

> for how it's done. Also you mlock complete vma but the userptr you get
> might be inside say 16M vma and you only care about 1M of userptr, if
> you mark the whole vma as locked than anytime a new page is fault in
> the vma else where than in the buffer you are interested then it got
> allocated for ever until the gem buffer is destroy, i am not sure of
> what happen to the vma on next malloc if it grows or not (i would
> think it won't grow at it would have different flags than new
> anonymous memory).
> 
> The whole business of directly using malloced memory for gpu is fishy
> and i would really like to get it right rather than relying on never
> hitting strange things like page migration, vma merging, or worse
> things like over locking pages and stealing memory.
> 

Your comments are very helpful to me and I will consider some cases I missed
and you pointed out for next patch.

Thanks,
Inki Dae

> Cheers,
> Jerome
> 
> 
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_drv.c |    2 +
> >  drivers/gpu/drm/exynos/exynos_drm_gem.c |  334
> +++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/exynos/exynos_drm_gem.h |   17 ++-
> >  include/drm/exynos_drm.h                |   26 +++-
> >  4 files changed, 376 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > index 1e68ec2..e8ae3f1 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > @@ -220,6 +220,8 @@ static struct drm_ioctl_desc exynos_ioctls[] = {
> >        DRM_IOCTL_DEF_DRV(EXYNOS_GEM_MAP_OFFSET,
> >                        exynos_drm_gem_map_offset_ioctl, DRM_UNLOCKED |
> >                        DRM_AUTH),
> > +       DRM_IOCTL_DEF_DRV(EXYNOS_GEM_USERPTR,
> > +                       exynos_drm_gem_userptr_ioctl, DRM_UNLOCKED),
> >        DRM_IOCTL_DEF_DRV(EXYNOS_GEM_MMAP,
> >                        exynos_drm_gem_mmap_ioctl, DRM_UNLOCKED |
DRM_AUTH),
> >        DRM_IOCTL_DEF_DRV(EXYNOS_GEM_GET,
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > index e6abb66..ccc6e3d 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > @@ -68,6 +68,83 @@ static int check_gem_flags(unsigned int flags)
> >        return 0;
> >  }
> >
> > +static struct vm_area_struct *get_vma(struct vm_area_struct *vma)
> > +{
> > +       struct vm_area_struct *vma_copy;
> > +
> > +       vma_copy = kmalloc(sizeof(*vma_copy), GFP_KERNEL);
> > +       if (!vma_copy)
> > +               return NULL;
> > +
> > +       if (vma->vm_ops && vma->vm_ops->open)
> > +               vma->vm_ops->open(vma);
> > +
> > +       if (vma->vm_file)
> > +               get_file(vma->vm_file);
> > +
> > +       memcpy(vma_copy, vma, sizeof(*vma));
> > +
> > +       vma_copy->vm_mm = NULL;
> > +       vma_copy->vm_next = NULL;
> > +       vma_copy->vm_prev = NULL;
> > +
> > +       return vma_copy;
> > +}
> > +
> > +
> > +static void put_vma(struct vm_area_struct *vma)
> > +{
> > +       if (!vma)
> > +               return;
> > +
> > +       if (vma->vm_ops && vma->vm_ops->close)
> > +               vma->vm_ops->close(vma);
> > +
> > +       if (vma->vm_file)
> > +               fput(vma->vm_file);
> > +
> > +       kfree(vma);
> > +}
> > +
> > +/*
> > + * lock_userptr_vma - lock VMAs within user address space
> > + *
> > + * this function locks vma within user address space to avoid pages
> > + * to the userspace from being swapped out.
> > + * if this vma isn't locked, the pages to the userspace could be
> swapped out
> > + * so unprivileged user might access different pages and dma of any
> device
> > + * could access physical memory region not intended once swap-in.
> > + */
> > +static int lock_userptr_vma(struct exynos_drm_gem_buf *buf, unsigned
> int lock)
> > +{
> > +       struct vm_area_struct *vma;
> > +       unsigned long start, end;
> > +
> > +       start = buf->userptr;
> > +       end = buf->userptr + buf->size - 1;
> > +
> > +       down_write(&current->mm->mmap_sem);
> > +
> > +       do {
> > +               vma = find_vma(current->mm, start);
> > +               if (!vma) {
> > +                       up_write(&current->mm->mmap_sem);
> > +                       return -EFAULT;
> > +               }
> > +
> > +               if (lock)
> > +                       vma->vm_flags |= VM_LOCKED;
> > +               else
> > +                       vma->vm_flags &= ~VM_LOCKED;
> > +
> > +               start = vma->vm_end + 1;
> > +       } while (vma->vm_end < end);
> > +
> > +       up_write(&current->mm->mmap_sem);
> > +
> > +       return 0;
> > +}
> > +
> >  static void update_vm_cache_attr(struct exynos_drm_gem_obj *obj,
> >                                        struct vm_area_struct *vma)
> >  {
> > @@ -256,6 +333,44 @@ static void exynos_drm_gem_put_pages(struct
> drm_gem_object *obj)
> >        /* add some codes for UNCACHED type here. TODO */
> >  }
> >
> > +static void exynos_drm_put_userptr(struct drm_gem_object *obj)
> > +{
> > +       struct exynos_drm_gem_obj *exynos_gem_obj;
> > +       struct exynos_drm_gem_buf *buf;
> > +       struct vm_area_struct *vma;
> > +       int npages;
> > +
> > +       exynos_gem_obj = to_exynos_gem_obj(obj);
> > +       buf = exynos_gem_obj->buffer;
> > +       vma = exynos_gem_obj->vma;
> > +
> > +       if (vma && (vma->vm_flags & VM_PFNMAP) && (vma->vm_pgoff)) {
> > +               put_vma(exynos_gem_obj->vma);
> > +               goto out;
> > +       }
> > +
> > +       npages = buf->size >> PAGE_SHIFT;
> > +
> > +       if (exynos_gem_obj->flags & EXYNOS_BO_USERPTR && !buf->pfnmap)
> > +               lock_userptr_vma(buf, 0);
> > +
> > +       npages--;
> > +       while (npages >= 0) {
> > +               if (buf->write)
> > +                       set_page_dirty_lock(buf->pages[npages]);
> > +
> > +               put_page(buf->pages[npages]);
> > +               npages--;
> > +       }
> > +
> > +out:
> > +       kfree(buf->pages);
> > +       buf->pages = NULL;
> > +
> > +       kfree(buf->sgt);
> > +       buf->sgt = NULL;
> > +}
> > +
> >  static int exynos_drm_gem_handle_create(struct drm_gem_object *obj,
> >                                        struct drm_file *file_priv,
> >                                        unsigned int *handle)
> > @@ -295,6 +410,8 @@ void exynos_drm_gem_destroy(struct
> exynos_drm_gem_obj *exynos_gem_obj)
> >
> >        if (exynos_gem_obj->flags & EXYNOS_BO_NONCONTIG)
> >                exynos_drm_gem_put_pages(obj);
> > +       else if (exynos_gem_obj->flags & EXYNOS_BO_USERPTR)
> > +               exynos_drm_put_userptr(obj);
> >        else
> >                exynos_drm_free_buf(obj->dev, exynos_gem_obj->flags,
buf);
> >
> > @@ -606,6 +723,223 @@ int exynos_drm_gem_mmap_ioctl(struct drm_device
> *dev, void *data,
> >        return 0;
> >  }
> >
> > +static int exynos_drm_get_userptr(struct drm_device *dev,
> > +                               struct exynos_drm_gem_obj *obj,
> > +                               unsigned long userptr,
> > +                               unsigned int write)
> > +{
> > +       unsigned int get_npages;
> > +       unsigned long npages = 0;
> > +       struct vm_area_struct *vma;
> > +       struct exynos_drm_gem_buf *buf = obj->buffer;
> > +       int ret;
> > +
> > +       down_read(&current->mm->mmap_sem);
> > +       vma = find_vma(current->mm, userptr);
> > +
> > +       /* the memory region mmaped with VM_PFNMAP. */
> > +       if (vma && (vma->vm_flags & VM_PFNMAP) && (vma->vm_pgoff)) {
> > +               unsigned long this_pfn, prev_pfn, pa;
> > +               unsigned long start, end, offset;
> > +               struct scatterlist *sgl;
> > +               int ret;
> > +
> > +               start = userptr;
> > +               offset = userptr & ~PAGE_MASK;
> > +               end = start + buf->size;
> > +               sgl = buf->sgt->sgl;
> > +
> > +               for (prev_pfn = 0; start < end; start += PAGE_SIZE) {
> > +                       ret = follow_pfn(vma, start, &this_pfn);
> > +                       if (ret)
> > +                               goto err;
> > +
> > +                       if (prev_pfn == 0) {
> > +                               pa = this_pfn << PAGE_SHIFT;
> > +                               buf->dma_addr = pa + offset;
> > +                       } else if (this_pfn != prev_pfn + 1) {
> > +                               ret = -EINVAL;
> > +                               goto err;
> > +                       }
> > +
> > +                       sg_dma_address(sgl) = (pa + offset);
> > +                       sg_dma_len(sgl) = PAGE_SIZE;
> > +                       prev_pfn = this_pfn;
> > +                       pa += PAGE_SIZE;
> > +                       npages++;
> > +                       sgl = sg_next(sgl);
> > +               }
> > +
> > +               obj->vma = get_vma(vma);
> > +               if (!obj->vma) {
> > +                       ret = -ENOMEM;
> > +                       goto err;
> > +               }
> > +
> > +               up_read(&current->mm->mmap_sem);
> > +               buf->pfnmap = true;
> > +
> > +               return npages;
> > +err:
> > +               buf->dma_addr = 0;
> > +               up_read(&current->mm->mmap_sem);
> > +
> > +               return ret;
> > +       }
> > +
> > +       up_read(&current->mm->mmap_sem);
> > +
> > +       /*
> > +        * lock the vma within userptr to avoid userspace buffer
> > +        * from being swapped out.
> > +        */
> > +       ret = lock_userptr_vma(buf, 1);
> > +       if (ret < 0) {
> > +               DRM_ERROR("failed to lock vma for userptr.\n");
> > +               lock_userptr_vma(buf, 0);
> > +               return 0;
> > +       }
> > +
> > +       buf->write = write;
> > +       npages = buf->size >> PAGE_SHIFT;
> > +
> > +       down_read(&current->mm->mmap_sem);
> > +       get_npages = get_user_pages(current, current->mm, userptr,
> > +                                       npages, write, 1, buf->pages,
NULL);
> > +       up_read(&current->mm->mmap_sem);
> > +       if (get_npages != npages)
> > +               DRM_ERROR("failed to get user_pages.\n");
> > +
> > +       buf->userptr = userptr;
> > +       buf->pfnmap = false;
> > +
> > +       return get_npages;
> > +}
> > +
> > +int exynos_drm_gem_userptr_ioctl(struct drm_device *dev, void *data,
> > +                                     struct drm_file *file_priv)
> > +{
> > +       struct exynos_drm_private *priv = dev->dev_private;
> > +       struct exynos_drm_gem_obj *exynos_gem_obj;
> > +       struct drm_exynos_gem_userptr *args = data;
> > +       struct exynos_drm_gem_buf *buf;
> > +       struct scatterlist *sgl;
> > +       unsigned long size, userptr;
> > +       unsigned int npages;
> > +       int ret, get_npages;
> > +
> > +       DRM_DEBUG_KMS("%s\n", __FILE__);
> > +
> > +       if (!args->size) {
> > +               DRM_ERROR("invalid size.\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = check_gem_flags(args->flags);
> > +       if (ret)
> > +               return ret;
> > +
> > +       size = roundup_gem_size(args->size, EXYNOS_BO_USERPTR);
> > +
> > +       if (size > priv->userptr_limit) {
> > +               DRM_ERROR("excessed maximum size of userptr.\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       userptr = args->userptr;
> > +
> > +       buf = exynos_drm_init_buf(dev, size);
> > +       if (!buf)
> > +               return -ENOMEM;
> > +
> > +       exynos_gem_obj = exynos_drm_gem_init(dev, size);
> > +       if (!exynos_gem_obj) {
> > +               ret = -ENOMEM;
> > +               goto err_free_buffer;
> > +       }
> > +
> > +       buf->sgt = kzalloc(sizeof(struct sg_table), GFP_KERNEL);
> > +       if (!buf->sgt) {
> > +               DRM_ERROR("failed to allocate buf->sgt.\n");
> > +               ret = -ENOMEM;
> > +               goto err_release_gem;
> > +       }
> > +
> > +       npages = size >> PAGE_SHIFT;
> > +
> > +       ret = sg_alloc_table(buf->sgt, npages, GFP_KERNEL);
> > +       if (ret < 0) {
> > +               DRM_ERROR("failed to initailize sg table.\n");
> > +               goto err_free_sgt;
> > +       }
> > +
> > +       buf->pages = kzalloc(npages * sizeof(struct page *),
GFP_KERNEL);
> > +       if (!buf->pages) {
> > +               DRM_ERROR("failed to allocate buf->pages\n");
> > +               ret = -ENOMEM;
> > +               goto err_free_table;
> > +       }
> > +
> > +       exynos_gem_obj->buffer = buf;
> > +
> > +       get_npages = exynos_drm_get_userptr(dev, exynos_gem_obj,
userptr,
> 1);
> > +       if (get_npages != npages) {
> > +               DRM_ERROR("failed to get user_pages.\n");
> > +               ret = get_npages;
> > +               goto err_release_userptr;
> > +       }
> > +
> > +       ret = exynos_drm_gem_handle_create(&exynos_gem_obj->base,
> file_priv,
> > +                                               &args->handle);
> > +       if (ret < 0) {
> > +               DRM_ERROR("failed to create gem handle.\n");
> > +               goto err_release_userptr;
> > +       }
> > +
> > +       sgl = buf->sgt->sgl;
> > +
> > +       /*
> > +        * if buf->pfnmap is true then update sgl of sgt with pages but
> > +        * if buf->pfnmap is false then it means the sgl was updated
> already
> > +        * so it doesn't need to update the sgl.
> > +        */
> > +       if (!buf->pfnmap) {
> > +               unsigned int i = 0;
> > +
> > +               /* set all pages to sg list. */
> > +               while (i < npages) {
> > +                       sg_set_page(sgl, buf->pages[i], PAGE_SIZE, 0);
> > +                       sg_dma_address(sgl) =
page_to_phys(buf->pages[i]);
> > +                       i++;
> > +                       sgl = sg_next(sgl);
> > +               }
> > +       }
> > +
> > +       /* always use EXYNOS_BO_USERPTR as memory type for userptr. */
> > +       exynos_gem_obj->flags |= EXYNOS_BO_USERPTR;
> > +
> > +       return 0;
> > +
> > +err_release_userptr:
> > +       get_npages--;
> > +       while (get_npages >= 0)
> > +               put_page(buf->pages[get_npages--]);
> > +       kfree(buf->pages);
> > +       buf->pages = NULL;
> > +err_free_table:
> > +       sg_free_table(buf->sgt);
> > +err_free_sgt:
> > +       kfree(buf->sgt);
> > +       buf->sgt = NULL;
> > +err_release_gem:
> > +       drm_gem_object_release(&exynos_gem_obj->base);
> > +       kfree(exynos_gem_obj);
> > +       exynos_gem_obj = NULL;
> > +err_free_buffer:
> > +       exynos_drm_free_buf(dev, 0, buf);
> > +       return ret;
> > +}
> > +
> >  int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data,
> >                                      struct drm_file *file_priv)
> >  {      struct exynos_drm_gem_obj *exynos_gem_obj;
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> > index 3334c9f..72bd993 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> > @@ -29,27 +29,35 @@
> >  #define to_exynos_gem_obj(x)   container_of(x,\
> >                        struct exynos_drm_gem_obj, base)
> >
> > -#define IS_NONCONTIG_BUFFER(f)         (f & EXYNOS_BO_NONCONTIG)
> > +#define IS_NONCONTIG_BUFFER(f) ((f & EXYNOS_BO_NONCONTIG) ||\
> > +                                       (f & EXYNOS_BO_USERPTR))
> >
> >  /*
> >  * exynos drm gem buffer structure.
> >  *
> >  * @kvaddr: kernel virtual address to allocated memory region.
> > + * @userptr: user space address.
> >  * @dma_addr: bus address(accessed by dma) to allocated memory region.
> >  *     - this address could be physical address without IOMMU and
> >  *     device address with IOMMU.
> > + * @write: whether pages will be written to by the caller.
> >  * @sgt: sg table to transfer page data.
> >  * @pages: contain all pages to allocated memory region.
> >  * @page_size: could be 4K, 64K or 1MB.
> >  * @size: size of allocated memory region.
> > + * @pfnmap: indicate whether memory region from userptr is mmaped with
> > + *     VM_PFNMAP or not.
> >  */
> >  struct exynos_drm_gem_buf {
> >        void __iomem            *kvaddr;
> > +       unsigned long           userptr;
> >        dma_addr_t              dma_addr;
> > +       unsigned int            write;
> >        struct sg_table         *sgt;
> >        struct page             **pages;
> >        unsigned long           page_size;
> >        unsigned long           size;
> > +       bool                    pfnmap;
> >  };
> >
> >  /*
> > @@ -64,6 +72,8 @@ struct exynos_drm_gem_buf {
> >  *     continuous memory region allocated by user request
> >  *     or at framebuffer creation.
> >  * @size: total memory size to physically non-continuous memory region.
> > + * @vma: a pointer to the vma to user address space and used to release
> > + *     the pages to user space.
> >  * @flags: indicate memory type to allocated buffer and cache
attruibute.
> >  *
> >  * P.S. this object would be transfered to user as kms_bo.handle so
> > @@ -73,6 +83,7 @@ struct exynos_drm_gem_obj {
> >        struct drm_gem_object           base;
> >        struct exynos_drm_gem_buf       *buffer;
> >        unsigned long                   size;
> > +       struct vm_area_struct           *vma;
> >        unsigned int                    flags;
> >  };
> >
> > @@ -130,6 +141,10 @@ int exynos_drm_gem_map_offset_ioctl(struct
> drm_device *dev, void *data,
> >  int exynos_drm_gem_mmap_ioctl(struct drm_device *dev, void *data,
> >                              struct drm_file *file_priv);
> >
> > +/* map user space allocated by malloc to pages. */
> > +int exynos_drm_gem_userptr_ioctl(struct drm_device *dev, void *data,
> > +                                     struct drm_file *file_priv);
> > +
> >  /* get buffer information to memory region allocated by gem. */
> >  int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data,
> >                                      struct drm_file *file_priv);
> > diff --git a/include/drm/exynos_drm.h b/include/drm/exynos_drm.h
> > index 52465dc..33fa1e2 100644
> > --- a/include/drm/exynos_drm.h
> > +++ b/include/drm/exynos_drm.h
> > @@ -77,6 +77,23 @@ struct drm_exynos_gem_mmap {
> >  };
> >
> >  /**
> > + * User-requested user space importing structure
> > + *
> > + * @userptr: user space address allocated by malloc.
> > + * @size: size to the buffer allocated by malloc.
> > + * @flags: indicate user-desired cache attribute to map the allocated
> buffer
> > + *     to kernel space.
> > + * @handle: a returned handle to created gem object.
> > + *     - this handle will be set by gem module of kernel side.
> > + */
> > +struct drm_exynos_gem_userptr {
> > +       uint64_t userptr;
> > +       uint64_t size;
> > +       unsigned int flags;
> > +       unsigned int handle;
> > +};
> > +
> > +/**
> >  * A structure to gem information.
> >  *
> >  * @handle: a handle to gem object created.
> > @@ -135,8 +152,10 @@ enum e_drm_exynos_gem_mem_type {
> >        EXYNOS_BO_CACHABLE      = 1 << 1,
> >        /* write-combine mapping. */
> >        EXYNOS_BO_WC            = 1 << 2,
> > +       /* user space memory allocated by malloc. */
> > +       EXYNOS_BO_USERPTR       = 1 << 3,
> >        EXYNOS_BO_MASK          = EXYNOS_BO_NONCONTIG |
EXYNOS_BO_CACHABLE
> |
> > -                                       EXYNOS_BO_WC
> > +                                       EXYNOS_BO_WC | EXYNOS_BO_USERPTR
> >  };
> >
> >  struct drm_exynos_g2d_get_ver {
> > @@ -173,7 +192,7 @@ struct drm_exynos_g2d_exec {
> >  #define DRM_EXYNOS_GEM_CREATE          0x00
> >  #define DRM_EXYNOS_GEM_MAP_OFFSET      0x01
> >  #define DRM_EXYNOS_GEM_MMAP            0x02
> > -/* Reserved 0x03 ~ 0x05 for exynos specific gem ioctl */
> > +#define DRM_EXYNOS_GEM_USERPTR         0x03
> >  #define DRM_EXYNOS_GEM_GET             0x04
> >  #define DRM_EXYNOS_USER_LIMIT          0x05
> >  #define DRM_EXYNOS_PLANE_SET_ZPOS      0x06
> > @@ -193,6 +212,9 @@ struct drm_exynos_g2d_exec {
> >  #define DRM_IOCTL_EXYNOS_GEM_MMAP      DRM_IOWR(DRM_COMMAND_BASE + \
> >                DRM_EXYNOS_GEM_MMAP, struct drm_exynos_gem_mmap)
> >
> > +#define DRM_IOCTL_EXYNOS_GEM_USERPTR   DRM_IOWR(DRM_COMMAND_BASE + \
> > +               DRM_EXYNOS_GEM_USERPTR, struct drm_exynos_gem_userptr)
> > +
> >  #define DRM_IOCTL_EXYNOS_GEM_GET       DRM_IOWR(DRM_COMMAND_BASE + \
> >                DRM_EXYNOS_GEM_GET,     struct drm_exynos_gem_info)
> >
> > --
> > 1.7.4.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Minchan Kim]

On 05/10/2012 10:39 AM, Inki Dae wrote:

> Hi Jerome,
> 
>> -----Original Message-----
>> From: Jerome Glisse [mailto:j.glisse@gmail.com]
>> Sent: Wednesday, May 09, 2012 11:46 PM
>> To: Inki Dae
>> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
>> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
>> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
>>
>> On Wed, May 9, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com> wrote:
>>> this feature is used to import user space region allocated by malloc()
>> or
>>> mmaped into a gem. and to guarantee the pages to user space not to be
>>> swapped out, the VMAs within the user space would be locked and then
>> unlocked
>>> when the pages are released.
>>>
>>> but this lock might result in significant degradation of system
>> performance
>>> because the pages couldn't be swapped out so we limit user-desired
>> userptr
>>> size to pre-defined.
>>>
>>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>
>>
>> Again i would like feedback from mm people (adding cc). I am not sure
> 
> Thank you, I missed adding mm as cc.
> 
>> locking the vma is the right anwser as i said in my previous mail,
>> userspace can munlock it in your back, maybe VM_RESERVED is better.
> 
> I know that with VM_RESERVED flag, also we can avoid the pages from being
> swapped out. but these pages should be unlocked anytime we want because we
> could allocate all pages on system and lock them, which in turn, it may
> result in significant deterioration of system performance.(maybe other
> processes requesting free memory would be blocked) so I used VM_LOCKED flags
> instead. but I'm not sure this way is best also.
> 
>> Anyway even not considering that you don't check at all that process
>> don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK
> 
> Thank you for your advices.
> 
>> for how it's done. Also you mlock complete vma but the userptr you get
>> might be inside say 16M vma and you only care about 1M of userptr, if
>> you mark the whole vma as locked than anytime a new page is fault in
>> the vma else where than in the buffer you are interested then it got
>> allocated for ever until the gem buffer is destroy, i am not sure of
>> what happen to the vma on next malloc if it grows or not (i would
>> think it won't grow at it would have different flags than new
>> anonymous memory).


I don't know history in detail because you didn't have sent full patches to linux-mm and
I didn't read the below code, either.
Just read your description and reply of Jerome. Apparently, there is something I missed.

Your goal is to avoid swap out some user pages which is used in kernel at the same time. Right?
Let's use get_user_pages. Is there any issue you can't use it?
It increases page count so reclaimer can't swap out page.
Isn't it enough?
Marking whole VMA into MLCOKED is overkill.

-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[KOSAKI Motohiro]

(5/10/12 12:58 AM), Minchan Kim wrote:
> On 05/10/2012 10:39 AM, Inki Dae wrote:
>
>> Hi Jerome,
>>
>>> -----Original Message-----
>>> From: Jerome Glisse [mailto:j.glisse@gmail.com]
>>> Sent: Wednesday, May 09, 2012 11:46 PM
>>> To: Inki Dae
>>> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
>>> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
>>> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
>>>
>>> On Wed, May 9, 2012 at 2:17 AM, Inki Dae<inki.dae@samsung.com>  wrote:
>>>> this feature is used to import user space region allocated by malloc()
>>> or
>>>> mmaped into a gem. and to guarantee the pages to user space not to be
>>>> swapped out, the VMAs within the user space would be locked and then
>>> unlocked
>>>> when the pages are released.
>>>>
>>>> but this lock might result in significant degradation of system
>>> performance
>>>> because the pages couldn't be swapped out so we limit user-desired
>>> userptr
>>>> size to pre-defined.
>>>>
>>>> Signed-off-by: Inki Dae<inki.dae@samsung.com>
>>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>>>
>>>
>>> Again i would like feedback from mm people (adding cc). I am not sure
>>
>> Thank you, I missed adding mm as cc.
>>
>>> locking the vma is the right anwser as i said in my previous mail,
>>> userspace can munlock it in your back, maybe VM_RESERVED is better.
>>
>> I know that with VM_RESERVED flag, also we can avoid the pages from being
>> swapped out. but these pages should be unlocked anytime we want because we
>> could allocate all pages on system and lock them, which in turn, it may
>> result in significant deterioration of system performance.(maybe other
>> processes requesting free memory would be blocked) so I used VM_LOCKED flags
>> instead. but I'm not sure this way is best also.
>>
>>> Anyway even not considering that you don't check at all that process
>>> don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK
>>
>> Thank you for your advices.
>>
>>> for how it's done. Also you mlock complete vma but the userptr you get
>>> might be inside say 16M vma and you only care about 1M of userptr, if
>>> you mark the whole vma as locked than anytime a new page is fault in
>>> the vma else where than in the buffer you are interested then it got
>>> allocated for ever until the gem buffer is destroy, i am not sure of
>>> what happen to the vma on next malloc if it grows or not (i would
>>> think it won't grow at it would have different flags than new
>>> anonymous memory).
>
>
> I don't know history in detail because you didn't have sent full patches to linux-mm and
> I didn't read the below code, either.
> Just read your description and reply of Jerome. Apparently, there is something I missed.
>
> Your goal is to avoid swap out some user pages which is used in kernel at the same time. Right?
> Let's use get_user_pages. Is there any issue you can't use it?

Maybe because get_user_pages() is fork unsafe? dunno.



> It increases page count so reclaimer can't swap out page.
> Isn't it enough?
> Marking whole VMA into MLCOKED is overkill.
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Minchan Kim]

On 05/10/2012 03:53 PM, KOSAKI Motohiro wrote:

> (5/10/12 12:58 AM), Minchan Kim wrote:
>> On 05/10/2012 10:39 AM, Inki Dae wrote:
>>
>>> Hi Jerome,
>>>
>>>> -----Original Message-----
>>>> From: Jerome Glisse [mailto:j.glisse@gmail.com]
>>>> Sent: Wednesday, May 09, 2012 11:46 PM
>>>> To: Inki Dae
>>>> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
>>>> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
>>>> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
>>>>
>>>> On Wed, May 9, 2012 at 2:17 AM, Inki Dae<inki.dae@samsung.com>  wrote:
>>>>> this feature is used to import user space region allocated by malloc()
>>>> or
>>>>> mmaped into a gem. and to guarantee the pages to user space not to be
>>>>> swapped out, the VMAs within the user space would be locked and then
>>>> unlocked
>>>>> when the pages are released.
>>>>>
>>>>> but this lock might result in significant degradation of system
>>>> performance
>>>>> because the pages couldn't be swapped out so we limit user-desired
>>>> userptr
>>>>> size to pre-defined.
>>>>>
>>>>> Signed-off-by: Inki Dae<inki.dae@samsung.com>
>>>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>>>>
>>>>
>>>> Again i would like feedback from mm people (adding cc). I am not sure
>>>
>>> Thank you, I missed adding mm as cc.
>>>
>>>> locking the vma is the right anwser as i said in my previous mail,
>>>> userspace can munlock it in your back, maybe VM_RESERVED is better.
>>>
>>> I know that with VM_RESERVED flag, also we can avoid the pages from
>>> being
>>> swapped out. but these pages should be unlocked anytime we want
>>> because we
>>> could allocate all pages on system and lock them, which in turn, it may
>>> result in significant deterioration of system performance.(maybe other
>>> processes requesting free memory would be blocked) so I used
>>> VM_LOCKED flags
>>> instead. but I'm not sure this way is best also.
>>>
>>>> Anyway even not considering that you don't check at all that process
>>>> don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK
>>>
>>> Thank you for your advices.
>>>
>>>> for how it's done. Also you mlock complete vma but the userptr you get
>>>> might be inside say 16M vma and you only care about 1M of userptr, if
>>>> you mark the whole vma as locked than anytime a new page is fault in
>>>> the vma else where than in the buffer you are interested then it got
>>>> allocated for ever until the gem buffer is destroy, i am not sure of
>>>> what happen to the vma on next malloc if it grows or not (i would
>>>> think it won't grow at it would have different flags than new
>>>> anonymous memory).
>>
>>
>> I don't know history in detail because you didn't have sent full
>> patches to linux-mm and
>> I didn't read the below code, either.
>> Just read your description and reply of Jerome. Apparently, there is
>> something I missed.
>>
>> Your goal is to avoid swap out some user pages which is used in kernel
>> at the same time. Right?
>> Let's use get_user_pages. Is there any issue you can't use it?
> 
> Maybe because get_user_pages() is fork unsafe? dunno.


If there is such problem, I think user program should handle it by MADV_DONTFORK 
and make to allow write by only parent process.

-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Kyungmin Park]

On 5/10/12, Minchan Kim <minchan@kernel.org> wrote:
> On 05/10/2012 03:53 PM, KOSAKI Motohiro wrote:
>
>> (5/10/12 12:58 AM), Minchan Kim wrote:
>>> On 05/10/2012 10:39 AM, Inki Dae wrote:
>>>
>>>> Hi Jerome,
>>>>
>>>>> -----Original Message-----
>>>>> From: Jerome Glisse [mailto:j.glisse@gmail.com]
>>>>> Sent: Wednesday, May 09, 2012 11:46 PM
>>>>> To: Inki Dae
>>>>> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
>>>>> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
>>>>> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
>>>>>
>>>>> On Wed, May 9, 2012 at 2:17 AM, Inki Dae<inki.dae@samsung.com>  wrote:
>>>>>> this feature is used to import user space region allocated by
>>>>>> malloc()
>>>>> or
>>>>>> mmaped into a gem. and to guarantee the pages to user space not to be
>>>>>> swapped out, the VMAs within the user space would be locked and then
>>>>> unlocked
>>>>>> when the pages are released.
>>>>>>
>>>>>> but this lock might result in significant degradation of system
>>>>> performance
>>>>>> because the pages couldn't be swapped out so we limit user-desired
>>>>> userptr
>>>>>> size to pre-defined.
>>>>>>
>>>>>> Signed-off-by: Inki Dae<inki.dae@samsung.com>
>>>>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>>>>>
>>>>>
>>>>> Again i would like feedback from mm people (adding cc). I am not sure
>>>>
>>>> Thank you, I missed adding mm as cc.
>>>>
>>>>> locking the vma is the right anwser as i said in my previous mail,
>>>>> userspace can munlock it in your back, maybe VM_RESERVED is better.
>>>>
>>>> I know that with VM_RESERVED flag, also we can avoid the pages from
>>>> being
>>>> swapped out. but these pages should be unlocked anytime we want
>>>> because we
>>>> could allocate all pages on system and lock them, which in turn, it may
>>>> result in significant deterioration of system performance.(maybe other
>>>> processes requesting free memory would be blocked) so I used
>>>> VM_LOCKED flags
>>>> instead. but I'm not sure this way is best also.
>>>>
>>>>> Anyway even not considering that you don't check at all that process
>>>>> don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK
>>>>
>>>> Thank you for your advices.
>>>>
>>>>> for how it's done. Also you mlock complete vma but the userptr you get
>>>>> might be inside say 16M vma and you only care about 1M of userptr, if
>>>>> you mark the whole vma as locked than anytime a new page is fault in
>>>>> the vma else where than in the buffer you are interested then it got
>>>>> allocated for ever until the gem buffer is destroy, i am not sure of
>>>>> what happen to the vma on next malloc if it grows or not (i would
>>>>> think it won't grow at it would have different flags than new
>>>>> anonymous memory).
>>>
>>>
>>> I don't know history in detail because you didn't have sent full
>>> patches to linux-mm and
>>> I didn't read the below code, either.
>>> Just read your description and reply of Jerome. Apparently, there is
>>> something I missed.
>>>
>>> Your goal is to avoid swap out some user pages which is used in kernel
>>> at the same time. Right?
>>> Let's use get_user_pages. Is there any issue you can't use it?
>>
>> Maybe because get_user_pages() is fork unsafe? dunno.
>
>
> If there is such problem, I think user program should handle it by
> MADV_DONTFORK
> and make to allow write by only parent process.
Please read the original patches and discuss the root cause. Does it
harm to pass user space memory to kernel space and how to make is
possible at DRM?

Thank you,
Kyungmin Park
>
> --
> Kind regards,
> Minchan Kim
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign
> http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Minchan Kim]

On 05/10/2012 04:31 PM, Kyungmin Park wrote:

> On 5/10/12, Minchan Kim <minchan@kernel.org> wrote:
>> On 05/10/2012 03:53 PM, KOSAKI Motohiro wrote:
>>
>>> (5/10/12 12:58 AM), Minchan Kim wrote:
>>>> On 05/10/2012 10:39 AM, Inki Dae wrote:
>>>>
>>>>> Hi Jerome,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Jerome Glisse [mailto:j.glisse@gmail.com]
>>>>>> Sent: Wednesday, May 09, 2012 11:46 PM
>>>>>> To: Inki Dae
>>>>>> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
>>>>>> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
>>>>>> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
>>>>>>
>>>>>> On Wed, May 9, 2012 at 2:17 AM, Inki Dae<inki.dae@samsung.com>  wrote:
>>>>>>> this feature is used to import user space region allocated by
>>>>>>> malloc()
>>>>>> or
>>>>>>> mmaped into a gem. and to guarantee the pages to user space not to be
>>>>>>> swapped out, the VMAs within the user space would be locked and then
>>>>>> unlocked
>>>>>>> when the pages are released.
>>>>>>>
>>>>>>> but this lock might result in significant degradation of system
>>>>>> performance
>>>>>>> because the pages couldn't be swapped out so we limit user-desired
>>>>>> userptr
>>>>>>> size to pre-defined.
>>>>>>>
>>>>>>> Signed-off-by: Inki Dae<inki.dae@samsung.com>
>>>>>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>>>>>>
>>>>>>
>>>>>> Again i would like feedback from mm people (adding cc). I am not sure
>>>>>
>>>>> Thank you, I missed adding mm as cc.
>>>>>
>>>>>> locking the vma is the right anwser as i said in my previous mail,
>>>>>> userspace can munlock it in your back, maybe VM_RESERVED is better.
>>>>>
>>>>> I know that with VM_RESERVED flag, also we can avoid the pages from
>>>>> being
>>>>> swapped out. but these pages should be unlocked anytime we want
>>>>> because we
>>>>> could allocate all pages on system and lock them, which in turn, it may
>>>>> result in significant deterioration of system performance.(maybe other
>>>>> processes requesting free memory would be blocked) so I used
>>>>> VM_LOCKED flags
>>>>> instead. but I'm not sure this way is best also.
>>>>>
>>>>>> Anyway even not considering that you don't check at all that process
>>>>>> don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK
>>>>>
>>>>> Thank you for your advices.
>>>>>
>>>>>> for how it's done. Also you mlock complete vma but the userptr you get
>>>>>> might be inside say 16M vma and you only care about 1M of userptr, if
>>>>>> you mark the whole vma as locked than anytime a new page is fault in
>>>>>> the vma else where than in the buffer you are interested then it got
>>>>>> allocated for ever until the gem buffer is destroy, i am not sure of
>>>>>> what happen to the vma on next malloc if it grows or not (i would
>>>>>> think it won't grow at it would have different flags than new
>>>>>> anonymous memory).
>>>>
>>>>
>>>> I don't know history in detail because you didn't have sent full
>>>> patches to linux-mm and
>>>> I didn't read the below code, either.
>>>> Just read your description and reply of Jerome. Apparently, there is
>>>> something I missed.
>>>>
>>>> Your goal is to avoid swap out some user pages which is used in kernel
>>>> at the same time. Right?
>>>> Let's use get_user_pages. Is there any issue you can't use it?
>>>
>>> Maybe because get_user_pages() is fork unsafe? dunno.
>>
>>
>> If there is such problem, I think user program should handle it by
>> MADV_DONTFORK
>> and make to allow write by only parent process.
> Please read the original patches and discuss the root cause. Does it
> harm to pass user space memory to kernel space and how to make is
> possible at DRM?


Where can I read original discussion history?
I am not expert of DRAM so I can answer only mm stuff and it's why Jerome ccing mm-list.
About mm stuff, I think it's no harm for kernel to use user space memory if it uses carefully.
If you are saying about permission, at least, DRM code can check it by can_do_mlock and checking lock_limit.
If you are saying another security, I'm not right person to discuss it.
please Ccing security@kernel.org.

Thanks.

> 
> Thank you,
> Kyungmin Park
>>
>> --
>> Kind regards,
>> Minchan Kim
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Fight unfair telecom internet charges in Canada: sign
>> http://stopthemeter.ca/
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 



-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Minchan Kim]

On 05/10/2012 04:56 PM, Minchan Kim wrote:

> On 05/10/2012 04:31 PM, Kyungmin Park wrote:
> 
>> On 5/10/12, Minchan Kim <minchan@kernel.org> wrote:
>>> On 05/10/2012 03:53 PM, KOSAKI Motohiro wrote:
>>>
>>>> (5/10/12 12:58 AM), Minchan Kim wrote:
>>>>> On 05/10/2012 10:39 AM, Inki Dae wrote:
>>>>>
>>>>>> Hi Jerome,
>>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Jerome Glisse [mailto:j.glisse@gmail.com]
>>>>>>> Sent: Wednesday, May 09, 2012 11:46 PM
>>>>>>> To: Inki Dae
>>>>>>> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
>>>>>>> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
>>>>>>> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
>>>>>>>
>>>>>>> On Wed, May 9, 2012 at 2:17 AM, Inki Dae<inki.dae@samsung.com>  wrote:
>>>>>>>> this feature is used to import user space region allocated by
>>>>>>>> malloc()
>>>>>>> or
>>>>>>>> mmaped into a gem. and to guarantee the pages to user space not to be
>>>>>>>> swapped out, the VMAs within the user space would be locked and then
>>>>>>> unlocked
>>>>>>>> when the pages are released.
>>>>>>>>
>>>>>>>> but this lock might result in significant degradation of system
>>>>>>> performance
>>>>>>>> because the pages couldn't be swapped out so we limit user-desired
>>>>>>> userptr
>>>>>>>> size to pre-defined.
>>>>>>>>
>>>>>>>> Signed-off-by: Inki Dae<inki.dae@samsung.com>
>>>>>>>> Signed-off-by: Kyungmin Park<kyungmin.park@samsung.com>
>>>>>>>
>>>>>>>
>>>>>>> Again i would like feedback from mm people (adding cc). I am not sure
>>>>>>
>>>>>> Thank you, I missed adding mm as cc.
>>>>>>
>>>>>>> locking the vma is the right anwser as i said in my previous mail,
>>>>>>> userspace can munlock it in your back, maybe VM_RESERVED is better.
>>>>>>
>>>>>> I know that with VM_RESERVED flag, also we can avoid the pages from
>>>>>> being
>>>>>> swapped out. but these pages should be unlocked anytime we want
>>>>>> because we
>>>>>> could allocate all pages on system and lock them, which in turn, it may
>>>>>> result in significant deterioration of system performance.(maybe other
>>>>>> processes requesting free memory would be blocked) so I used
>>>>>> VM_LOCKED flags
>>>>>> instead. but I'm not sure this way is best also.
>>>>>>
>>>>>>> Anyway even not considering that you don't check at all that process
>>>>>>> don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK
>>>>>>
>>>>>> Thank you for your advices.
>>>>>>
>>>>>>> for how it's done. Also you mlock complete vma but the userptr you get
>>>>>>> might be inside say 16M vma and you only care about 1M of userptr, if
>>>>>>> you mark the whole vma as locked than anytime a new page is fault in
>>>>>>> the vma else where than in the buffer you are interested then it got
>>>>>>> allocated for ever until the gem buffer is destroy, i am not sure of
>>>>>>> what happen to the vma on next malloc if it grows or not (i would
>>>>>>> think it won't grow at it would have different flags than new
>>>>>>> anonymous memory).
>>>>>
>>>>>
>>>>> I don't know history in detail because you didn't have sent full
>>>>> patches to linux-mm and
>>>>> I didn't read the below code, either.
>>>>> Just read your description and reply of Jerome. Apparently, there is
>>>>> something I missed.
>>>>>
>>>>> Your goal is to avoid swap out some user pages which is used in kernel
>>>>> at the same time. Right?
>>>>> Let's use get_user_pages. Is there any issue you can't use it?
>>>>
>>>> Maybe because get_user_pages() is fork unsafe? dunno.
>>>
>>>
>>> If there is such problem, I think user program should handle it by
>>> MADV_DONTFORK
>>> and make to allow write by only parent process.
>> Please read the original patches and discuss the root cause. Does it
>> harm to pass user space memory to kernel space and how to make is
>> possible at DRM?
> 
> 
> Where can I read original discussion history?
> I am not expert of DRAM so I can answer only mm stuff and it's why Jerome ccing mm-list.

                     ^^^ silly typo
                     DRM

-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Inki Dae]

> -----Original Message-----
> From: Minchan Kim [mailto:minchan@kernel.org]
> Sent: Thursday, May 10, 2012 1:58 PM
> To: Inki Dae
> Cc: 'Jerome Glisse'; airlied@linux.ie; dri-devel@lists.freedesktop.org;
> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
> 
> On 05/10/2012 10:39 AM, Inki Dae wrote:
> 
> > Hi Jerome,
> >
> >> -----Original Message-----
> >> From: Jerome Glisse [mailto:j.glisse@gmail.com]
> >> Sent: Wednesday, May 09, 2012 11:46 PM
> >> To: Inki Dae
> >> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
> >> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
> >> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
> >>
> >> On Wed, May 9, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com> wrote:
> >>> this feature is used to import user space region allocated by malloc()
> >> or
> >>> mmaped into a gem. and to guarantee the pages to user space not to be
> >>> swapped out, the VMAs within the user space would be locked and then
> >> unlocked
> >>> when the pages are released.
> >>>
> >>> but this lock might result in significant degradation of system
> >> performance
> >>> because the pages couldn't be swapped out so we limit user-desired
> >> userptr
> >>> size to pre-defined.
> >>>
> >>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> >>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >>
> >>
> >> Again i would like feedback from mm people (adding cc). I am not sure
> >
> > Thank you, I missed adding mm as cc.
> >
> >> locking the vma is the right anwser as i said in my previous mail,
> >> userspace can munlock it in your back, maybe VM_RESERVED is better.
> >
> > I know that with VM_RESERVED flag, also we can avoid the pages from
> being
> > swapped out. but these pages should be unlocked anytime we want because
> we
> > could allocate all pages on system and lock them, which in turn, it may
> > result in significant deterioration of system performance.(maybe other
> > processes requesting free memory would be blocked) so I used VM_LOCKED
> flags
> > instead. but I'm not sure this way is best also.
> >
> >> Anyway even not considering that you don't check at all that process
> >> don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK
> >
> > Thank you for your advices.
> >
> >> for how it's done. Also you mlock complete vma but the userptr you get
> >> might be inside say 16M vma and you only care about 1M of userptr, if
> >> you mark the whole vma as locked than anytime a new page is fault in
> >> the vma else where than in the buffer you are interested then it got
> >> allocated for ever until the gem buffer is destroy, i am not sure of
> >> what happen to the vma on next malloc if it grows or not (i would
> >> think it won't grow at it would have different flags than new
> >> anonymous memory).
> 
> 
> I don't know history in detail because you didn't have sent full patches
> to linux-mm and
> I didn't read the below code, either.
> Just read your description and reply of Jerome. Apparently, there is
> something I missed.
> 
> Your goal is to avoid swap out some user pages which is used in kernel at
> the same time. Right?
> Let's use get_user_pages. Is there any issue you can't use it?
> It increases page count so reclaimer can't swap out page.
> Isn't it enough?
> Marking whole VMA into MLCOKED is overkill.
> 

As I mentioned, we are already using get_user_pages. as you said, this
function increases page count but just only things to the user address space
cpu already accessed. other would be allocated by page fault hander once
get_user_pages call. if so... ok, after that refcount(page->_count) of the
pages user already accessed would have 2 and just 1 for other all pages. so
we may have to consider only pages never accessed by cpu to be locked to
avoid from swapped out.

Thanks,
Inki Dae

> --
> Kind regards,
> Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Minchan Kim]

On 05/10/2012 03:57 PM, Inki Dae wrote:

> 
> 
>> -----Original Message-----
>> From: Minchan Kim [mailto:minchan@kernel.org]
>> Sent: Thursday, May 10, 2012 1:58 PM
>> To: Inki Dae
>> Cc: 'Jerome Glisse'; airlied@linux.ie; dri-devel@lists.freedesktop.org;
>> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
>> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
>>
>> On 05/10/2012 10:39 AM, Inki Dae wrote:
>>
>>> Hi Jerome,
>>>
>>>> -----Original Message-----
>>>> From: Jerome Glisse [mailto:j.glisse@gmail.com]
>>>> Sent: Wednesday, May 09, 2012 11:46 PM
>>>> To: Inki Dae
>>>> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
>>>> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
>>>> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
>>>>
>>>> On Wed, May 9, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com> wrote:
>>>>> this feature is used to import user space region allocated by malloc()
>>>> or
>>>>> mmaped into a gem. and to guarantee the pages to user space not to be
>>>>> swapped out, the VMAs within the user space would be locked and then
>>>> unlocked
>>>>> when the pages are released.
>>>>>
>>>>> but this lock might result in significant degradation of system
>>>> performance
>>>>> because the pages couldn't be swapped out so we limit user-desired
>>>> userptr
>>>>> size to pre-defined.
>>>>>
>>>>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>>
>>>>
>>>> Again i would like feedback from mm people (adding cc). I am not sure
>>>
>>> Thank you, I missed adding mm as cc.
>>>
>>>> locking the vma is the right anwser as i said in my previous mail,
>>>> userspace can munlock it in your back, maybe VM_RESERVED is better.
>>>
>>> I know that with VM_RESERVED flag, also we can avoid the pages from
>> being
>>> swapped out. but these pages should be unlocked anytime we want because
>> we
>>> could allocate all pages on system and lock them, which in turn, it may
>>> result in significant deterioration of system performance.(maybe other
>>> processes requesting free memory would be blocked) so I used VM_LOCKED
>> flags
>>> instead. but I'm not sure this way is best also.
>>>
>>>> Anyway even not considering that you don't check at all that process
>>>> don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK
>>>
>>> Thank you for your advices.
>>>
>>>> for how it's done. Also you mlock complete vma but the userptr you get
>>>> might be inside say 16M vma and you only care about 1M of userptr, if
>>>> you mark the whole vma as locked than anytime a new page is fault in
>>>> the vma else where than in the buffer you are interested then it got
>>>> allocated for ever until the gem buffer is destroy, i am not sure of
>>>> what happen to the vma on next malloc if it grows or not (i would
>>>> think it won't grow at it would have different flags than new
>>>> anonymous memory).
>>
>>
>> I don't know history in detail because you didn't have sent full patches
>> to linux-mm and
>> I didn't read the below code, either.
>> Just read your description and reply of Jerome. Apparently, there is
>> something I missed.
>>
>> Your goal is to avoid swap out some user pages which is used in kernel at
>> the same time. Right?
>> Let's use get_user_pages. Is there any issue you can't use it?
>> It increases page count so reclaimer can't swap out page.
>> Isn't it enough?
>> Marking whole VMA into MLCOKED is overkill.
>>
> 
> As I mentioned, we are already using get_user_pages. as you said, this
> function increases page count but just only things to the user address space
> cpu already accessed. other would be allocated by page fault hander once
> get_user_pages call. if so... ok, after that refcount(page->_count) of the


Not true. Look __get_user_pages.
It handles case you mentioned by handle_mm_fault.
Do I miss something?

> pages user already accessed would have 2 and just 1 for other all pages. so
> we may have to consider only pages never accessed by cpu to be locked to
> avoid from swapped out.
> 
> Thanks,
> Inki Dae
> 
>> --
>> Kind regards,
>> Minchan Kim
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 



-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[InKi Dae]

2012/5/10, Minchan Kim <minchan@kernel.org>:
> On 05/10/2012 03:57 PM, Inki Dae wrote:
>
>>
>>
>>> -----Original Message-----
>>> From: Minchan Kim [mailto:minchan@kernel.org]
>>> Sent: Thursday, May 10, 2012 1:58 PM
>>> To: Inki Dae
>>> Cc: 'Jerome Glisse'; airlied@linux.ie; dri-devel@lists.freedesktop.org;
>>> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
>>> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
>>>
>>> On 05/10/2012 10:39 AM, Inki Dae wrote:
>>>
>>>> Hi Jerome,
>>>>
>>>>> -----Original Message-----
>>>>> From: Jerome Glisse [mailto:j.glisse@gmail.com]
>>>>> Sent: Wednesday, May 09, 2012 11:46 PM
>>>>> To: Inki Dae
>>>>> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
>>>>> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
>>>>> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
>>>>>
>>>>> On Wed, May 9, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com> wrote:
>>>>>> this feature is used to import user space region allocated by
>>>>>> malloc()
>>>>> or
>>>>>> mmaped into a gem. and to guarantee the pages to user space not to be
>>>>>> swapped out, the VMAs within the user space would be locked and then
>>>>> unlocked
>>>>>> when the pages are released.
>>>>>>
>>>>>> but this lock might result in significant degradation of system
>>>>> performance
>>>>>> because the pages couldn't be swapped out so we limit user-desired
>>>>> userptr
>>>>>> size to pre-defined.
>>>>>>
>>>>>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>>>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>>>
>>>>>
>>>>> Again i would like feedback from mm people (adding cc). I am not sure
>>>>
>>>> Thank you, I missed adding mm as cc.
>>>>
>>>>> locking the vma is the right anwser as i said in my previous mail,
>>>>> userspace can munlock it in your back, maybe VM_RESERVED is better.
>>>>
>>>> I know that with VM_RESERVED flag, also we can avoid the pages from
>>> being
>>>> swapped out. but these pages should be unlocked anytime we want because
>>> we
>>>> could allocate all pages on system and lock them, which in turn, it may
>>>> result in significant deterioration of system performance.(maybe other
>>>> processes requesting free memory would be blocked) so I used VM_LOCKED
>>> flags
>>>> instead. but I'm not sure this way is best also.
>>>>
>>>>> Anyway even not considering that you don't check at all that process
>>>>> don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK
>>>>
>>>> Thank you for your advices.
>>>>
>>>>> for how it's done. Also you mlock complete vma but the userptr you get
>>>>> might be inside say 16M vma and you only care about 1M of userptr, if
>>>>> you mark the whole vma as locked than anytime a new page is fault in
>>>>> the vma else where than in the buffer you are interested then it got
>>>>> allocated for ever until the gem buffer is destroy, i am not sure of
>>>>> what happen to the vma on next malloc if it grows or not (i would
>>>>> think it won't grow at it would have different flags than new
>>>>> anonymous memory).
>>>
>>>
>>> I don't know history in detail because you didn't have sent full patches
>>> to linux-mm and
>>> I didn't read the below code, either.
>>> Just read your description and reply of Jerome. Apparently, there is
>>> something I missed.
>>>
>>> Your goal is to avoid swap out some user pages which is used in kernel
>>> at
>>> the same time. Right?
>>> Let's use get_user_pages. Is there any issue you can't use it?
>>> It increases page count so reclaimer can't swap out page.
>>> Isn't it enough?
>>> Marking whole VMA into MLCOKED is overkill.
>>>
>>
>> As I mentioned, we are already using get_user_pages. as you said, this
>> function increases page count but just only things to the user address
>> space
>> cpu already accessed. other would be allocated by page fault hander once
>> get_user_pages call. if so... ok, after that refcount(page->_count) of
>> the
>
>
> Not true. Look __get_user_pages.
> It handles case you mentioned by handle_mm_fault.
> Do I miss something?
>

let's assume that one application want to allocate user space memory
region using malloc() and then write something on the region. as you
may know, user space buffer doen't have real physical pages once
malloc() call so if user tries to access the region then page fault
handler would be triggered and then in turn next process like swap in
to fill physical frame number into entry of the page faulted. of
course, if user never access the buffer and requested userptr then
handle_mm_fault would be called by __get_user_pages. please give me
any comments if there is my missing point.

Thanks,
Inki Dae


>> pages user already accessed would have 2 and just 1 for other all pages.
>> so
>> we may have to consider only pages never accessed by cpu to be locked to
>> avoid from swapped out.
>>
>> Thanks,
>> Inki Dae
>>
>>> --
>>> Kind regards,
>>> Minchan Kim
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Fight unfair telecom internet charges in Canada: sign
>> http://stopthemeter.ca/
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>
>
>
>
> --
> Kind regards,
> Minchan Kim
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign
> http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Minchan Kim]

On 05/10/2012 04:59 PM, InKi Dae wrote:

> 2012/5/10, Minchan Kim <minchan@kernel.org>:
>> On 05/10/2012 03:57 PM, Inki Dae wrote:
>>
>>>
>>>
>>>> -----Original Message-----
>>>> From: Minchan Kim [mailto:minchan@kernel.org]
>>>> Sent: Thursday, May 10, 2012 1:58 PM
>>>> To: Inki Dae
>>>> Cc: 'Jerome Glisse'; airlied@linux.ie; dri-devel@lists.freedesktop.org;
>>>> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
>>>> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
>>>>
>>>> On 05/10/2012 10:39 AM, Inki Dae wrote:
>>>>
>>>>> Hi Jerome,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Jerome Glisse [mailto:j.glisse@gmail.com]
>>>>>> Sent: Wednesday, May 09, 2012 11:46 PM
>>>>>> To: Inki Dae
>>>>>> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
>>>>>> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
>>>>>> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
>>>>>>
>>>>>> On Wed, May 9, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com> wrote:
>>>>>>> this feature is used to import user space region allocated by
>>>>>>> malloc()
>>>>>> or
>>>>>>> mmaped into a gem. and to guarantee the pages to user space not to be
>>>>>>> swapped out, the VMAs within the user space would be locked and then
>>>>>> unlocked
>>>>>>> when the pages are released.
>>>>>>>
>>>>>>> but this lock might result in significant degradation of system
>>>>>> performance
>>>>>>> because the pages couldn't be swapped out so we limit user-desired
>>>>>> userptr
>>>>>>> size to pre-defined.
>>>>>>>
>>>>>>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
>>>>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>>>>
>>>>>>
>>>>>> Again i would like feedback from mm people (adding cc). I am not sure
>>>>>
>>>>> Thank you, I missed adding mm as cc.
>>>>>
>>>>>> locking the vma is the right anwser as i said in my previous mail,
>>>>>> userspace can munlock it in your back, maybe VM_RESERVED is better.
>>>>>
>>>>> I know that with VM_RESERVED flag, also we can avoid the pages from
>>>> being
>>>>> swapped out. but these pages should be unlocked anytime we want because
>>>> we
>>>>> could allocate all pages on system and lock them, which in turn, it may
>>>>> result in significant deterioration of system performance.(maybe other
>>>>> processes requesting free memory would be blocked) so I used VM_LOCKED
>>>> flags
>>>>> instead. but I'm not sure this way is best also.
>>>>>
>>>>>> Anyway even not considering that you don't check at all that process
>>>>>> don't go over the limit of locked page see mm/mlock.c RLIMIT_MEMLOCK
>>>>>
>>>>> Thank you for your advices.
>>>>>
>>>>>> for how it's done. Also you mlock complete vma but the userptr you get
>>>>>> might be inside say 16M vma and you only care about 1M of userptr, if
>>>>>> you mark the whole vma as locked than anytime a new page is fault in
>>>>>> the vma else where than in the buffer you are interested then it got
>>>>>> allocated for ever until the gem buffer is destroy, i am not sure of
>>>>>> what happen to the vma on next malloc if it grows or not (i would
>>>>>> think it won't grow at it would have different flags than new
>>>>>> anonymous memory).
>>>>
>>>>
>>>> I don't know history in detail because you didn't have sent full patches
>>>> to linux-mm and
>>>> I didn't read the below code, either.
>>>> Just read your description and reply of Jerome. Apparently, there is
>>>> something I missed.
>>>>
>>>> Your goal is to avoid swap out some user pages which is used in kernel
>>>> at
>>>> the same time. Right?
>>>> Let's use get_user_pages. Is there any issue you can't use it?
>>>> It increases page count so reclaimer can't swap out page.
>>>> Isn't it enough?
>>>> Marking whole VMA into MLCOKED is overkill.
>>>>
>>>
>>> As I mentioned, we are already using get_user_pages. as you said, this
>>> function increases page count but just only things to the user address
>>> space
>>> cpu already accessed. other would be allocated by page fault hander once
>>> get_user_pages call. if so... ok, after that refcount(page->_count) of
>>> the
>>
>>
>> Not true. Look __get_user_pages.
>> It handles case you mentioned by handle_mm_fault.
>> Do I miss something?
>>
> 
> let's assume that one application want to allocate user space memory
> region using malloc() and then write something on the region. as you
> may know, user space buffer doen't have real physical pages once
> malloc() call so if user tries to access the region then page fault
> handler would be triggered


Understood.

> and then in turn next process like swap in to fill physical frame number into entry of the page faulted.


Sorry, I can't understand your point due to my poor English.
Could you rewrite it easiliy? :)

Thanks.

> of course,if user never access the buffer and requested userptr then

> handle_mm_fault would be called by __get_user_pages. please give me
> any comments if there is my missing point.
> 
> Thanks,
> Inki Dae
> 
> 
>>> pages user already accessed would have 2 and just 1 for other all pages.
>>> so
>>> we may have to consider only pages never accessed by cpu to be locked to
>>> avoid from swapped out.
>>>
>>> Thanks,
>>> Inki Dae
>>>
>>>> --
>>>> Kind regards,
>>>> Minchan Kim
>>>
>>> --
>>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>>> the body to majordomo@kvack.org.  For more info on Linux MM,
>>> see: http://www.linux-mm.org/ .
>>> Fight unfair telecom internet charges in Canada: sign
>>> http://stopthemeter.ca/
>>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>>
>>
>>
>>
>> --
>> Kind regards,
>> Minchan Kim
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Fight unfair telecom internet charges in Canada: sign
>> http://stopthemeter.ca/
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>>
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 



-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Inki Dae]

> -----Original Message-----
> From: Minchan Kim [mailto:minchan@kernel.org]
> Sent: Thursday, May 10, 2012 5:11 PM
> To: InKi Dae
> Cc: Inki Dae; Jerome Glisse; airlied@linux.ie; dri-
> devel@lists.freedesktop.org; kyungmin.park@samsung.com;
> sw0312.kim@samsung.com; linux-mm@kvack.org
> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
> 
> On 05/10/2012 04:59 PM, InKi Dae wrote:
> 
> > 2012/5/10, Minchan Kim <minchan@kernel.org>:
> >> On 05/10/2012 03:57 PM, Inki Dae wrote:
> >>
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Minchan Kim [mailto:minchan@kernel.org]
> >>>> Sent: Thursday, May 10, 2012 1:58 PM
> >>>> To: Inki Dae
> >>>> Cc: 'Jerome Glisse'; airlied@linux.ie; dri-
> devel@lists.freedesktop.org;
> >>>> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-mm@kvack.org
> >>>> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
> >>>>
> >>>> On 05/10/2012 10:39 AM, Inki Dae wrote:
> >>>>
> >>>>> Hi Jerome,
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Jerome Glisse [mailto:j.glisse@gmail.com]
> >>>>>> Sent: Wednesday, May 09, 2012 11:46 PM
> >>>>>> To: Inki Dae
> >>>>>> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
> >>>>>> kyungmin.park@samsung.com; sw0312.kim@samsung.com; linux-
> mm@kvack.org
> >>>>>> Subject: Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.
> >>>>>>
> >>>>>> On Wed, May 9, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com>
> wrote:
> >>>>>>> this feature is used to import user space region allocated by
> >>>>>>> malloc()
> >>>>>> or
> >>>>>>> mmaped into a gem. and to guarantee the pages to user space not to
> be
> >>>>>>> swapped out, the VMAs within the user space would be locked and
> then
> >>>>>> unlocked
> >>>>>>> when the pages are released.
> >>>>>>>
> >>>>>>> but this lock might result in significant degradation of system
> >>>>>> performance
> >>>>>>> because the pages couldn't be swapped out so we limit user-desired
> >>>>>> userptr
> >>>>>>> size to pre-defined.
> >>>>>>>
> >>>>>>> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> >>>>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >>>>>>
> >>>>>>
> >>>>>> Again i would like feedback from mm people (adding cc). I am not
> sure
> >>>>>
> >>>>> Thank you, I missed adding mm as cc.
> >>>>>
> >>>>>> locking the vma is the right anwser as i said in my previous mail,
> >>>>>> userspace can munlock it in your back, maybe VM_RESERVED is better.
> >>>>>
> >>>>> I know that with VM_RESERVED flag, also we can avoid the pages from
> >>>> being
> >>>>> swapped out. but these pages should be unlocked anytime we want
> because
> >>>> we
> >>>>> could allocate all pages on system and lock them, which in turn, it
> may
> >>>>> result in significant deterioration of system performance.(maybe
> other
> >>>>> processes requesting free memory would be blocked) so I used
> VM_LOCKED
> >>>> flags
> >>>>> instead. but I'm not sure this way is best also.
> >>>>>
> >>>>>> Anyway even not considering that you don't check at all that
> process
> >>>>>> don't go over the limit of locked page see mm/mlock.c
> RLIMIT_MEMLOCK
> >>>>>
> >>>>> Thank you for your advices.
> >>>>>
> >>>>>> for how it's done. Also you mlock complete vma but the userptr you
> get
> >>>>>> might be inside say 16M vma and you only care about 1M of userptr,
> if
> >>>>>> you mark the whole vma as locked than anytime a new page is fault
> in
> >>>>>> the vma else where than in the buffer you are interested then it
> got
> >>>>>> allocated for ever until the gem buffer is destroy, i am not sure
> of
> >>>>>> what happen to the vma on next malloc if it grows or not (i would
> >>>>>> think it won't grow at it would have different flags than new
> >>>>>> anonymous memory).
> >>>>
> >>>>
> >>>> I don't know history in detail because you didn't have sent full
> patches
> >>>> to linux-mm and
> >>>> I didn't read the below code, either.
> >>>> Just read your description and reply of Jerome. Apparently, there is
> >>>> something I missed.
> >>>>
> >>>> Your goal is to avoid swap out some user pages which is used in
> kernel
> >>>> at
> >>>> the same time. Right?
> >>>> Let's use get_user_pages. Is there any issue you can't use it?
> >>>> It increases page count so reclaimer can't swap out page.
> >>>> Isn't it enough?
> >>>> Marking whole VMA into MLCOKED is overkill.
> >>>>
> >>>
> >>> As I mentioned, we are already using get_user_pages. as you said, this
> >>> function increases page count but just only things to the user address
> >>> space
> >>> cpu already accessed. other would be allocated by page fault hander
> once
> >>> get_user_pages call. if so... ok, after that refcount(page->_count) of
> >>> the
> >>
> >>
> >> Not true. Look __get_user_pages.
> >> It handles case you mentioned by handle_mm_fault.
> >> Do I miss something?
> >>
> >
> > let's assume that one application want to allocate user space memory
> > region using malloc() and then write something on the region. as you
> > may know, user space buffer doen't have real physical pages once
> > malloc() call so if user tries to access the region then page fault
> > handler would be triggered
> 
> 
> Understood.
> 
> > and then in turn next process like swap in to fill physical frame number
> into entry of the page faulted.
> 
> 
> Sorry, I can't understand your point due to my poor English.
> Could you rewrite it easiliy? :)
> 

Simply saying, handle_mm_fault would be called to update pte after finding
vma and checking access right. and as you know, there are many cases to
process page fault such as COW or demand paging.

Thanks,
Inki Dae

> Thanks.
> 
> > of course,if user never access the buffer and requested userptr then
> 
> > handle_mm_fault would be called by __get_user_pages. please give me
> > any comments if there is my missing point.
> >
> > Thanks,
> > Inki Dae
> >
> >
> >>> pages user already accessed would have 2 and just 1 for other all
> pages.
> >>> so
> >>> we may have to consider only pages never accessed by cpu to be locked
> to
> >>> avoid from swapped out.
> >>>
> >>> Thanks,
> >>> Inki Dae
> >>>
> >>>> --
> >>>> Kind regards,
> >>>> Minchan Kim
> >>>
> >>> --
> >>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> >>> the body to majordomo@kvack.org.  For more info on Linux MM,
> >>> see: http://www.linux-mm.org/ .
> >>> Fight unfair telecom internet charges in Canada: sign
> >>> http://stopthemeter.ca/
> >>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> >>>
> >>
> >>
> >>
> >> --
> >> Kind regards,
> >> Minchan Kim
> >>
> >> --
> >> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> >> the body to majordomo@kvack.org.  For more info on Linux MM,
> >> see: http://www.linux-mm.org/ .
> >> Fight unfair telecom internet charges in Canada: sign
> >> http://stopthemeter.ca/
> >> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> >>
> >
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > the body to majordomo@kvack.org.  For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Fight unfair telecom internet charges in Canada: sign
> http://stopthemeter.ca/
> > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> >
> 
> 
> 
> --
> Kind regards,
> Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[KOSAKI Motohiro]

>>> let's assume that one application want to allocate user space memory
>>> region using malloc() and then write something on the region. as you
>>> may know, user space buffer doen't have real physical pages once
>>> malloc() call so if user tries to access the region then page fault
>>> handler would be triggered
>>
>>
>> Understood.
>>
>>> and then in turn next process like swap in to fill physical frame number
>> into entry of the page faulted.
>>
>>
>> Sorry, I can't understand your point due to my poor English.
>> Could you rewrite it easiliy? :)
>>
>
> Simply saying, handle_mm_fault would be called to update pte after finding
> vma and checking access right. and as you know, there are many cases to
> process page fault such as COW or demand paging.

Hmm. If I understand correctly, you guys misunderstand mlock. it doesn't page pinning
nor prevent pfn change. It only guarantee to don't make swap out. e.g. memory campaction
feature may automatically change page physical address.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Minchan Kim]

Hi KOSAKI,

On 05/11/2012 02:53 AM, KOSAKI Motohiro wrote:

>>>> let's assume that one application want to allocate user space memory
>>>> region using malloc() and then write something on the region. as you
>>>> may know, user space buffer doen't have real physical pages once
>>>> malloc() call so if user tries to access the region then page fault
>>>> handler would be triggered
>>>
>>>
>>> Understood.
>>>
>>>> and then in turn next process like swap in to fill physical frame
>>>> number
>>> into entry of the page faulted.
>>>
>>>
>>> Sorry, I can't understand your point due to my poor English.
>>> Could you rewrite it easiliy? :)
>>>
>>
>> Simply saying, handle_mm_fault would be called to update pte after
>> finding
>> vma and checking access right. and as you know, there are many cases to
>> process page fault such as COW or demand paging.
> 
> Hmm. If I understand correctly, you guys misunderstand mlock. it doesn't
> page pinning
> nor prevent pfn change. It only guarantee to don't make swap out. e.g.


Symantic point of view, you're right but the implementation makes sure page pinning.

> memory campaction
> feature may automatically change page physical address.


I tried it last year but decided drop by realtime issue.
https://lkml.org/lkml/2011/8/29/295

so I think mlock is a kind of page pinning. If elsewhere I don't realized is doing, that place should be fixed.
Or my above patch should go ahead.

> 
> 
> -- 
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign
> http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 



-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[KOSAKI Motohiro]

(5/10/12 8:50 PM), Minchan Kim wrote:
> Hi KOSAKI,
>
> On 05/11/2012 02:53 AM, KOSAKI Motohiro wrote:
>
>>>>> let's assume that one application want to allocate user space memory
>>>>> region using malloc() and then write something on the region. as you
>>>>> may know, user space buffer doen't have real physical pages once
>>>>> malloc() call so if user tries to access the region then page fault
>>>>> handler would be triggered
>>>>
>>>>
>>>> Understood.
>>>>
>>>>> and then in turn next process like swap in to fill physical frame
>>>>> number
>>>> into entry of the page faulted.
>>>>
>>>>
>>>> Sorry, I can't understand your point due to my poor English.
>>>> Could you rewrite it easiliy? :)
>>>>
>>>
>>> Simply saying, handle_mm_fault would be called to update pte after
>>> finding
>>> vma and checking access right. and as you know, there are many cases to
>>> process page fault such as COW or demand paging.
>>
>> Hmm. If I understand correctly, you guys misunderstand mlock. it doesn't
>> page pinning
>> nor prevent pfn change. It only guarantee to don't make swap out. e.g.
>
>
> Symantic point of view, you're right but the implementation makes sure page pinning.
>
>> memory campaction
>> feature may automatically change page physical address.
>
>
> I tried it last year but decided drop by realtime issue.
> https://lkml.org/lkml/2011/8/29/295
>
> so I think mlock is a kind of page pinning. If elsewhere I don't realized is doing, that place should be fixed.
> Or my above patch should go ahead.

Thanks pointing out. I didn't realized your patch didn't merged. I think it should go ahead. think autonuma case,
if mlock disable autonuma migration, that's bug.  I don't think we can promise mlock don't change physical page.
I wonder if any realtime guys page migration is free lunch. they should disable both auto migration and compaction.

And, think if application explictly use migrate_pages(2) or admins uses cpusets. driver code can't assume such scenario
doesn't occur, yes?


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Jerome Glisse]

On Thu, May 10, 2012 at 10:51 PM, KOSAKI Motohiro
<kosaki.motohiro@gmail.com> wrote:
> (5/10/12 8:50 PM), Minchan Kim wrote:
>>
>> Hi KOSAKI,
>>
>> On 05/11/2012 02:53 AM, KOSAKI Motohiro wrote:
>>
>>>>>> let's assume that one application want to allocate user space memory
>>>>>> region using malloc() and then write something on the region. as you
>>>>>> may know, user space buffer doen't have real physical pages once
>>>>>> malloc() call so if user tries to access the region then page fault
>>>>>> handler would be triggered
>>>>>
>>>>>
>>>>>
>>>>> Understood.
>>>>>
>>>>>> and then in turn next process like swap in to fill physical frame
>>>>>> number
>>>>>
>>>>> into entry of the page faulted.
>>>>>
>>>>>
>>>>> Sorry, I can't understand your point due to my poor English.
>>>>> Could you rewrite it easiliy? :)
>>>>>
>>>>
>>>> Simply saying, handle_mm_fault would be called to update pte after
>>>> finding
>>>> vma and checking access right. and as you know, there are many cases to
>>>> process page fault such as COW or demand paging.
>>>
>>>
>>> Hmm. If I understand correctly, you guys misunderstand mlock. it doesn't
>>> page pinning
>>> nor prevent pfn change. It only guarantee to don't make swap out. e.g.
>>
>>
>>
>> Symantic point of view, you're right but the implementation makes sure
>> page pinning.
>>
>>> memory campaction
>>> feature may automatically change page physical address.
>>
>>
>>
>> I tried it last year but decided drop by realtime issue.
>> https://lkml.org/lkml/2011/8/29/295
>>
>> so I think mlock is a kind of page pinning. If elsewhere I don't realized
>> is doing, that place should be fixed.
>> Or my above patch should go ahead.
>
>
> Thanks pointing out. I didn't realized your patch didn't merged. I think it
> should go ahead. think autonuma case,
> if mlock disable autonuma migration, that's bug.  I don't think we can
> promise mlock don't change physical page.
> I wonder if any realtime guys page migration is free lunch. they should
> disable both auto migration and compaction.
>
> And, think if application explictly use migrate_pages(2) or admins uses
> cpusets. driver code can't assume such scenario
> doesn't occur, yes?
>
>

I am ok with patch being merge as is if you add restriction for the
ioctl to be root only and a big comment stating that user ptr thing is
just abusing the kernel API and that it should not be replicated by
other driver except if fully understanding that all hell might break
loose with it.

If you know it's only the ddx that will use it and that their wont be
fork that better to not worry about but again state it in the comment
about the ioctl.

I really wish there was some magical VM_DRIVER_MAPPED flags that would
add the proper restriction to other memory code while keeping fork
behavior consistant (ie cow). But such things would need massive
chirurgy of the linux mm code.

Cheers,
Jerome

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[KOSAKI Motohiro]

(5/10/12 11:01 PM), Jerome Glisse wrote:
> On Thu, May 10, 2012 at 10:51 PM, KOSAKI Motohiro
> <kosaki.motohiro@gmail.com>  wrote:
>> (5/10/12 8:50 PM), Minchan Kim wrote:
>>>
>>> Hi KOSAKI,
>>>
>>> On 05/11/2012 02:53 AM, KOSAKI Motohiro wrote:
>>>
>>>>>>> let's assume that one application want to allocate user space memory
>>>>>>> region using malloc() and then write something on the region. as you
>>>>>>> may know, user space buffer doen't have real physical pages once
>>>>>>> malloc() call so if user tries to access the region then page fault
>>>>>>> handler would be triggered
>>>>>>
>>>>>>
>>>>>>
>>>>>> Understood.
>>>>>>
>>>>>>> and then in turn next process like swap in to fill physical frame
>>>>>>> number
>>>>>>
>>>>>> into entry of the page faulted.
>>>>>>
>>>>>>
>>>>>> Sorry, I can't understand your point due to my poor English.
>>>>>> Could you rewrite it easiliy? :)
>>>>>>
>>>>>
>>>>> Simply saying, handle_mm_fault would be called to update pte after
>>>>> finding
>>>>> vma and checking access right. and as you know, there are many cases to
>>>>> process page fault such as COW or demand paging.
>>>>
>>>>
>>>> Hmm. If I understand correctly, you guys misunderstand mlock. it doesn't
>>>> page pinning
>>>> nor prevent pfn change. It only guarantee to don't make swap out. e.g.
>>>
>>>
>>>
>>> Symantic point of view, you're right but the implementation makes sure
>>> page pinning.
>>>
>>>> memory campaction
>>>> feature may automatically change page physical address.
>>>
>>>
>>>
>>> I tried it last year but decided drop by realtime issue.
>>> https://lkml.org/lkml/2011/8/29/295
>>>
>>> so I think mlock is a kind of page pinning. If elsewhere I don't realized
>>> is doing, that place should be fixed.
>>> Or my above patch should go ahead.
>>
>>
>> Thanks pointing out. I didn't realized your patch didn't merged. I think it
>> should go ahead. think autonuma case,
>> if mlock disable autonuma migration, that's bug.  I don't think we can
>> promise mlock don't change physical page.
>> I wonder if any realtime guys page migration is free lunch. they should
>> disable both auto migration and compaction.
>>
>> And, think if application explictly use migrate_pages(2) or admins uses
>> cpusets. driver code can't assume such scenario
>> doesn't occur, yes?
>>
>>
>
> I am ok with patch being merge as is if you add restriction for the
> ioctl to be root only and a big comment stating that user ptr thing is
> just abusing the kernel API and that it should not be replicated by
> other driver except if fully understanding that all hell might break
> loose with it.

Oh, apology. I didn't intend to assist as is merge. Basically I agree with
minchan. Is should be replaced get_user_pages(). I only intended to clarify
pros/cons and where is original author's intention. If I understand correctly,
MADV_DONT_FORK is best solution for this case.




> If you know it's only the ddx that will use it and that their wont be
> fork that better to not worry about but again state it in the comment
> about the ioctl.
>
> I really wish there was some magical VM_DRIVER_MAPPED flags that would
> add the proper restriction to other memory code while keeping fork
> behavior consistant (ie cow). But such things would need massive
> chirurgy of the linux mm code.
>
> Cheers,
> Jerome

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Jerome Glisse]

On Fri, May 11, 2012 at 5:20 PM, KOSAKI Motohiro
<kosaki.motohiro@gmail.com> wrote:
> (5/10/12 11:01 PM), Jerome Glisse wrote:
>>
>> On Thu, May 10, 2012 at 10:51 PM, KOSAKI Motohiro
>> <kosaki.motohiro@gmail.com>  wrote:
>>>
>>> (5/10/12 8:50 PM), Minchan Kim wrote:
>>>>
>>>>
>>>> Hi KOSAKI,
>>>>
>>>> On 05/11/2012 02:53 AM, KOSAKI Motohiro wrote:
>>>>
>>>>>>>> let's assume that one application want to allocate user space memory
>>>>>>>> region using malloc() and then write something on the region. as you
>>>>>>>> may know, user space buffer doen't have real physical pages once
>>>>>>>> malloc() call so if user tries to access the region then page fault
>>>>>>>> handler would be triggered
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Understood.
>>>>>>>
>>>>>>>> and then in turn next process like swap in to fill physical frame
>>>>>>>> number
>>>>>>>
>>>>>>>
>>>>>>> into entry of the page faulted.
>>>>>>>
>>>>>>>
>>>>>>> Sorry, I can't understand your point due to my poor English.
>>>>>>> Could you rewrite it easiliy? :)
>>>>>>>
>>>>>>
>>>>>> Simply saying, handle_mm_fault would be called to update pte after
>>>>>> finding
>>>>>> vma and checking access right. and as you know, there are many cases
>>>>>> to
>>>>>> process page fault such as COW or demand paging.
>>>>>
>>>>>
>>>>>
>>>>> Hmm. If I understand correctly, you guys misunderstand mlock. it
>>>>> doesn't
>>>>> page pinning
>>>>> nor prevent pfn change. It only guarantee to don't make swap out. e.g.
>>>>
>>>>
>>>>
>>>>
>>>> Symantic point of view, you're right but the implementation makes sure
>>>> page pinning.
>>>>
>>>>> memory campaction
>>>>> feature may automatically change page physical address.
>>>>
>>>>
>>>>
>>>>
>>>> I tried it last year but decided drop by realtime issue.
>>>> https://lkml.org/lkml/2011/8/29/295
>>>>
>>>> so I think mlock is a kind of page pinning. If elsewhere I don't
>>>> realized
>>>> is doing, that place should be fixed.
>>>> Or my above patch should go ahead.
>>>
>>>
>>>
>>> Thanks pointing out. I didn't realized your patch didn't merged. I think
>>> it
>>> should go ahead. think autonuma case,
>>> if mlock disable autonuma migration, that's bug.  I don't think we can
>>> promise mlock don't change physical page.
>>> I wonder if any realtime guys page migration is free lunch. they should
>>> disable both auto migration and compaction.
>>>
>>> And, think if application explictly use migrate_pages(2) or admins uses
>>> cpusets. driver code can't assume such scenario
>>> doesn't occur, yes?
>>>
>>>
>>
>> I am ok with patch being merge as is if you add restriction for the
>> ioctl to be root only and a big comment stating that user ptr thing is
>> just abusing the kernel API and that it should not be replicated by
>> other driver except if fully understanding that all hell might break
>> loose with it.
>
>
> Oh, apology. I didn't intend to assist as is merge. Basically I agree with
> minchan. Is should be replaced get_user_pages(). I only intended to clarify
> pros/cons and where is original author's intention. If I understand
> correctly,
> MADV_DONT_FORK is best solution for this case.
>

My point is this ioctl will be restricted to one user (Xserver if i
understand) and only this user, there is no fork in it so no need to
worry about fork, just setting the vma as locked will be enough.

But i don't want people reading this driver suddenly think that what
it's doing is ok, it's not, it's hack and can never make to work
properly on a general case, that's why it needs a big comment stating,
stressing that. I just wanted to make sure Inki and Kyungmin
understood that this kind of ioctl should be restricted to carefully
selected user and that there is no way to make it general or reliable
outside that.

Cheers,
Jerome

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[KOSAKI Motohiro]

> My point is this ioctl will be restricted to one user (Xserver if i
> understand) and only this user, there is no fork in it so no need to
> worry about fork, just setting the vma as locked will be enough.
>
> But i don't want people reading this driver suddenly think that what
> it's doing is ok, it's not, it's hack and can never make to work
> properly on a general case, that's why it needs a big comment stating,
> stressing that. I just wanted to make sure Inki and Kyungmin
> understood that this kind of ioctl should be restricted to carefully
> selected user and that there is no way to make it general or reliable
> outside that.

first off, I'm not drm guy and then I don't intend to insist you. but if
application don't use fork, get_user_pages() has no downside. I guess we
don't need VM_LOCKED hack.

but again, up to drm folks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Jerome Glisse]

On Fri, May 11, 2012 at 6:59 PM, KOSAKI Motohiro
<kosaki.motohiro@gmail.com> wrote:
>> My point is this ioctl will be restricted to one user (Xserver if i
>> understand) and only this user, there is no fork in it so no need to
>> worry about fork, just setting the vma as locked will be enough.
>>
>> But i don't want people reading this driver suddenly think that what
>> it's doing is ok, it's not, it's hack and can never make to work
>> properly on a general case, that's why it needs a big comment stating,
>> stressing that. I just wanted to make sure Inki and Kyungmin
>> understood that this kind of ioctl should be restricted to carefully
>> selected user and that there is no way to make it general or reliable
>> outside that.
>
>
> first off, I'm not drm guy and then I don't intend to insist you. but if
> application don't use fork, get_user_pages() has no downside. I guess we
> don't need VM_LOCKED hack.
>
> but again, up to drm folks.

You need the VM_LOCKED hack to mare sure that the xorg vma still point
to the same page, afaict with get_user_pages pages can be migrated out
of the anonymous vma so the vma might point to new page, while old
page are still in use by the gpu and not recycle until their refcount
drop to 0.

Cheers,
Jerome

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[KOSAKI Motohiro]

On Fri, May 11, 2012 at 7:29 PM, Jerome Glisse <j.glisse@gmail.com> wrote:
> On Fri, May 11, 2012 at 6:59 PM, KOSAKI Motohiro
> <kosaki.motohiro@gmail.com> wrote:
>>> My point is this ioctl will be restricted to one user (Xserver if i
>>> understand) and only this user, there is no fork in it so no need to
>>> worry about fork, just setting the vma as locked will be enough.
>>>
>>> But i don't want people reading this driver suddenly think that what
>>> it's doing is ok, it's not, it's hack and can never make to work
>>> properly on a general case, that's why it needs a big comment stating,
>>> stressing that. I just wanted to make sure Inki and Kyungmin
>>> understood that this kind of ioctl should be restricted to carefully
>>> selected user and that there is no way to make it general or reliable
>>> outside that.
>>
>>
>> first off, I'm not drm guy and then I don't intend to insist you. but if
>> application don't use fork, get_user_pages() has no downside. I guess we
>> don't need VM_LOCKED hack.
>>
>> but again, up to drm folks.
>
> You need the VM_LOCKED hack to mare sure that the xorg vma still point
> to the same page, afaict with get_user_pages pages can be migrated out
> of the anonymous vma so the vma might point to new page, while old
> page are still in use by the gpu and not recycle until their refcount
> drop to 0.

afaik, get_user_pages() prevent page migration. (see
migrate_page_move_mapping). but mlock doesn't.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[InKi Dae]

2012/5/12 KOSAKI Motohiro <kosaki.motohiro@gmail.com>:
> On Fri, May 11, 2012 at 7:29 PM, Jerome Glisse <j.glisse@gmail.com> wrote:
>> On Fri, May 11, 2012 at 6:59 PM, KOSAKI Motohiro
>> <kosaki.motohiro@gmail.com> wrote:
>>>> My point is this ioctl will be restricted to one user (Xserver if i
>>>> understand) and only this user, there is no fork in it so no need to
>>>> worry about fork, just setting the vma as locked will be enough.
>>>>
>>>> But i don't want people reading this driver suddenly think that what
>>>> it's doing is ok, it's not, it's hack and can never make to work
>>>> properly on a general case, that's why it needs a big comment stating,
>>>> stressing that. I just wanted to make sure Inki and Kyungmin
>>>> understood that this kind of ioctl should be restricted to carefully
>>>> selected user and that there is no way to make it general or reliable
>>>> outside that.
>>>
>>>
>>> first off, I'm not drm guy and then I don't intend to insist you. but if
>>> application don't use fork, get_user_pages() has no downside. I guess we
>>> don't need VM_LOCKED hack.
>>>
>>> but again, up to drm folks.
>>
>> You need the VM_LOCKED hack to mare sure that the xorg vma still point
>> to the same page, afaict with get_user_pages pages can be migrated out
>> of the anonymous vma so the vma might point to new page, while old
>> page are still in use by the gpu and not recycle until their refcount
>> drop to 0.
>
> afaik, get_user_pages() prevent page migration. (see
> migrate_page_move_mapping). but mlock doesn't.


I'd like to make sure some points before preparing next patch to
userptr feature.

in case that userptr ioctl can be accessed by user.
1. all the pages from get_user_pages can't be migrated by CMA and also
it doesn't need VM_LOCKED or VM_RESERVED flag.

2. if VM_DONTCOPY is set to vma->flags then all the pages to this vma
are safe from being COW.

3. userptr ioctl  has limited size and the limited size can be changed
by only root user. this is for preventing from dropping system
performance by malicious software.

with above actions taken, are there something we didn't care? if so,
we will preparing next path for the userptr ioctl to be accessed by
only root user. this means that this feature is used by only X Server
but isn't used by any users. so we are going to wait something
resolved fully. of course, as Jerome said, we wil add big comments
describing this feature enough to next patch.

Thanks,
Inki Dae

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v3] drm/exynos: added userptr feature.

[Minchan Kim]

On 05/12/2012 01:48 PM, InKi Dae wrote:

> 2012/5/12 KOSAKI Motohiro <kosaki.motohiro@gmail.com>:
>> On Fri, May 11, 2012 at 7:29 PM, Jerome Glisse <j.glisse@gmail.com> wrote:
>>> On Fri, May 11, 2012 at 6:59 PM, KOSAKI Motohiro
>>> <kosaki.motohiro@gmail.com> wrote:
>>>>> My point is this ioctl will be restricted to one user (Xserver if i
>>>>> understand) and only this user, there is no fork in it so no need to
>>>>> worry about fork, just setting the vma as locked will be enough.
>>>>>
>>>>> But i don't want people reading this driver suddenly think that what
>>>>> it's doing is ok, it's not, it's hack and can never make to work
>>>>> properly on a general case, that's why it needs a big comment stating,
>>>>> stressing that. I just wanted to make sure Inki and Kyungmin
>>>>> understood that this kind of ioctl should be restricted to carefully
>>>>> selected user and that there is no way to make it general or reliable
>>>>> outside that.
>>>>
>>>>
>>>> first off, I'm not drm guy and then I don't intend to insist you. but if
>>>> application don't use fork, get_user_pages() has no downside. I guess we
>>>> don't need VM_LOCKED hack.
>>>>
>>>> but again, up to drm folks.
>>>
>>> You need the VM_LOCKED hack to mare sure that the xorg vma still point
>>> to the same page, afaict with get_user_pages pages can be migrated out
>>> of the anonymous vma so the vma might point to new page, while old
>>> page are still in use by the gpu and not recycle until their refcount
>>> drop to 0.
>>
>> afaik, get_user_pages() prevent page migration. (see
>> migrate_page_move_mapping). but mlock doesn't.
> 
> 
> I'd like to make sure some points before preparing next patch to
> userptr feature.
> 
> in case that userptr ioctl can be accessed by user.
> 1. all the pages from get_user_pages can't be migrated by CMA and also


Yes. I already mentioned it.

> it doesn't need VM_LOCKED or VM_RESERVED flag.


Yes, if you just use that flag to prevent migration.

> 
> 2. if VM_DONTCOPY is set to vma->flags then all the pages to this vma
> are safe from being COW.


Yes.

> 
> 3. userptr ioctl  has limited size and the limited size can be changed
> by only root user. this is for preventing from dropping system
> performance by malicious software.


IMHO, looks good to me but need answer from DRM guy on the question.

> 
> with above actions taken, are there something we didn't care? if so,
> we will preparing next path for the userptr ioctl to be accessed by
> only root user. this means that this feature is used by only X Server
> but isn't used by any users. so we are going to wait something
> resolved fully. of course, as Jerome said, we wil add big comments
> describing this feature enough to next patch.
> 
> Thanks,
> Inki Dae
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 



-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH 0/2 v4] drm/exynos: added userptr feature

[Inki Dae]

ccing linux-mm

> -----Original Message-----
> From: Inki Dae [mailto:inki.dae@samsung.com]
> Sent: Monday, May 14, 2012 3:18 PM
> To: airlied@linux.ie; dri-devel@lists.freedesktop.org
> Cc: j.glisse@gmail.com; minchan@kernel.org; kosaki.motohiro@gmail.com;
> kyungmin.park@samsung.com; sw0312.kim@samsung.com;
jy0922.shim@samsung.com;
> Inki Dae
> Subject: [PATCH 0/2 v4] drm/exynos: added userptr feature
> 
> this feature could be used to memory region allocated by malloc() in user
> mode
> and mmaped memory region allocated by other memory allocators.
> userptr interface can identify memory type through vm_flags value and
> would get
> pages or page frame numbers to user space appropriately.
> 
> Changelog v4:
> we have discussed some issues to userptr feature with drm and mm guys and
> below are the issues.
> 
> The migration issue.
> - Pages reserved by CMA for some device using DMA could be used by
> kernel and if the device driver wants to use those pages
> while being used by kernel then the pages are copied into
> other ones allocated to migrate them and then finally,
> the device driver can use the pages for itself.
> Thus, migrated, the pages being accessed by DMA could be changed
> to other so this situation may incur that DMA accesses any pages
> it doesn't want.
> 
> The COW issue.
> - while DMA of a device is using the pages to VMAs, if current
> process was forked then the pages being accessed by the DMA
> would be copied into child's pages.(Copy On Write) so
> these pages may not have coherrency with parent's ones if
> child process wrote something on those pages so we need to
> flag VM_DONTCOPY to prevent pages from being COWed.
> 
> But the use of get_user_pages is safe from such magration issue
> because all the pages from get_user_pages CAN NOT BE not only
> migrated but also swapped out. this true has been confirmed
> by mm guys, Minchan Kim and KOSAKI Motohiro.
> However below issue could be incurred.
> 
> The deterioration issue of system performance by malicious process.
> - any malicious process can request all the pages of entire system memory
> through this userptr ioctl and as the result, all other processes would be
> blocked and this would incur the deterioration of system performance
> because the pages couldn't be swapped out.
> 
> So we limit user-desired userptr size to pre-defined and this ioctl
> command
> CAN BE accessed by only root user.
> 
> Changelog v3:
> Mitigated the issues pointed out by Dave and Jerome.
> 
> for this, added some codes to guarantee the pages to user space not
> to be swapped out, the VMAs within the user space would be locked and
> then unlocked when the pages are released.
> 
> but this lock might result in significant degradation of system
> performance
> because the pages couldn't be swapped out so added one more featrue
> that we can limit user-desired userptr size to pre-defined value using
> userptr limit ioctl that can be accessed by only root user.
> 
> Changelog v2:
> the memory regino mmaped with VM_PFNMAP type is physically continuous and
> start address of the memory region should be set into buf->dma_addr but
> previous patch had a problem that end address is set into buf->dma_addr
> so v2 fixes that problem.
> 
> Inki Dae (2):
>   drm/exynos: added userptr limit ioctl.
>   drm/exynos: added userptr feature.
> 
>  drivers/gpu/drm/exynos/exynos_drm_drv.c |    8 +
>  drivers/gpu/drm/exynos/exynos_drm_drv.h |    6 +
>  drivers/gpu/drm/exynos/exynos_drm_gem.c |  413
> +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/exynos/exynos_drm_gem.h |   20 ++-
>  include/drm/exynos_drm.h                |   43 +++-
>  5 files changed, 487 insertions(+), 3 deletions(-)
> 
> --
> 1.7.4.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH 1/2 v4] drm/exynos: added userptr limit ioctl.

[Inki Dae]

ccing linux-mm

> -----Original Message-----
> From: Inki Dae [mailto:inki.dae@samsung.com]
> Sent: Monday, May 14, 2012 3:18 PM
> To: airlied@linux.ie; dri-devel@lists.freedesktop.org
> Cc: j.glisse@gmail.com; minchan@kernel.org; kosaki.motohiro@gmail.com;
> kyungmin.park@samsung.com; sw0312.kim@samsung.com;
jy0922.shim@samsung.com;
> Inki Dae
> Subject: [PATCH 1/2 v4] drm/exynos: added userptr limit ioctl.
> 
> this ioctl is used to limit user-desired userptr size as pre-defined
> and also could be accessed by only root user.
> 
> with userptr feature, unprivileged user can allocate all the pages on
> system,
> so the amount of free physical pages will be very limited. if the VMAs
> within user address space was pinned, the pages couldn't be swapped out so
> it may result in significant degradation of system performance. so this
> feature would be used to avoid such situation.
> 
> Signed-off-by: Inki Dae <inki.dae@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.c |    6 ++++++
>  drivers/gpu/drm/exynos/exynos_drm_drv.h |    6 ++++++
>  drivers/gpu/drm/exynos/exynos_drm_gem.c |   22 ++++++++++++++++++++++
>  drivers/gpu/drm/exynos/exynos_drm_gem.h |    3 +++
>  include/drm/exynos_drm.h                |   17 +++++++++++++++++
>  5 files changed, 54 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 9d3204c..1e68ec2 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -64,6 +64,9 @@ static int exynos_drm_load(struct drm_device *dev,
> unsigned long flags)
>  		return -ENOMEM;
>  	}
> 
> +	/* maximum size of userptr is limited to 16MB as default. */
> +	private->userptr_limit = SZ_16M;
> +
>  	INIT_LIST_HEAD(&private->pageflip_event_list);
>  	dev->dev_private = (void *)private;
> 
> @@ -221,6 +224,9 @@ static struct drm_ioctl_desc exynos_ioctls[] = {
>  			exynos_drm_gem_mmap_ioctl, DRM_UNLOCKED | DRM_AUTH),
>  	DRM_IOCTL_DEF_DRV(EXYNOS_GEM_GET,
>  			exynos_drm_gem_get_ioctl, DRM_UNLOCKED),
> +	DRM_IOCTL_DEF_DRV(EXYNOS_USER_LIMIT,
> +			exynos_drm_gem_user_limit_ioctl, DRM_MASTER |
> +			DRM_ROOT_ONLY),
>  	DRM_IOCTL_DEF_DRV(EXYNOS_PLANE_SET_ZPOS,
> exynos_plane_set_zpos_ioctl,
>  			DRM_UNLOCKED | DRM_AUTH),
>  	DRM_IOCTL_DEF_DRV(EXYNOS_VIDI_CONNECTION,
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index c82c90c..b38ed6f 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -235,6 +235,12 @@ struct exynos_drm_private {
>  	 * this array is used to be aware of which crtc did it request
> vblank.
>  	 */
>  	struct drm_crtc *crtc[MAX_CRTC];
> +
> +	/*
> +	 * maximum size of allocation by userptr feature.
> +	 * - as default, this has 16MB and only root user can change it.
> +	 */
> +	unsigned long userptr_limit;
>  };
> 
>  /*
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> index fc91293..e6abb66 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> @@ -33,6 +33,8 @@
>  #include "exynos_drm_gem.h"
>  #include "exynos_drm_buf.h"
> 
> +#define USERPTR_MAX_SIZE		SZ_64M
> +
>  static unsigned int convert_to_vm_err_msg(int msg)
>  {
>  	unsigned int out_msg;
> @@ -630,6 +632,26 @@ int exynos_drm_gem_get_ioctl(struct drm_device *dev,
> void *data,
>  	return 0;
>  }
> 
> +int exynos_drm_gem_user_limit_ioctl(struct drm_device *dev, void *data,
> +				      struct drm_file *filp)
> +{
> +	struct exynos_drm_private *priv = dev->dev_private;
> +	struct drm_exynos_user_limit *limit = data;
> +
> +	if (limit->userptr_limit < PAGE_SIZE ||
> +			limit->userptr_limit > USERPTR_MAX_SIZE) {
> +		DRM_DEBUG_KMS("invalid userptr_limit size.\n");
> +		return -EINVAL;
> +	}
> +
> +	if (priv->userptr_limit == limit->userptr_limit)
> +		return 0;
> +
> +	priv->userptr_limit = limit->userptr_limit;
> +
> +	return 0;
> +}
> +
>  int exynos_drm_gem_init_object(struct drm_gem_object *obj)
>  {
>  	DRM_DEBUG_KMS("%s\n", __FILE__);
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> index 14d038b..3334c9f 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> @@ -78,6 +78,9 @@ struct exynos_drm_gem_obj {
> 
>  struct page **exynos_gem_get_pages(struct drm_gem_object *obj, gfp_t
> gfpmask);
> 
> +int exynos_drm_gem_user_limit_ioctl(struct drm_device *dev, void *data,
> +				      struct drm_file *filp);
> +
>  /* destroy a buffer with gem object */
>  void exynos_drm_gem_destroy(struct exynos_drm_gem_obj *exynos_gem_obj);
> 
> diff --git a/include/drm/exynos_drm.h b/include/drm/exynos_drm.h
> index 54c97e8..52465dc 100644
> --- a/include/drm/exynos_drm.h
> +++ b/include/drm/exynos_drm.h
> @@ -92,6 +92,19 @@ struct drm_exynos_gem_info {
>  };
> 
>  /**
> + * A structure to userptr limited information.
> + *
> + * @userptr_limit: maximum size to userptr buffer.
> + *	the buffer could be allocated by unprivileged user using malloc()
> + *	and the size of the buffer would be limited as userptr_limit value.
> + * @pad: just padding to be 64-bit aligned.
> + */
> +struct drm_exynos_user_limit {
> +	unsigned int userptr_limit;
> +	unsigned int pad;
> +};
> +
> +/**
>   * A structure for user connection request of virtual display.
>   *
>   * @connection: indicate whether doing connetion or not by user.
> @@ -162,6 +175,7 @@ struct drm_exynos_g2d_exec {
>  #define DRM_EXYNOS_GEM_MMAP		0x02
>  /* Reserved 0x03 ~ 0x05 for exynos specific gem ioctl */
>  #define DRM_EXYNOS_GEM_GET		0x04
> +#define DRM_EXYNOS_USER_LIMIT		0x05
>  #define DRM_EXYNOS_PLANE_SET_ZPOS	0x06
>  #define DRM_EXYNOS_VIDI_CONNECTION	0x07
> 
> @@ -182,6 +196,9 @@ struct drm_exynos_g2d_exec {
>  #define DRM_IOCTL_EXYNOS_GEM_GET	DRM_IOWR(DRM_COMMAND_BASE + \
>  		DRM_EXYNOS_GEM_GET,	struct drm_exynos_gem_info)
> 
> +#define DRM_IOCTL_EXYNOS_USER_LIMIT	DRM_IOWR(DRM_COMMAND_BASE + \
> +		DRM_EXYNOS_USER_LIMIT,	struct drm_exynos_user_limit)
> +
>  #define DRM_IOCTL_EXYNOS_PLANE_SET_ZPOS	DRM_IOWR(DRM_COMMAND_BASE +
> \
>  		DRM_EXYNOS_PLANE_SET_ZPOS, struct drm_exynos_plane_set_zpos)
> 
> --
> 1.7.4.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH 2/2 v4] drm/exynos: added userptr feature.

[Inki Dae]

ccing linux-mm

> -----Original Message-----
> From: KOSAKI Motohiro [mailto:kosaki.motohiro@gmail.com]
> Sent: Monday, May 14, 2012 4:05 PM
> To: Inki Dae
> Cc: 'KOSAKI Motohiro'; airlied@linux.ie; dri-devel@lists.freedesktop.org;
> j.glisse@gmail.com; minchan@kernel.org; kyungmin.park@samsung.com;
> sw0312.kim@samsung.com; jy0922.shim@samsung.com
> Subject: Re: [PATCH 2/2 v4] drm/exynos: added userptr feature.
> 
> (5/14/12 2:52 AM), Inki Dae wrote:
> >
> >
> >> -----Original Message-----
> >> From: KOSAKI Motohiro [mailto:kosaki.motohiro@gmail.com]
> >> Sent: Monday, May 14, 2012 3:33 PM
> >> To: Inki Dae
> >> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
> j.glisse@gmail.com;
> >> minchan@kernel.org; kyungmin.park@samsung.com; sw0312.kim@samsung.com;
> >> jy0922.shim@samsung.com
> >> Subject: Re: [PATCH 2/2 v4] drm/exynos: added userptr feature.
> >>
> >>> +       npages = buf->size>>  PAGE_SHIFT;
> >>
> >> Why round down? usually we use round up.
> >>
> >
> > The size was already rounded up by exynos_drm_gem_userptr_ioctl so this
> is
> > just used to get page count.
> 
> got it.
> 
> 
> 
> >>> +       down_read(&current->mm->mmap_sem);
> >>> +
> >>> +       /*
> >>> +        * Basically, all the pages from get_user_pages() can not be
not
> >> only
> >>> +        * migrated by CMA but also swapped out.
> >>> +        *
> >>> +        * The migration issue.
> >>> +        * - Pages reserved by CMA for some device using DMA could be
> >> used by
> >>> +        * kernel and if the device driver wants to use those pages
> >>> +        * while being used by kernel then the pages are copied into
> >>> +        * other ones allocated to migrate them and then finally,
> >>> +        * the device driver can use the pages for itself.
> >>> +        * Thus, migrated, the pages being accessed by DMA could be
> >> changed
> >>> +        * to other so this situation may incur that DMA accesses any
> >> pages
> >>> +        * it doesn't want.
> >>> +        *
> >>> +        * But the use of get_user_pages is safe from such magration
> >> issue
> >>> +        * because all the pages from get_user_pages CAN NOT be not
only
> >>> +        * migrated, but also swapped out.
> >>> +        */
> >>> +       get_npages = get_user_pages(current, current->mm, userptr,
> >>> +                                       npages, write, 1, buf->pages,
> > NULL);
> >>
> >> Why force=1? It is almostly core-dump specific option. Why don't you
> >> return
> >
> > I know that force indicates whether to force write access even  if user
> > mapping is readonly.
> 
> right. and then, usually we don't want to ignore access permission. but
> note,
> I'm only talk about generic thing. I have no knowledge drm area.
> 
> 
> 
> > so we just want to use pages from get_user_pages as
> > read/write permission.
> 
> >> EFAULT when the page has write permission. IOW, Why your Xorg module
> >> don't map memory w/ PROT_WRITE?
> >
> > No, Xorg can map memory w/ PROT_WRITE. Couldn't the Xorg map w/
> PROT_WRITE
> > if force = 1? plz, let me know if there is my missing point.
> 
> I meant, if Xorg always use PROT_WRITE, you don't need force=1.
> 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH 2/2 v4] drm/exynos: added userptr feature.

[Inki Dae]

Hi Jerome,

> -----Original Message-----
> From: Jerome Glisse [mailto:j.glisse@gmail.com]
> Sent: Tuesday, May 15, 2012 4:27 AM
> To: Inki Dae
> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org; minchan@kernel.org;
> kosaki.motohiro@gmail.com; kyungmin.park@samsung.com;
> sw0312.kim@samsung.com; jy0922.shim@samsung.com
> Subject: Re: [PATCH 2/2 v4] drm/exynos: added userptr feature.
> 
> On Mon, May 14, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com> wrote:
> > this feature is used to import user space region allocated by malloc()
> or
> > mmaped into a gem. for this, we uses get_user_pages() to get all the
> pages
> > to VMAs within user address space. However we should pay attention to
> use
> > this userptr feature like below.
> >
> > The migration issue.
> > - Pages reserved by CMA for some device using DMA could be used by
> > kernel and if the device driver wants to use those pages
> > while being used by kernel then the pages are copied into
> > other ones allocated to migrate them and then finally,
> > the device driver can use the pages for itself.
> > Thus, migrated, the pages being accessed by DMA could be changed
> > to other so this situation may incur that DMA accesses any pages
> > it doesn't want.
> >
> > The COW issue.
> > - while DMA of a device is using the pages to VMAs, if current
> > process was forked then the pages being accessed by the DMA
> > would be copied into child's pages.(Copy On Write) so
> > these pages may not have coherrency with parent's ones if
> > child process wrote something on those pages so we need to
> > flag VM_DONTCOPY to prevent pages from being COWed.
> 
> Note that this is a massive change in behavior of anonymous mapping
> this effectively completely change the linux API from application
> point of view on your platform. Any application that have memory
> mapped by your ioctl will have different fork behavior that other
> application. I think this should be stressed, it's one of the thing i
> am really uncomfortable with i would rather not have the dont copy
> flag and have the page cowed and have the child not working with the
> 3d/2d/drm driver. That would means that your driver (opengl
> implementation for instance) would have to detect fork and work around
> it, nvidia closed source driver do that.
> 

First of all, thank you for your comments.

Right, VM_DONTCOPY flag would change original behavior of user. Do you think
this way has no problem but no generic way? anyway our issue was that the
pages to VMAs are copied into child's ones(COW) so we prevented those pages
from being COWed with using VM_DONTCOPY flag.

For this, I have three questions below

1. in case of not using VM_DONTCOPY flag, you think that the application
using our userptr feature has COW issue; parent's pages being accessed by
DMA of some device would be copied into child's ones if the child wrote
something on the pages. after that, DMA of a device could access pages user
doesn't want. I'm not sure but I think such behavior has no any problem and
is generic behavior and it's role of user to do fork or not. Do you think
such COW behavior could create any issue I don't aware of so we have to
prevent that somehow?

2. so we added VM_DONTCOPY flag to prevent the pages from being COWed but
this changes original behavior of user. Do you think this is not generic way
or could create any issue also?

3. and last one, what is the difference between to flag VM_DONTCOPY and to
detect fork? I mean the device driver should do something to need after
detecting fork. and I'm not sure but I think the something may also change
original behavior of user.

Please let me know if there is my missing point.

Thanks,
Inki Dae

> Anyway this is a big red flag for me, as app on your platform using
> your drm driver might start relaying on a completely non con-formant
> fork behavior.
> 
> > But the use of get_user_pages is safe from such magration issue
> > because all the pages from get_user_pages CAN NOT BE not only
> > migrated, but also swapped out. However below issue could be incurred.
> >
> > The deterioration issue of system performance by malicious process.
> > - any malicious process can request all the pages of entire system
> memory
> > through this userptr ioctl. which in turn, all other processes would be
> > blocked and incur the deterioration of system performance because the
> pages
> > couldn't be swapped out.
> >
> > So this feature limit user-desired userptr size to pre-defined and this
> value
> > CAN BE changed by only root user.
> >
> > Signed-off-by: Inki Dae <inki.dae@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_drv.c |    2 +
> >  drivers/gpu/drm/exynos/exynos_drm_gem.c |  391
> +++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/exynos/exynos_drm_gem.h |   17 ++-
> >  include/drm/exynos_drm.h                |   26 ++-
> >  4 files changed, 433 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > index 1e68ec2..e8ae3f1 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > @@ -220,6 +220,8 @@ static struct drm_ioctl_desc exynos_ioctls[] = {
> >        DRM_IOCTL_DEF_DRV(EXYNOS_GEM_MAP_OFFSET,
> >                        exynos_drm_gem_map_offset_ioctl, DRM_UNLOCKED |
> >                        DRM_AUTH),
> > +       DRM_IOCTL_DEF_DRV(EXYNOS_GEM_USERPTR,
> > +                       exynos_drm_gem_userptr_ioctl, DRM_UNLOCKED),
> >        DRM_IOCTL_DEF_DRV(EXYNOS_GEM_MMAP,
> >                        exynos_drm_gem_mmap_ioctl, DRM_UNLOCKED |
DRM_AUTH),
> >        DRM_IOCTL_DEF_DRV(EXYNOS_GEM_GET,
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > index e6abb66..3c8a5f3 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.c
> > @@ -68,6 +68,80 @@ static int check_gem_flags(unsigned int flags)
> >        return 0;
> >  }
> >
> > +static struct vm_area_struct *get_vma(struct vm_area_struct *vma)
> > +{
> > +       struct vm_area_struct *vma_copy;
> > +
> > +       vma_copy = kmalloc(sizeof(*vma_copy), GFP_KERNEL);
> > +       if (!vma_copy)
> > +               return NULL;
> > +
> > +       if (vma->vm_ops && vma->vm_ops->open)
> > +               vma->vm_ops->open(vma);
> > +
> > +       if (vma->vm_file)
> > +               get_file(vma->vm_file);
> > +
> > +       memcpy(vma_copy, vma, sizeof(*vma));
> > +
> > +       vma_copy->vm_mm = NULL;
> > +       vma_copy->vm_next = NULL;
> > +       vma_copy->vm_prev = NULL;
> > +
> > +       return vma_copy;
> > +}
> > +
> > +
> > +static void put_vma(struct vm_area_struct *vma)
> > +{
> > +       if (!vma)
> > +               return;
> > +
> > +       if (vma->vm_ops && vma->vm_ops->close)
> > +               vma->vm_ops->close(vma);
> > +
> > +       if (vma->vm_file)
> > +               fput(vma->vm_file);
> > +
> > +       kfree(vma);
> > +}
> > +
> > +/*
> > + * cow_userptr_vma - flag VM_DONTCOPY to VMAs to user address space.
> > + *
> > + * this function flags VM_DONTCOPY to VMAs to user address space to
> prevent
> > + * pages to VMAs from being COWed.
> > + */
> > +static int cow_userptr_vma(struct exynos_drm_gem_buf *buf, unsigned int
> no_cow)
> > +{
> > +       struct vm_area_struct *vma;
> > +       unsigned long start, end;
> > +
> > +       start = buf->userptr;
> > +       end = buf->userptr + buf->size - 1;
> > +
> > +       down_write(&current->mm->mmap_sem);
> > +
> > +       do {
> > +               vma = find_vma(current->mm, start);
> > +               if (!vma) {
> > +                       up_write(&current->mm->mmap_sem);
> > +                       return -EFAULT;
> > +               }
> > +
> > +               if (no_cow)
> > +                       vma->vm_flags |= VM_DONTCOPY;
> > +               else
> > +                       vma->vm_flags &= ~VM_DONTCOPY;
> > +
> > +               start = vma->vm_end + 1;
> > +       } while (vma->vm_end < end);
> > +
> > +       up_write(&current->mm->mmap_sem);
> > +
> > +       return 0;
> > +}
> > +
> >  static void update_vm_cache_attr(struct exynos_drm_gem_obj *obj,
> >                                        struct vm_area_struct *vma)
> >  {
> > @@ -256,6 +330,50 @@ static void exynos_drm_gem_put_pages(struct
> drm_gem_object *obj)
> >        /* add some codes for UNCACHED type here. TODO */
> >  }
> >
> > +static void exynos_drm_put_userptr(struct drm_gem_object *obj)
> > +{
> > +       struct drm_device *dev = obj->dev;
> > +       struct exynos_drm_private *priv = dev->dev_private;
> > +       struct exynos_drm_gem_obj *exynos_gem_obj;
> > +       struct exynos_drm_gem_buf *buf;
> > +       struct vm_area_struct *vma;
> > +       int npages;
> > +
> > +       exynos_gem_obj = to_exynos_gem_obj(obj);
> > +       buf = exynos_gem_obj->buffer;
> > +       vma = exynos_gem_obj->vma;
> > +
> > +       if (vma && (vma->vm_flags & VM_PFNMAP) && (vma->vm_pgoff)) {
> > +               put_vma(exynos_gem_obj->vma);
> > +               goto out;
> > +       }
> > +
> > +       npages = buf->size >> PAGE_SHIFT;
> > +
> > +       if (exynos_gem_obj->flags & EXYNOS_BO_USERPTR && !buf->pfnmap)
> > +               cow_userptr_vma(buf, 0);
> > +
> > +       npages--;
> > +       while (npages >= 0) {
> > +               if (buf->write)
> > +                       set_page_dirty_lock(buf->pages[npages]);
> > +
> > +               put_page(buf->pages[npages]);
> > +               npages--;
> > +       }
> > +
> > +out:
> > +       mutex_lock(&dev->struct_mutex);
> > +       priv->userptr_limit += buf->size;
> > +       mutex_unlock(&dev->struct_mutex);
> > +
> > +       kfree(buf->pages);
> > +       buf->pages = NULL;
> > +
> > +       kfree(buf->sgt);
> > +       buf->sgt = NULL;
> > +}
> > +
> >  static int exynos_drm_gem_handle_create(struct drm_gem_object *obj,
> >                                        struct drm_file *file_priv,
> >                                        unsigned int *handle)
> > @@ -295,6 +413,8 @@ void exynos_drm_gem_destroy(struct
> exynos_drm_gem_obj *exynos_gem_obj)
> >
> >        if (exynos_gem_obj->flags & EXYNOS_BO_NONCONTIG)
> >                exynos_drm_gem_put_pages(obj);
> > +       else if (exynos_gem_obj->flags & EXYNOS_BO_USERPTR)
> > +               exynos_drm_put_userptr(obj);
> >        else
> >                exynos_drm_free_buf(obj->dev, exynos_gem_obj->flags,
buf);
> >
> > @@ -606,6 +726,277 @@ int exynos_drm_gem_mmap_ioctl(struct drm_device
> *dev, void *data,
> >        return 0;
> >  }
> >
> > +/*
> > + * exynos_drm_get_userptr - get pages to VMSs to user address space.
> > + *
> > + * this function is used for gpu driver to access user space directly
> > + * for performance enhancement to avoid memcpy.
> > + */
> > +static int exynos_drm_get_userptr(struct drm_device *dev,
> > +                               struct exynos_drm_gem_obj *obj,
> > +                               unsigned long userptr,
> > +                               unsigned int write)
> > +{
> > +       unsigned int get_npages;
> > +       unsigned long npages = 0;
> > +       struct vm_area_struct *vma;
> > +       struct exynos_drm_gem_buf *buf = obj->buffer;
> > +       int ret;
> > +
> > +       down_read(&current->mm->mmap_sem);
> > +       vma = find_vma(current->mm, userptr);
> > +
> > +       /* the memory region mmaped with VM_PFNMAP. */
> > +       if (vma && (vma->vm_flags & VM_PFNMAP) && (vma->vm_pgoff)) {
> > +               unsigned long this_pfn, prev_pfn, pa;
> > +               unsigned long start, end, offset;
> > +               struct scatterlist *sgl;
> > +               int ret;
> > +
> > +               start = userptr;
> > +               offset = userptr & ~PAGE_MASK;
> > +               end = start + buf->size;
> > +               sgl = buf->sgt->sgl;
> > +
> > +               for (prev_pfn = 0; start < end; start += PAGE_SIZE) {
> > +                       ret = follow_pfn(vma, start, &this_pfn);
> > +                       if (ret)
> > +                               goto err;
> > +
> > +                       if (prev_pfn == 0) {
> > +                               pa = this_pfn << PAGE_SHIFT;
> > +                               buf->dma_addr = pa + offset;
> > +                       } else if (this_pfn != prev_pfn + 1) {
> > +                               ret = -EINVAL;
> > +                               goto err;
> > +                       }
> > +
> > +                       sg_dma_address(sgl) = (pa + offset);
> > +                       sg_dma_len(sgl) = PAGE_SIZE;
> > +                       prev_pfn = this_pfn;
> > +                       pa += PAGE_SIZE;
> > +                       npages++;
> > +                       sgl = sg_next(sgl);
> > +               }
> > +
> > +               obj->vma = get_vma(vma);
> > +               if (!obj->vma) {
> > +                       ret = -ENOMEM;
> > +                       goto err;
> > +               }
> > +
> > +               up_read(&current->mm->mmap_sem);
> > +               buf->pfnmap = true;
> > +
> > +               return npages;
> > +err:
> > +               buf->dma_addr = 0;
> > +               up_read(&current->mm->mmap_sem);
> > +
> > +               return ret;
> > +       }
> > +
> > +       up_read(&current->mm->mmap_sem);
> > +
> > +       /*
> > +        * flag VM_DONTCOPY to VMAs to user address space to prevent
> > +        * pages to VMAs from being COWed.
> > +        *
> > +        * The COW issue.
> > +        * - while DMA of a device is using the pages to VMAs, if
current
> > +        * process was forked then the pages being accessed by the DMA
> > +        * would be copied into child's pages.(Copy On Write) so
> > +        * these pages may not have coherrency with parent's ones if
> > +        * child process wrote something on those pages so we need to
> > +        * flag VM_DONTCOPY to prevent pages from being COWed.
> > +        */
> > +       ret = cow_userptr_vma(buf, 1);
> > +       if (ret < 0) {
> > +               DRM_ERROR("failed to set VM_DONTCOPY to  vma.\n");
> > +               cow_userptr_vma(buf, 0);
> > +               return 0;
> > +       }
> > +
> > +       buf->write = write;
> > +       npages = buf->size >> PAGE_SHIFT;
> > +
> > +       down_read(&current->mm->mmap_sem);
> > +
> > +       /*
> > +        * Basically, all the pages from get_user_pages() can not be not
> only
> > +        * migrated by CMA but also swapped out.
> > +        *
> > +        * The migration issue.
> > +        * - Pages reserved by CMA for some device using DMA could be
> used by
> > +        * kernel and if the device driver wants to use those pages
> > +        * while being used by kernel then the pages are copied into
> > +        * other ones allocated to migrate them and then finally,
> > +        * the device driver can use the pages for itself.
> > +        * Thus, migrated, the pages being accessed by DMA could be
> changed
> > +        * to other so this situation may incur that DMA accesses any
> pages
> > +        * it doesn't want.
> > +        *
> > +        * But the use of get_user_pages is safe from such magration
> issue
> > +        * because all the pages from get_user_pages CAN NOT be not only
> > +        * migrated, but also swapped out.
> > +        */
> > +       get_npages = get_user_pages(current, current->mm, userptr,
> > +                                       npages, write, 1, buf->pages,
NULL);
> > +       up_read(&current->mm->mmap_sem);
> > +       if (get_npages != npages)
> > +               DRM_ERROR("failed to get user_pages.\n");
> > +
> > +       buf->userptr = userptr;
> > +       buf->pfnmap = false;
> > +
> > +       return get_npages;
> > +}
> > +
> > +int exynos_drm_gem_userptr_ioctl(struct drm_device *dev, void *data,
> > +                                     struct drm_file *file_priv)
> > +{
> > +       struct exynos_drm_private *priv = dev->dev_private;
> > +       struct exynos_drm_gem_obj *exynos_gem_obj;
> > +       struct drm_exynos_gem_userptr *args = data;
> > +       struct exynos_drm_gem_buf *buf;
> > +       struct scatterlist *sgl;
> > +       unsigned long size, userptr;
> > +       unsigned int npages;
> > +       int ret, get_npages;
> > +
> > +       DRM_DEBUG_KMS("%s\n", __FILE__);
> > +
> > +       if (!args->size) {
> > +               DRM_ERROR("invalid size.\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       ret = check_gem_flags(args->flags);
> > +       if (ret)
> > +               return ret;
> > +
> > +       size = roundup_gem_size(args->size, EXYNOS_BO_USERPTR);
> > +
> > +       if (size > priv->userptr_limit) {
> > +               DRM_ERROR("excessed maximum size of userptr.\n");
> > +               return -EINVAL;
> > +       }
> > +
> > +       mutex_lock(&dev->struct_mutex);
> > +
> > +       /*
> > +        * Limited userptr size
> > +        * - User request with userptr SHOULD BE limited as
userptr_limit
> size
> > +        * Malicious process is possible to make all the processes to be
> > +        * blocked so this would incur a deterioation of system
> > +        * performance. so the size that user can request is limited as
> > +        * userptr_limit value and also the value CAN BE changed by only
> root
> > +        * user.
> > +        */
> > +       if (priv->userptr_limit >= size)
> > +               priv->userptr_limit -= size;
> > +       else {
> > +               DRM_DEBUG_KMS("insufficient userptr size.\n");
> > +               mutex_unlock(&dev->struct_mutex);
> > +               return -EINVAL;
> > +       }
> > +
> > +       mutex_unlock(&dev->struct_mutex);
> > +
> > +       userptr = args->userptr;
> > +
> > +       buf = exynos_drm_init_buf(dev, size);
> > +       if (!buf)
> > +               return -ENOMEM;
> > +
> > +       exynos_gem_obj = exynos_drm_gem_init(dev, size);
> > +       if (!exynos_gem_obj) {
> > +               ret = -ENOMEM;
> > +               goto err_free_buffer;
> > +       }
> > +
> > +       buf->sgt = kzalloc(sizeof(struct sg_table), GFP_KERNEL);
> > +       if (!buf->sgt) {
> > +               DRM_ERROR("failed to allocate buf->sgt.\n");
> > +               ret = -ENOMEM;
> > +               goto err_release_gem;
> > +       }
> > +
> > +       npages = size >> PAGE_SHIFT;
> > +
> > +       ret = sg_alloc_table(buf->sgt, npages, GFP_KERNEL);
> > +       if (ret < 0) {
> > +               DRM_ERROR("failed to initailize sg table.\n");
> > +               goto err_free_sgt;
> > +       }
> > +
> > +       buf->pages = kzalloc(npages * sizeof(struct page *),
GFP_KERNEL);
> > +       if (!buf->pages) {
> > +               DRM_ERROR("failed to allocate buf->pages\n");
> > +               ret = -ENOMEM;
> > +               goto err_free_table;
> > +       }
> > +
> > +       exynos_gem_obj->buffer = buf;
> > +
> > +       get_npages = exynos_drm_get_userptr(dev, exynos_gem_obj,
userptr,
> 1);
> > +       if (get_npages != npages) {
> > +               DRM_ERROR("failed to get user_pages.\n");
> > +               ret = get_npages;
> > +               goto err_release_userptr;
> > +       }
> > +
> > +       ret = exynos_drm_gem_handle_create(&exynos_gem_obj->base,
> file_priv,
> > +                                               &args->handle);
> > +       if (ret < 0) {
> > +               DRM_ERROR("failed to create gem handle.\n");
> > +               goto err_release_userptr;
> > +       }
> > +
> > +       sgl = buf->sgt->sgl;
> > +
> > +       /*
> > +        * if buf->pfnmap is true then update sgl of sgt with pages else
> > +        * then it means the sgl was updated already so it doesn't need
> > +        * to update the sgl.
> > +        */
> > +       if (!buf->pfnmap) {
> > +               unsigned int i = 0;
> > +
> > +               /* set all pages to sg list. */
> > +               while (i < npages) {
> > +                       sg_set_page(sgl, buf->pages[i], PAGE_SIZE, 0);
> > +                       sg_dma_address(sgl) =
page_to_phys(buf->pages[i]);
> > +                       i++;
> > +                       sgl = sg_next(sgl);
> > +               }
> > +       }
> > +
> > +       /* always use EXYNOS_BO_USERPTR as memory type for userptr. */
> > +       exynos_gem_obj->flags |= EXYNOS_BO_USERPTR;
> > +
> > +       return 0;
> > +
> > +err_release_userptr:
> > +       get_npages--;
> > +       while (get_npages >= 0)
> > +               put_page(buf->pages[get_npages--]);
> > +       kfree(buf->pages);
> > +       buf->pages = NULL;
> > +err_free_table:
> > +       sg_free_table(buf->sgt);
> > +err_free_sgt:
> > +       kfree(buf->sgt);
> > +       buf->sgt = NULL;
> > +err_release_gem:
> > +       drm_gem_object_release(&exynos_gem_obj->base);
> > +       kfree(exynos_gem_obj);
> > +       exynos_gem_obj = NULL;
> > +err_free_buffer:
> > +       exynos_drm_free_buf(dev, 0, buf);
> > +       return ret;
> > +}
> > +
> >  int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data,
> >                                      struct drm_file *file_priv)
> >  {      struct exynos_drm_gem_obj *exynos_gem_obj;
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> > index 3334c9f..72bd993 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_gem.h
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_gem.h
> > @@ -29,27 +29,35 @@
> >  #define to_exynos_gem_obj(x)   container_of(x,\
> >                        struct exynos_drm_gem_obj, base)
> >
> > -#define IS_NONCONTIG_BUFFER(f)         (f & EXYNOS_BO_NONCONTIG)
> > +#define IS_NONCONTIG_BUFFER(f) ((f & EXYNOS_BO_NONCONTIG) ||\
> > +                                       (f & EXYNOS_BO_USERPTR))
> >
> >  /*
> >  * exynos drm gem buffer structure.
> >  *
> >  * @kvaddr: kernel virtual address to allocated memory region.
> > + * @userptr: user space address.
> >  * @dma_addr: bus address(accessed by dma) to allocated memory region.
> >  *     - this address could be physical address without IOMMU and
> >  *     device address with IOMMU.
> > + * @write: whether pages will be written to by the caller.
> >  * @sgt: sg table to transfer page data.
> >  * @pages: contain all pages to allocated memory region.
> >  * @page_size: could be 4K, 64K or 1MB.
> >  * @size: size of allocated memory region.
> > + * @pfnmap: indicate whether memory region from userptr is mmaped with
> > + *     VM_PFNMAP or not.
> >  */
> >  struct exynos_drm_gem_buf {
> >        void __iomem            *kvaddr;
> > +       unsigned long           userptr;
> >        dma_addr_t              dma_addr;
> > +       unsigned int            write;
> >        struct sg_table         *sgt;
> >        struct page             **pages;
> >        unsigned long           page_size;
> >        unsigned long           size;
> > +       bool                    pfnmap;
> >  };
> >
> >  /*
> > @@ -64,6 +72,8 @@ struct exynos_drm_gem_buf {
> >  *     continuous memory region allocated by user request
> >  *     or at framebuffer creation.
> >  * @size: total memory size to physically non-continuous memory region.
> > + * @vma: a pointer to the vma to user address space and used to release
> > + *     the pages to user space.
> >  * @flags: indicate memory type to allocated buffer and cache
attruibute.
> >  *
> >  * P.S. this object would be transfered to user as kms_bo.handle so
> > @@ -73,6 +83,7 @@ struct exynos_drm_gem_obj {
> >        struct drm_gem_object           base;
> >        struct exynos_drm_gem_buf       *buffer;
> >        unsigned long                   size;
> > +       struct vm_area_struct           *vma;
> >        unsigned int                    flags;
> >  };
> >
> > @@ -130,6 +141,10 @@ int exynos_drm_gem_map_offset_ioctl(struct
> drm_device *dev, void *data,
> >  int exynos_drm_gem_mmap_ioctl(struct drm_device *dev, void *data,
> >                              struct drm_file *file_priv);
> >
> > +/* map user space allocated by malloc to pages. */
> > +int exynos_drm_gem_userptr_ioctl(struct drm_device *dev, void *data,
> > +                                     struct drm_file *file_priv);
> > +
> >  /* get buffer information to memory region allocated by gem. */
> >  int exynos_drm_gem_get_ioctl(struct drm_device *dev, void *data,
> >                                      struct drm_file *file_priv);
> > diff --git a/include/drm/exynos_drm.h b/include/drm/exynos_drm.h
> > index 52465dc..33fa1e2 100644
> > --- a/include/drm/exynos_drm.h
> > +++ b/include/drm/exynos_drm.h
> > @@ -77,6 +77,23 @@ struct drm_exynos_gem_mmap {
> >  };
> >
> >  /**
> > + * User-requested user space importing structure
> > + *
> > + * @userptr: user space address allocated by malloc.
> > + * @size: size to the buffer allocated by malloc.
> > + * @flags: indicate user-desired cache attribute to map the allocated
> buffer
> > + *     to kernel space.
> > + * @handle: a returned handle to created gem object.
> > + *     - this handle will be set by gem module of kernel side.
> > + */
> > +struct drm_exynos_gem_userptr {
> > +       uint64_t userptr;
> > +       uint64_t size;
> > +       unsigned int flags;
> > +       unsigned int handle;
> > +};
> > +
> > +/**
> >  * A structure to gem information.
> >  *
> >  * @handle: a handle to gem object created.
> > @@ -135,8 +152,10 @@ enum e_drm_exynos_gem_mem_type {
> >        EXYNOS_BO_CACHABLE      = 1 << 1,
> >        /* write-combine mapping. */
> >        EXYNOS_BO_WC            = 1 << 2,
> > +       /* user space memory allocated by malloc. */
> > +       EXYNOS_BO_USERPTR       = 1 << 3,
> >        EXYNOS_BO_MASK          = EXYNOS_BO_NONCONTIG |
EXYNOS_BO_CACHABLE
> |
> > -                                       EXYNOS_BO_WC
> > +                                       EXYNOS_BO_WC | EXYNOS_BO_USERPTR
> >  };
> >
> >  struct drm_exynos_g2d_get_ver {
> > @@ -173,7 +192,7 @@ struct drm_exynos_g2d_exec {
> >  #define DRM_EXYNOS_GEM_CREATE          0x00
> >  #define DRM_EXYNOS_GEM_MAP_OFFSET      0x01
> >  #define DRM_EXYNOS_GEM_MMAP            0x02
> > -/* Reserved 0x03 ~ 0x05 for exynos specific gem ioctl */
> > +#define DRM_EXYNOS_GEM_USERPTR         0x03
> >  #define DRM_EXYNOS_GEM_GET             0x04
> >  #define DRM_EXYNOS_USER_LIMIT          0x05
> >  #define DRM_EXYNOS_PLANE_SET_ZPOS      0x06
> > @@ -193,6 +212,9 @@ struct drm_exynos_g2d_exec {
> >  #define DRM_IOCTL_EXYNOS_GEM_MMAP      DRM_IOWR(DRM_COMMAND_BASE + \
> >                DRM_EXYNOS_GEM_MMAP, struct drm_exynos_gem_mmap)
> >
> > +#define DRM_IOCTL_EXYNOS_GEM_USERPTR   DRM_IOWR(DRM_COMMAND_BASE + \
> > +               DRM_EXYNOS_GEM_USERPTR, struct drm_exynos_gem_userptr)
> > +
> >  #define DRM_IOCTL_EXYNOS_GEM_GET       DRM_IOWR(DRM_COMMAND_BASE + \
> >                DRM_EXYNOS_GEM_GET,     struct drm_exynos_gem_info)
> >
> > --
> > 1.7.4.1
> >

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2 v4] drm/exynos: added userptr feature.

[Jerome Glisse]

On Tue, May 15, 2012 at 12:33 AM, Inki Dae <inki.dae@samsung.com> wrote:
> Hi Jerome,
>
>> -----Original Message-----
>> From: Jerome Glisse [mailto:j.glisse@gmail.com]
>> Sent: Tuesday, May 15, 2012 4:27 AM
>> To: Inki Dae
>> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org; minchan@kernel.org;
>> kosaki.motohiro@gmail.com; kyungmin.park@samsung.com;
>> sw0312.kim@samsung.com; jy0922.shim@samsung.com
>> Subject: Re: [PATCH 2/2 v4] drm/exynos: added userptr feature.
>>
>> On Mon, May 14, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com> wrote:
>> > this feature is used to import user space region allocated by malloc()
>> or
>> > mmaped into a gem. for this, we uses get_user_pages() to get all the
>> pages
>> > to VMAs within user address space. However we should pay attention to
>> use
>> > this userptr feature like below.
>> >
>> > The migration issue.
>> > - Pages reserved by CMA for some device using DMA could be used by
>> > kernel and if the device driver wants to use those pages
>> > while being used by kernel then the pages are copied into
>> > other ones allocated to migrate them and then finally,
>> > the device driver can use the pages for itself.
>> > Thus, migrated, the pages being accessed by DMA could be changed
>> > to other so this situation may incur that DMA accesses any pages
>> > it doesn't want.
>> >
>> > The COW issue.
>> > - while DMA of a device is using the pages to VMAs, if current
>> > process was forked then the pages being accessed by the DMA
>> > would be copied into child's pages.(Copy On Write) so
>> > these pages may not have coherrency with parent's ones if
>> > child process wrote something on those pages so we need to
>> > flag VM_DONTCOPY to prevent pages from being COWed.
>>
>> Note that this is a massive change in behavior of anonymous mapping
>> this effectively completely change the linux API from application
>> point of view on your platform. Any application that have memory
>> mapped by your ioctl will have different fork behavior that other
>> application. I think this should be stressed, it's one of the thing i
>> am really uncomfortable with i would rather not have the dont copy
>> flag and have the page cowed and have the child not working with the
>> 3d/2d/drm driver. That would means that your driver (opengl
>> implementation for instance) would have to detect fork and work around
>> it, nvidia closed source driver do that.
>>
>
> First of all, thank you for your comments.
>
> Right, VM_DONTCOPY flag would change original behavior of user. Do you think
> this way has no problem but no generic way? anyway our issue was that the
> pages to VMAs are copied into child's ones(COW) so we prevented those pages
> from being COWed with using VM_DONTCOPY flag.
>
> For this, I have three questions below
>
> 1. in case of not using VM_DONTCOPY flag, you think that the application
> using our userptr feature has COW issue; parent's pages being accessed by
> DMA of some device would be copied into child's ones if the child wrote
> something on the pages. after that, DMA of a device could access pages user
> doesn't want. I'm not sure but I think such behavior has no any problem and
> is generic behavior and it's role of user to do fork or not. Do you think
> such COW behavior could create any issue I don't aware of so we have to
> prevent that somehow?

My point is the father will keep the page that the GPU know about as
long as the father dont destroy the associated object. But if the
child expect to be able to use the same GPU object and still be able
to change the content through its anonymous mapping than i would
consider this behavior buggy (ie application have wrong expectation).
So i am all for only the father is able to keep its memory mapped into
GPU address space through same GEM object.

> 2. so we added VM_DONTCOPY flag to prevent the pages from being COWed but
> this changes original behavior of user. Do you think this is not generic way
> or could create any issue also?

I would say don't add the flag and consider application that do fork
as special case in userspace. See below for how i would handle it.

> 3. and last one, what is the difference between to flag VM_DONTCOPY and to
> detect fork? I mean the device driver should do something to need after
> detecting fork. and I'm not sure but I think the something may also change
> original behavior of user.
>
> Please let me know if there is my missing point.

I would detect fork by storing process id along gem object. So
something like (userspace code that could be in your pixman library):

struct gpu_object_process {
  struct list list;
  uint32_t gem_handle;
  unsigned process_id;
};

struct gpu_object {
  struct list gpu_object_process;
  void *ptr;
  unsigned size;
  ...
}

When creating a GPU object from userptr you fill the above structure
in the userspace code. Then whenever you library want to use this
object it call something like:

int gpu_object_validate(struct gpu_object *bo)

Which check if there is the current process id in the
gpu_object_process list, if there is one then use the gem object
handle, otherwise you create a new GEM object using this userptr and
same size and other properties.

Note you really need this only in case you expect application using
you library to fork and still expect to use your gpu accelerated
library in the same way.

So doing this you conserve proper unix fork behavior, child change to
anonymous memory don't reflect into the father anonymous memory and
that should be the expected behavior even regarding GPU object. Of
course this means there would be memcpy btw father and child on write
but that's the expected behavior of fork.

Note also that i don't expect any of your graphic application to use
fork so in most case your  gpu_object_process list would be only one
element.

Cheers,
Jerome

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

RE: [PATCH 2/2 v4] drm/exynos: added userptr feature.

[Inki Dae]

> -----Original Message-----
> From: Jerome Glisse [mailto:j.glisse@gmail.com]
> Sent: Tuesday, May 15, 2012 11:31 PM
> To: Inki Dae
> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org; linux-mm@kvack.org;
> minchan@kernel.org; kosaki.motohiro@gmail.com; kyungmin.park@samsung.com;
> sw0312.kim@samsung.com; jy0922.shim@samsung.com
> Subject: Re: [PATCH 2/2 v4] drm/exynos: added userptr feature.
> 
> On Tue, May 15, 2012 at 12:33 AM, Inki Dae <inki.dae@samsung.com> wrote:
> > Hi Jerome,
> >
> >> -----Original Message-----
> >> From: Jerome Glisse [mailto:j.glisse@gmail.com]
> >> Sent: Tuesday, May 15, 2012 4:27 AM
> >> To: Inki Dae
> >> Cc: airlied@linux.ie; dri-devel@lists.freedesktop.org;
> minchan@kernel.org;
> >> kosaki.motohiro@gmail.com; kyungmin.park@samsung.com;
> >> sw0312.kim@samsung.com; jy0922.shim@samsung.com
> >> Subject: Re: [PATCH 2/2 v4] drm/exynos: added userptr feature.
> >>
> >> On Mon, May 14, 2012 at 2:17 AM, Inki Dae <inki.dae@samsung.com> wrote:
> >> > this feature is used to import user space region allocated by
malloc()
> >> or
> >> > mmaped into a gem. for this, we uses get_user_pages() to get all the
> >> pages
> >> > to VMAs within user address space. However we should pay attention to
> >> use
> >> > this userptr feature like below.
> >> >
> >> > The migration issue.
> >> > - Pages reserved by CMA for some device using DMA could be used by
> >> > kernel and if the device driver wants to use those pages
> >> > while being used by kernel then the pages are copied into
> >> > other ones allocated to migrate them and then finally,
> >> > the device driver can use the pages for itself.
> >> > Thus, migrated, the pages being accessed by DMA could be changed
> >> > to other so this situation may incur that DMA accesses any pages
> >> > it doesn't want.
> >> >
> >> > The COW issue.
> >> > - while DMA of a device is using the pages to VMAs, if current
> >> > process was forked then the pages being accessed by the DMA
> >> > would be copied into child's pages.(Copy On Write) so
> >> > these pages may not have coherrency with parent's ones if
> >> > child process wrote something on those pages so we need to
> >> > flag VM_DONTCOPY to prevent pages from being COWed.
> >>
> >> Note that this is a massive change in behavior of anonymous mapping
> >> this effectively completely change the linux API from application
> >> point of view on your platform. Any application that have memory
> >> mapped by your ioctl will have different fork behavior that other
> >> application. I think this should be stressed, it's one of the thing i
> >> am really uncomfortable with i would rather not have the dont copy
> >> flag and have the page cowed and have the child not working with the
> >> 3d/2d/drm driver. That would means that your driver (opengl
> >> implementation for instance) would have to detect fork and work around
> >> it, nvidia closed source driver do that.
> >>
> >
> > First of all, thank you for your comments.
> >
> > Right, VM_DONTCOPY flag would change original behavior of user. Do you
> think
> > this way has no problem but no generic way? anyway our issue was that
> the
> > pages to VMAs are copied into child's ones(COW) so we prevented those
> pages
> > from being COWed with using VM_DONTCOPY flag.
> >
> > For this, I have three questions below
> >
> > 1. in case of not using VM_DONTCOPY flag, you think that the application
> > using our userptr feature has COW issue; parent's pages being accessed
> by
> > DMA of some device would be copied into child's ones if the child wrote
> > something on the pages. after that, DMA of a device could access pages
> user
> > doesn't want. I'm not sure but I think such behavior has no any problem
> and
> > is generic behavior and it's role of user to do fork or not. Do you
> think
> > such COW behavior could create any issue I don't aware of so we have to
> > prevent that somehow?
> 
> My point is the father will keep the page that the GPU know about as
> long as the father dont destroy the associated object. But if the
> child expect to be able to use the same GPU object and still be able
> to change the content through its anonymous mapping than i would
> consider this behavior buggy (ie application have wrong expectation).
> So i am all for only the father is able to keep its memory mapped into
> GPU address space through same GEM object.
> 
> > 2. so we added VM_DONTCOPY flag to prevent the pages from being COWed
> but
> > this changes original behavior of user. Do you think this is not generic
> way
> > or could create any issue also?
> 
> I would say don't add the flag and consider application that do fork
> as special case in userspace. See below for how i would handle it.
> 
> > 3. and last one, what is the difference between to flag VM_DONTCOPY and
> to
> > detect fork? I mean the device driver should do something to need after
> > detecting fork. and I'm not sure but I think the something may also
> change
> > original behavior of user.
> >
> > Please let me know if there is my missing point.
> 
> I would detect fork by storing process id along gem object. So
> something like (userspace code that could be in your pixman library):
> 
> struct gpu_object_process {
>   struct list list;
>   uint32_t gem_handle;
>   unsigned process_id;
> };
> 
> struct gpu_object {
>   struct list gpu_object_process;
>   void *ptr;
>   unsigned size;
>   ...
> }
> 
> When creating a GPU object from userptr you fill the above structure
> in the userspace code. Then whenever you library want to use this
> object it call something like:
> 
> int gpu_object_validate(struct gpu_object *bo)
> 
> Which check if there is the current process id in the
> gpu_object_process list, if there is one then use the gem object
> handle, otherwise you create a new GEM object using this userptr and
> same size and other properties.
> 
> Note you really need this only in case you expect application using
> you library to fork and still expect to use your gpu accelerated
> library in the same way.
> 
> So doing this you conserve proper unix fork behavior, child change to
> anonymous memory don't reflect into the father anonymous memory and
> that should be the expected behavior even regarding GPU object. Of
> course this means there would be memcpy btw father and child on write
> but that's the expected behavior of fork.
> 
> Note also that i don't expect any of your graphic application to use
> fork so in most case your  gpu_object_process list would be only one
> element.
> 

Thanks for detailed example and that would be very helpful to me and also I
think gpu_object_validate() should be called in normal case(not userptr
case). User can allocate a new gem and map it with its own user space. after
that, if fork is done then it would have same issue as userptr. so
gpu_object_validate() should be called before mapping the gem with user
space also. I understood what you mention and I will unset VM_DONTCOPY flag
and in our case, EXA backend will check that. for this, I gonna add some
comments enough to next patch. please let me know if there is my missing
point.

Thanks,
Inki Dae

> Cheers,
> Jerome

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>