mm: Introduce new function vm_insert_kmem_page

vm_insert_kmem_page is similar to vm_insert_page and will be used by drivers to map kernel (kmalloc/vmalloc/pages) allocated memory to user vma.

Going forward, the plan is to restrict future drivers not to use vm_insert_page ( *it will generate new errno to VM_FAULT_CODE mapping code for new drivers which were already cleaned up for existing drivers*) in #PF (page fault handler) context but to make use of vmf_insert_page which returns VMF_FAULT_CODE and that is not possible until both vm_insert_page and vmf_insert_page API exists.

But there are some consumers of vm_insert_page which use it outside #PF context. straight forward conversion of vm_insert_page to vmf_insert_page won't work there as those function calls expects errno not vm_fault_t in return.

These are the approaches which could have been taken to handle this scenario -

* Replace vm_insert_page with vmf_insert_page and then write few extra lines of code to convert VM_FAULT_CODE to errno which makes driver users more complex ( also the reverse mapping errno to VM_FAULT_CODE have been cleaned up as part of vm_fault_t migration , not preferred to introduce anything similar again)

* Maintain both vm_insert_page and vmf_insert_page and use it in respective places. But it won't gurantee that vm_insert_page will never be used in #PF context.

* Introduce a similar API like vm_insert_page, convert all non #PF consumer to use it and finally remove vm_insert_page by converting it to vmf_insert_page.

And the 3rd approach was taken by introducing vm_insert_kmem_page().

In short, vmf_insert_page will be used in page fault handlers context and vm_insert_kmem_page will be used to map kernel memory to user vma outside page fault handlers context.

Few drivers are converted to use vm_insert_kmem_page(). This will allow both to review the api and that it serves it purpose. other consumers of vm_insert_page (*used in non #PF context*) will be replaced by vm_insert_kmem_page, but in separate patches.

Signed-off-by Souptick Joarder <[email protected]>

---
v2: Few non #PF consumers of vm_insert_page are converted
to use vm_insert_kmem_page in patch v2.

Updated the change log.

 arch/arm/mm/dma-mapping.c                   |  2 +-
 drivers/auxdisplay/cfag12864bfb.c           |  2 +-
 drivers/auxdisplay/ht16k33.c                |  2 +-
 drivers/firewire/core-iso.c                 |  2 +-
 drivers/gpu/drm/rockchip/rockchip_drm_gem.c |  2 +-
 include/linux/mm.h                          |  2 +
 kernel/kcov.c                               |  4 +-
 mm/memory.c                                 | 69 +++++++++++++++++++++++++++++
 mm/nommu.c                                  |  7 +++
 mm/vmalloc.c                                |  2 +-
 10 files changed, 86 insertions(+), 8 deletions(-)

arch/arm/mm/dma-mapping.c +1 -1

index 6656647..58d7971 100644
@@ -1598,7 +1598,7 @@ static int __arm_iommu_mmap_attrs(struct device *dev, struct vm_area_struct *vma
        pages += off;
 
        do {
-               int ret = vm_insert_page(vma, uaddr, *pages++);
+               int ret = vm_insert_kmem_page(vma, uaddr, *pages++);
                if (ret) {
                        pr_err("Remapping memory failed: %d\n", ret);
                        return ret;

drivers/auxdisplay/cfag12864bfb.c +1 -1

index 40c8a55..82fd627 100644
@@ -52,7 +52,7 @@
 
 static int cfag12864bfb_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
-       return vm_insert_page(vma, vma->vm_start,
+       return vm_insert_kmem_page(vma, vma->vm_start,
                virt_to_page(cfag12864b_buffer));
 }
 

drivers/auxdisplay/ht16k33.c +1 -1

index a43276c..64de30b 100644
@@ -224,7 +224,7 @@ static int ht16k33_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
        struct ht16k33_priv *priv = info->par;
 
-       return vm_insert_page(vma, vma->vm_start,
+       return vm_insert_kmem_page(vma, vma->vm_start,
                              virt_to_page(priv->fbdev.buffer));
 }
 

drivers/firewire/core-iso.c +1 -1

index 051327a..5f1548d 100644
@@ -112,7 +112,7 @@ int fw_iso_buffer_map_vma(struct fw_iso_buffer *buffer,
 
        uaddr = vma->vm_start;
        for (i = 0; i < buffer->page_count; i++) {
-               err = vm_insert_page(vma, uaddr, buffer->pages[i]);
+               err = vm_insert_kmem_page(vma, uaddr, buffer->pages[i]);
                if (err)
                        return err;
 

drivers/gpu/drm/rockchip/rockchip_drm_gem.c +1 -1

index a8db758..57eb7af 100644
@@ -234,7 +234,7 @@ static int rockchip_drm_gem_object_mmap_iommu(struct drm_gem_object *obj,
                return -ENXIO;
 
        for (i = offset; i < end; i++) {
-               ret = vm_insert_page(vma, uaddr, rk_obj->pages[i]);
+               ret = vm_insert_kmem_page(vma, uaddr, rk_obj->pages[i]);
                if (ret)
                        return ret;
                uaddr += PAGE_SIZE;

include/linux/mm.h +2 -0

index a61ebe8..5f42d35 100644
@@ -2477,6 +2477,8 @@ unsigned long change_prot_numa(struct vm_area_struct *vma,
 struct vm_area_struct *find_extend_vma(struct mm_struct *, unsigned long addr);
 int remap_pfn_range(struct vm_area_struct *, unsigned long addr,
                        unsigned long pfn, unsigned long size, pgprot_t);
+int vm_insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
+                               struct page *page);
 int vm_insert_page(struct vm_area_struct *, unsigned long addr, struct page *);
 int vm_insert_pfn(struct vm_area_struct *vma, unsigned long addr,
                        unsigned long pfn);

kernel/kcov.c +2 -2

index 3ebd09e..2afaeb4 100644
@@ -293,8 +293,8 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
                spin_unlock(&kcov->lock);
                for (off = 0; off < size; off += PAGE_SIZE) {
                        page = vmalloc_to_page(kcov->area + off);
-                       if (vm_insert_page(vma, vma->vm_start + off, page))
-                               WARN_ONCE(1, "vm_insert_page() failed");
+                       if (vm_insert_kmem_page(vma, vma->vm_start + off, page))
+                               WARN_ONCE(1, "vm_insert_kmem_page() failed");
                }
                return 0;
        }

mm/memory.c +37 -0

index c467102..b800c10 100644
@@ -1682,6 +1682,75 @@ pte_t *__get_locked_pte(struct mm_struct *mm, unsigned long addr,
        return pte_alloc_map_lock(mm, pmd, addr, ptl);
 }
 
+static int insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
+               struct page *page, pgprot_t prot)
+{
+       struct mm_struct *mm = vma->vm_mm;
+       int retval;
+       pte_t *pte;
+       spinlock_t *ptl;
+
+       retval = -EINVAL;
+       if (PageAnon(page))
+               goto out;
+       retval = -ENOMEM;
+       flush_dcache_page(page);
+       pte = get_locked_pte(mm, addr, &ptl);
+       if (!pte)
+               goto out;
+       retval = -EBUSY;
+       if (!pte_none(*pte))
+               goto out_unlock;
+
+       get_page(page);
+       inc_mm_counter_fast(mm, mm_counter_file(page));
+       page_add_file_rmap(page, false);
+       set_pte_at(mm, addr, pte, mk_pte(page, prot));
+
+       retval = 0;
+       pte_unmap_unlock(pte, ptl);
+       return retval;
+out_unlock:
+       pte_unmap_unlock(pte, ptl);
+out:
+       return retval;
+}
+
+/**
+ * vm_insert_kmem_page - insert single page into user vma
+ * @vma: user vma to map to

mm/memory.c +16 -0

+int vm_insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
+                       struct page *page)
+{
+       if (addr < vma->vm_start || addr >= vma->vm_end)
+               return -EFAULT;
+       if (!page_count(page))
+               return -EINVAL;
+       if (!(vma->vm_flags & VM_MIXEDMAP)) {
+               BUG_ON(down_read_trylock(&vma->vm_mm->mmap_sem));
+               BUG_ON(vma->vm_flags & VM_PFNMAP);
+               vma->vm_flags |= VM_MIXEDMAP;
+       }
+       return insert_kmem_page(vma, addr, page, vma->vm_page_prot);
+}
+EXPORT_SYMBOL(vm_insert_kmem_page);
+
 /*
  * This is the old fallback for page remapping.
  *

mm/nommu.c +7 -0

index e4aac33..153b8c8 100644
@@ -473,6 +473,13 @@ int vm_insert_page(struct vm_area_struct *vma, unsigned long addr,
 }
 EXPORT_SYMBOL(vm_insert_page);
 
+int vm_insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
+                               struct page *page)
+{
+       return -EINVAL;
+}
+EXPORT_SYMBOL(vm_insert_kmem_page);
+
 /*
  *  sys_brk() for the most part doesn't need the global kernel
  *  lock, except when an application is doing something nasty

mm/vmalloc.c +1 -2

index a728fc4..61d279f 100644
@@ -2251,7 +2251,7 @@ int remap_vmalloc_range_partial(struct vm_area_struct *vma, unsigned long uaddr,
                struct page *page = vmalloc_to_page(kaddr);
                int ret;
 
-               ret = vm_insert_page(vma, uaddr, page);
+               ret = vm_insert_kmem_page(vma, uaddr, page);
                if (ret)
                        return ret;
 
--

1.9.1

Hi Souptick,

Quoted 31 lines · Souptick Joarder

vm_insert_kmem_page is similar to vm_insert_page and will be used by drivers to map kernel (kmalloc/vmalloc/pages) allocated memory to user vma.

Going forward, the plan is to restrict future drivers not to use vm_insert_page ( *it will generate new errno to VM_FAULT_CODE mapping code for new drivers which were already cleaned up for existing drivers*) in #PF (page fault handler) context but to make use of vmf_insert_page which returns VMF_FAULT_CODE and that is not possible until both vm_insert_page and vmf_insert_page API exists.

But there are some consumers of vm_insert_page which use it outside #PF context. straight forward conversion of vm_insert_page to vmf_insert_page won't work there as those function calls expects errno not vm_fault_t in return.

These are the approaches which could have been taken to handle this scenario -

* Replace vm_insert_page with vmf_insert_page and then write few extra lines of code to convert VM_FAULT_CODE to errno which makes driver users more complex ( also the reverse mapping errno to VM_FAULT_CODE have been cleaned up as part of vm_fault_t migration , not preferred to introduce anything similar again)

* Maintain both vm_insert_page and vmf_insert_page and use it in respective places. But it won't gurantee that vm_insert_page will never be used in #PF context.

Quoted 5 lines

* Introduce a similar API like vm_insert_page, convert all non #PF consumer to use it and finally remove vm_insert_page by converting it to vmf_insert_page.

And the 3rd approach was taken by introducing vm_insert_kmem_page().

This looks better than the previous one of adding non-trivial code to each driver, thank you!

A couple of comments below.

Quoted 10 lines

In short, vmf_insert_page will be used in page fault handlers context and vm_insert_kmem_page will be used to map kernel memory to user vma outside page fault handlers context.

Few drivers are converted to use vm_insert_kmem_page(). This will allow both to review the api and that it serves it purpose. other consumers of vm_insert_page (*used in non #PF context*) will be replaced by vm_insert_kmem_page, but in separate patches.

other -> Other

Also, as far as I can see, there are only a few vm_insert_page users remaining. With the new function, they should be trivial to convert, no? Therefore, could we do them all in one go, possibly in a patch series?

Or, maybe, even better: wait until you remove the vm_* functions and simply reuse vm_insert_page for this -- that way you don't need a new name and you don't have to change any of the last users (I mean the drivers using it outside the page fault handlers).

Quoted 157 lines

Signed-off-by Souptick Joarder <[email protected]>

---
v2: Few non #PF consumers of vm_insert_page are converted
to use vm_insert_kmem_page in patch v2.

Updated the change log.

 arch/arm/mm/dma-mapping.c                   |  2 +-
 drivers/auxdisplay/cfag12864bfb.c           |  2 +-
 drivers/auxdisplay/ht16k33.c                |  2 +-
 drivers/firewire/core-iso.c                 |  2 +-
 drivers/gpu/drm/rockchip/rockchip_drm_gem.c |  2 +-
 include/linux/mm.h                          |  2 +
 kernel/kcov.c                               |  4 +-
 mm/memory.c                                 | 69 +++++++++++++++++++++++++++++
 mm/nommu.c                                  |  7 +++
 mm/vmalloc.c                                |  2 +-
 10 files changed, 86 insertions(+), 8 deletions(-)

arch/arm/mm/dma-mapping.c +1 -1

index 6656647..58d7971 100644
@@ -1598,7 +1598,7 @@ static int __arm_iommu_mmap_attrs(struct device *dev, struct vm_area_struct *vma
        pages += off;
 
        do {
-               int ret = vm_insert_page(vma, uaddr, *pages++);
+               int ret = vm_insert_kmem_page(vma, uaddr, *pages++);
                if (ret) {
                        pr_err("Remapping memory failed: %d\n", ret);
                        return ret;

drivers/auxdisplay/cfag12864bfb.c +1 -1

index 40c8a55..82fd627 100644
@@ -52,7 +52,7 @@
 
 static int cfag12864bfb_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
-       return vm_insert_page(vma, vma->vm_start,
+       return vm_insert_kmem_page(vma, vma->vm_start,
                virt_to_page(cfag12864b_buffer));
 }
 

drivers/auxdisplay/ht16k33.c +1 -1

index a43276c..64de30b 100644
@@ -224,7 +224,7 @@ static int ht16k33_mmap(struct fb_info *info, struct vm_area_struct *vma)
 {
        struct ht16k33_priv *priv = info->par;
 
-       return vm_insert_page(vma, vma->vm_start,
+       return vm_insert_kmem_page(vma, vma->vm_start,
                              virt_to_page(priv->fbdev.buffer));
 }
 

drivers/firewire/core-iso.c +1 -1

index 051327a..5f1548d 100644
@@ -112,7 +112,7 @@ int fw_iso_buffer_map_vma(struct fw_iso_buffer *buffer,
 
        uaddr = vma->vm_start;
        for (i = 0; i < buffer->page_count; i++) {
-               err = vm_insert_page(vma, uaddr, buffer->pages[i]);
+               err = vm_insert_kmem_page(vma, uaddr, buffer->pages[i]);
                if (err)
                        return err;
 

drivers/gpu/drm/rockchip/rockchip_drm_gem.c +1 -1

index a8db758..57eb7af 100644
@@ -234,7 +234,7 @@ static int rockchip_drm_gem_object_mmap_iommu(struct drm_gem_object *obj,
                return -ENXIO;
 
        for (i = offset; i < end; i++) {
-               ret = vm_insert_page(vma, uaddr, rk_obj->pages[i]);
+               ret = vm_insert_kmem_page(vma, uaddr, rk_obj->pages[i]);
                if (ret)
                        return ret;
                uaddr += PAGE_SIZE;

include/linux/mm.h +2 -0

index a61ebe8..5f42d35 100644
@@ -2477,6 +2477,8 @@ unsigned long change_prot_numa(struct vm_area_struct *vma,
 struct vm_area_struct *find_extend_vma(struct mm_struct *, unsigned long addr);
 int remap_pfn_range(struct vm_area_struct *, unsigned long addr,
                        unsigned long pfn, unsigned long size, pgprot_t);
+int vm_insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
+                               struct page *page);
 int vm_insert_page(struct vm_area_struct *, unsigned long addr, struct page *);
 int vm_insert_pfn(struct vm_area_struct *vma, unsigned long addr,
                        unsigned long pfn);

kernel/kcov.c +2 -2

index 3ebd09e..2afaeb4 100644
@@ -293,8 +293,8 @@ static int kcov_mmap(struct file *filep, struct vm_area_struct *vma)
                spin_unlock(&kcov->lock);
                for (off = 0; off < size; off += PAGE_SIZE) {
                        page = vmalloc_to_page(kcov->area + off);
-                       if (vm_insert_page(vma, vma->vm_start + off, page))
-                               WARN_ONCE(1, "vm_insert_page() failed");
+                       if (vm_insert_kmem_page(vma, vma->vm_start + off, page))
+                               WARN_ONCE(1, "vm_insert_kmem_page() failed");
                }
                return 0;
        }

mm/memory.c +37 -0

index c467102..b800c10 100644
@@ -1682,6 +1682,75 @@ pte_t *__get_locked_pte(struct mm_struct *mm, unsigned long addr,
        return pte_alloc_map_lock(mm, pmd, addr, ptl);
 }
 
+static int insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
+               struct page *page, pgprot_t prot)
+{
+       struct mm_struct *mm = vma->vm_mm;
+       int retval;
+       pte_t *pte;
+       spinlock_t *ptl;
+
+       retval = -EINVAL;
+       if (PageAnon(page))
+               goto out;
+       retval = -ENOMEM;
+       flush_dcache_page(page);
+       pte = get_locked_pte(mm, addr, &ptl);
+       if (!pte)
+               goto out;
+       retval = -EBUSY;
+       if (!pte_none(*pte))
+               goto out_unlock;
+
+       get_page(page);
+       inc_mm_counter_fast(mm, mm_counter_file(page));
+       page_add_file_rmap(page, false);
+       set_pte_at(mm, addr, pte, mk_pte(page, prot));
+
+       retval = 0;
+       pte_unmap_unlock(pte, ptl);
+       return retval;
+out_unlock:
+       pte_unmap_unlock(pte, ptl);
+out:
+       return retval;
+}
+
+/**
+ * vm_insert_kmem_page - insert single page into user vma
+ * @vma: user vma to map to

mm/memory.c pte_t *__get_locked_pte(struct mm_struct *mm, unsigned long addr, · line 1731

Quoted 10 lines

mm/memory.c +10 -0

+ * @addr: target user address of this page
+ * @page: source kernel page
+ *
+ * This allows drivers to insert individual kernel memory into a user vma.
+ * This API should be used outside page fault handlers context.
+ *
+ * Previously the same has been done with vm_insert_page by drivers. But
+ * vm_insert_page will be converted to vmf_insert_page and will be used
+ * in fault handlers context and return type of vmf_insert_page will be
+ * vm_fault_t type.

This is a "temporal" comment, i.e. it refers to things that are happening at the moment -- I would say that should be part of the commit message, not the code, since it will be obsolete soon. Also, consider that, in a way, vm_insert_page is actually being replaced by vmf_insert_page only in one of the use cases (the other being replaced by this). Maybe you could instead say something like:

In the past, vm_insert_page was used for this purpose. Do not use
vmf_insert_page because...

and leave the full explanation in the commit.

mm/memory.c pte_t *__get_locked_pte(struct mm_struct *mm, unsigned long addr, · line 1737

Quoted 7 lines

mm/memory.c +6 -0

+ *
+ * But there are places where drivers need to map kernel memory into user
+ * vma outside fault handlers context. As vmf_insert_page will be restricted
+ * to use within page fault handlers, vm_insert_kmem_page could be used
+ * to map kernel memory to user vma outside fault handlers context.
+ */

Ditto.

Quoted 51 lines

mm/memory.c +16 -0

+int vm_insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
+                       struct page *page)
+{
+       if (addr < vma->vm_start || addr >= vma->vm_end)
+               return -EFAULT;
+       if (!page_count(page))
+               return -EINVAL;
+       if (!(vma->vm_flags & VM_MIXEDMAP)) {
+               BUG_ON(down_read_trylock(&vma->vm_mm->mmap_sem));
+               BUG_ON(vma->vm_flags & VM_PFNMAP);
+               vma->vm_flags |= VM_MIXEDMAP;
+       }
+       return insert_kmem_page(vma, addr, page, vma->vm_page_prot);
+}
+EXPORT_SYMBOL(vm_insert_kmem_page);
+
 /*
  * This is the old fallback for page remapping.
  *

mm/nommu.c +7 -0

index e4aac33..153b8c8 100644
@@ -473,6 +473,13 @@ int vm_insert_page(struct vm_area_struct *vma, unsigned long addr,
 }
 EXPORT_SYMBOL(vm_insert_page);
 
+int vm_insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
+                               struct page *page)
+{
+       return -EINVAL;
+}
+EXPORT_SYMBOL(vm_insert_kmem_page);
+
 /*
  *  sys_brk() for the most part doesn't need the global kernel
  *  lock, except when an application is doing something nasty

mm/vmalloc.c +1 -2

index a728fc4..61d279f 100644
@@ -2251,7 +2251,7 @@ int remap_vmalloc_range_partial(struct vm_area_struct *vma, unsigned long uaddr,
                struct page *page = vmalloc_to_page(kaddr);
                int ret;
 
-               ret = vm_insert_page(vma, uaddr, page);
+               ret = vm_insert_kmem_page(vma, uaddr, page);
                if (ret)
                        return ret;
 
--

Quoted 2 lines

1.9.1

Cheers,
Miguel

These are the approaches which could have been taken to handle this scenario -

* Replace vm_insert_page with vmf_insert_page and then write few extra lines of code to convert VM_FAULT_CODE to errno which makes driver users more complex ( also the reverse mapping errno to VM_FAULT_CODE have been cleaned up as part of vm_fault_t migration , not preferred to introduce anything similar again)

* Maintain both vm_insert_page and vmf_insert_page and use it in respective places. But it won't gurantee that vm_insert_page will never be used in #PF context.

* Introduce a similar API like vm_insert_page, convert all non #PF consumer to use it and finally remove vm_insert_page by converting it to vmf_insert_page.

And the 3rd approach was taken by introducing vm_insert_kmem_page().

In short, vmf_insert_page will be used in page fault handlers context and vm_insert_kmem_page will be used to map kernel memory to user vma outside page fault handlers context.

Quoted 17 lines · Souptick Joarder

These are the approaches which could have been taken to handle this scenario -

* Replace vm_insert_page with vmf_insert_page and then write few extra lines of code to convert VM_FAULT_CODE to errno which makes driver users more complex ( also the reverse mapping errno to VM_FAULT_CODE have been cleaned up as part of vm_fault_t migration , not preferred to introduce anything similar again)

* Maintain both vm_insert_page and vmf_insert_page and use it in respective places. But it won't gurantee that vm_insert_page will never be used in #PF context.

* Introduce a similar API like vm_insert_page, convert all non #PF consumer to use it and finally remove vm_insert_page by converting it to vmf_insert_page.

Quoted 5 lines

And the 3rd approach was taken by introducing vm_insert_kmem_page().

In short, vmf_insert_page will be used in page fault handlers context and vm_insert_kmem_page will be used to map kernel memory to user vma outside page fault handlers context.

As far as I can tell, vm_insert_kmem_page() is line-for-line identical with vm_insert_page(). Seriously, here's a diff I just did:

(quoted diff) +5 -6

-static int insert_page(struct vm_area_struct *vma, unsigned long addr,
-                       struct page *page, pgprot_t prot)
+static int insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
+               struct page *page, pgprot_t prot)
-       /* Ok, finally just insert the thing.. */
-int vm_insert_page(struct vm_area_struct *vma, unsigned long addr,
+int vm_insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
-       return insert_page(vma, addr, page, vma->vm_page_prot);
+       return insert_kmem_page(vma, addr, page, vma->vm_page_prot);
-EXPORT_SYMBOL(vm_insert_page);
+EXPORT_SYMBOL(vm_insert_kmem_page);

What on earth are you trying to do?

Quoted 26 lines · Matthew Wilcox

Quoted 17 lines · Souptick Joarder

These are the approaches which could have been taken to handle this scenario -

* Replace vm_insert_page with vmf_insert_page and then write few extra lines of code to convert VM_FAULT_CODE to errno which makes driver users more complex ( also the reverse mapping errno to VM_FAULT_CODE have been cleaned up as part of vm_fault_t migration , not preferred to introduce anything similar again)

* Maintain both vm_insert_page and vmf_insert_page and use it in respective places. But it won't gurantee that vm_insert_page will never be used in #PF context.

* Introduce a similar API like vm_insert_page, convert all non #PF consumer to use it and finally remove vm_insert_page by converting it to vmf_insert_page.

Quoted 5 lines

And the 3rd approach was taken by introducing vm_insert_kmem_page().

In short, vmf_insert_page will be used in page fault handlers context and vm_insert_kmem_page will be used to map kernel memory to user vma outside page fault handlers context.

As far as I can tell, vm_insert_kmem_page() is line-for-line identical with vm_insert_page(). Seriously, here's a diff I just did:

Quoted 1 line

What on earth are you trying to do?

Reading the commit log, it seems that the intention is to split out vm_insert_page() used outside of page-fault handling with the use within page-fault handling, so that different return codes can be used.

Quoted 39 lines · Matthew Wilcox

Quoted 17 lines · Souptick Joarder

These are the approaches which could have been taken to handle this scenario -

* Replace vm_insert_page with vmf_insert_page and then write few extra lines of code to convert VM_FAULT_CODE to errno which makes driver users more complex ( also the reverse mapping errno to VM_FAULT_CODE have been cleaned up as part of vm_fault_t migration , not preferred to introduce anything similar again)

* Maintain both vm_insert_page and vmf_insert_page and use it in respective places. But it won't gurantee that vm_insert_page will never be used in #PF context.

* Introduce a similar API like vm_insert_page, convert all non #PF consumer to use it and finally remove vm_insert_page by converting it to vmf_insert_page.

Quoted 5 lines

And the 3rd approach was taken by introducing vm_insert_kmem_page().

In short, vmf_insert_page will be used in page fault handlers context and vm_insert_kmem_page will be used to map kernel memory to user vma outside page fault handlers context.

As far as I can tell, vm_insert_kmem_page() is line-for-line identical with vm_insert_page(). Seriously, here's a diff I just did:

(quoted diff) +5 -6

-static int insert_page(struct vm_area_struct *vma, unsigned long addr,
-                       struct page *page, pgprot_t prot)
+static int insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
+               struct page *page, pgprot_t prot)
-       /* Ok, finally just insert the thing.. */
-int vm_insert_page(struct vm_area_struct *vma, unsigned long addr,
+int vm_insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
-       return insert_page(vma, addr, page, vma->vm_page_prot);
+       return insert_kmem_page(vma, addr, page, vma->vm_page_prot);
-EXPORT_SYMBOL(vm_insert_page);
+EXPORT_SYMBOL(vm_insert_kmem_page);

Quoted 1 line

What on earth are you trying to do?

Reading the commit log, it seems that the intention is to split out vm_insert_page() used outside of page-fault handling with the use within page-fault handling, so that different return codes can be used.

I don't see that justifies the code duplication - can't vm_insert_page() and vm_insert_kmem_page() use the same mechanics to do their job, and just translate the error code from the most- specific to the least-specific error code? Do we really need two copies of the same code just to return different error codes.

Quoted 49 lines · Russell King - ARM Linux

Quoted 39 lines · Matthew Wilcox

Quoted 17 lines · Souptick Joarder

These are the approaches which could have been taken to handle this scenario -

* Replace vm_insert_page with vmf_insert_page and then write few extra lines of code to convert VM_FAULT_CODE to errno which makes driver users more complex ( also the reverse mapping errno to VM_FAULT_CODE have been cleaned up as part of vm_fault_t migration , not preferred to introduce anything similar again)

* Maintain both vm_insert_page and vmf_insert_page and use it in respective places. But it won't gurantee that vm_insert_page will never be used in #PF context.

* Introduce a similar API like vm_insert_page, convert all non #PF consumer to use it and finally remove vm_insert_page by converting it to vmf_insert_page.

Quoted 5 lines

And the 3rd approach was taken by introducing vm_insert_kmem_page().

In short, vmf_insert_page will be used in page fault handlers context and vm_insert_kmem_page will be used to map kernel memory to user vma outside page fault handlers context.

As far as I can tell, vm_insert_kmem_page() is line-for-line identical with vm_insert_page(). Seriously, here's a diff I just did:

(quoted diff) +5 -6

-static int insert_page(struct vm_area_struct *vma, unsigned long addr,
-                       struct page *page, pgprot_t prot)
+static int insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
+               struct page *page, pgprot_t prot)
-       /* Ok, finally just insert the thing.. */
-int vm_insert_page(struct vm_area_struct *vma, unsigned long addr,
+int vm_insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
-       return insert_page(vma, addr, page, vma->vm_page_prot);
+       return insert_kmem_page(vma, addr, page, vma->vm_page_prot);
-EXPORT_SYMBOL(vm_insert_page);
+EXPORT_SYMBOL(vm_insert_kmem_page);

Quoted 1 line

What on earth are you trying to do?

Reading the commit log, it seems that the intention is to split out vm_insert_page() used outside of page-fault handling with the use within page-fault handling, so that different return codes can be used.

Quoted 5 lines

I don't see that justifies the code duplication - can't vm_insert_page() and vm_insert_kmem_page() use the same mechanics to do their job, and just translate the error code from the most- specific to the least-specific error code? Do we really need two copies of the same code just to return different error codes.

Sorry about it.
can I take below approach in a patch series ->

create a wrapper function vm_insert_kmem_page using vm_insert_page. Convert all the non #PF users to use it.
Then make vm_insert_page static and convert inline vmf_insert_page to caller.

From what I _think_ you're saying, you're trying to provide

Quoted 57 lines · Souptick Joarder

Quoted 49 lines · Russell King - ARM Linux

Quoted 39 lines · Matthew Wilcox

Quoted 17 lines · Souptick Joarder

These are the approaches which could have been taken to handle this scenario -

* Replace vm_insert_page with vmf_insert_page and then write few extra lines of code to convert VM_FAULT_CODE to errno which makes driver users more complex ( also the reverse mapping errno to VM_FAULT_CODE have been cleaned up as part of vm_fault_t migration , not preferred to introduce anything similar again)

* Maintain both vm_insert_page and vmf_insert_page and use it in respective places. But it won't gurantee that vm_insert_page will never be used in #PF context.

* Introduce a similar API like vm_insert_page, convert all non #PF consumer to use it and finally remove vm_insert_page by converting it to vmf_insert_page.

Quoted 5 lines

And the 3rd approach was taken by introducing vm_insert_kmem_page().

In short, vmf_insert_page will be used in page fault handlers context and vm_insert_kmem_page will be used to map kernel memory to user vma outside page fault handlers context.

As far as I can tell, vm_insert_kmem_page() is line-for-line identical with vm_insert_page(). Seriously, here's a diff I just did:

(quoted diff) +5 -6

-static int insert_page(struct vm_area_struct *vma, unsigned long addr,
-                       struct page *page, pgprot_t prot)
+static int insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
+               struct page *page, pgprot_t prot)
-       /* Ok, finally just insert the thing.. */
-int vm_insert_page(struct vm_area_struct *vma, unsigned long addr,
+int vm_insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
-       return insert_page(vma, addr, page, vma->vm_page_prot);
+       return insert_kmem_page(vma, addr, page, vma->vm_page_prot);
-EXPORT_SYMBOL(vm_insert_page);
+EXPORT_SYMBOL(vm_insert_kmem_page);

Quoted 1 line

What on earth are you trying to do?

Reading the commit log, it seems that the intention is to split out vm_insert_page() used outside of page-fault handling with the use within page-fault handling, so that different return codes can be used.

Quoted 5 lines

I don't see that justifies the code duplication - can't vm_insert_page() and vm_insert_kmem_page() use the same mechanics to do their job, and just translate the error code from the most- specific to the least-specific error code? Do we really need two copies of the same code just to return different error codes.

Quoted 6 lines

Sorry about it.
can I take below approach in a patch series ->

create a wrapper function vm_insert_kmem_page using vm_insert_page. Convert all the non #PF users to use it.
Then make vm_insert_page static and convert inline vmf_insert_page to caller.

I'm confused, what are you trying to do?

It seems that we already have:

vm_insert_page() - returns an errno
vmf_insert_page() - returns a VM_FAULT_* code

From what I _think_ you're saying, you're trying to provide vm_insert_kmem_page() as a direct replacement for the existing vm_insert_page(), and then make vm_insert_page() behave as per vmf_insert_page(), so we end up with:

vm_insert_kmem_page() - returns an errno vm_insert_page() - returns a VM_FAULT_* code vmf_insert_page() - returns a VM_FAULT_* code and is identical to
vm_insert_page()

Given that the documentation for vm_insert_page() says:

* Usually this function is called from f_op->mmap() handler
* under mm->mmap_sem write-lock, so it can change vma->vm_flags.
* Caller must set VM_MIXEDMAP on vma if it wants to call this
* function from other places, for example from page-fault handler.

this says that the "usual" use method for vm_insert_page() is _outside_ of page fault handling - if it is used _inside_ page fault handling, then it states that additional fixups are required on the VMA. So I don't get why your patch commentry seems to be saying that users of vm_insert_page() outside of page fault handling all need to be patched - isn't this the use case that this function is defined to be handling?

If you're going to be changing the semantics, doesn't this create a flag day where we could get new users of vm_insert_page() using the _existing_ semantics merged after you've changed its semantics (eg, the return code)?

Quoted 11 lines · Russell King - ARM Linux

I'm confused, what are you trying to do?

It seems that we already have:

vm_insert_page() - returns an errno
vmf_insert_page() - returns a VM_FAULT_* code

From what I _think_ you're saying, you're trying to provide vm_insert_kmem_page() as a direct replacement for the existing vm_insert_page(), and then make vm_insert_page() behave as per vmf_insert_page(), so we end up with:

yes, vm_insert_kmem_page() can be a direct replacement of vm_insert_page or might be a wrapper function written using vm_insert_page whichever suites better
based on feedback.

Quoted 6 lines

vm_insert_kmem_page() - returns an errno vm_insert_page() - returns a VM_FAULT_* code vmf_insert_page() - returns a VM_FAULT_* code and is identical to
vm_insert_page()

After completion of conversion we end up with

vm_insert_kmem_page() - returns an errno
vmf_insert_page() - returns a VM_FAULT_* code

Quoted 7 lines

Given that the documentation for vm_insert_page() says:

* Usually this function is called from f_op->mmap() handler
* under mm->mmap_sem write-lock, so it can change vma->vm_flags.
* Caller must set VM_MIXEDMAP on vma if it wants to call this
* function from other places, for example from page-fault handler.

Quoted 7 lines

this says that the "usual" use method for vm_insert_page() is _outside_ of page fault handling - if it is used _inside_ page fault handling, then it states that additional fixups are required on the VMA. So I don't get why your patch commentry seems to be saying that users of vm_insert_page() outside of page fault handling all need to be patched - isn't this the use case that this function is defined to be handling?

The answer is yes best of my knowledge.

But as mentioned in change log ->

Going forward, the plan is to restrict future drivers not to use vm_insert_page ( *it will generate new errno to VM_FAULT_CODE mapping code for new drivers which were already cleaned up for existing drivers*) in #PF (page fault handler) context but to make use of vmf_insert_page which returns VMF_FAULT_CODE and that is not possible until both vm_insert_page and vmf_insert_page API exists.

But there are some consumers of vm_insert_page which use it outside #PF context. straight forward conversion of vm_insert_page to vmf_insert_page won't work there as those function calls expects errno not vm_fault_t in return.

If both {vm, vmf}_insert_page exists, vm_insert_page might be used for #PF context which we want to protect by removing/ replacing vm_insert_page with another similar/ wrapper API.

Is that the right answer of your question ? no ?

Quoted 64 lines · Souptick Joarder

Quoted 11 lines · Russell King - ARM Linux

I'm confused, what are you trying to do?

It seems that we already have:

vm_insert_page() - returns an errno
vmf_insert_page() - returns a VM_FAULT_* code

From what I _think_ you're saying, you're trying to provide vm_insert_kmem_page() as a direct replacement for the existing vm_insert_page(), and then make vm_insert_page() behave as per vmf_insert_page(), so we end up with:

yes, vm_insert_kmem_page() can be a direct replacement of vm_insert_page or might be a wrapper function written using vm_insert_page whichever suites better
based on feedback.

Quoted 6 lines

vm_insert_kmem_page() - returns an errno vm_insert_page() - returns a VM_FAULT_* code vmf_insert_page() - returns a VM_FAULT_* code and is identical to
vm_insert_page()

After completion of conversion we end up with

vm_insert_kmem_page() - returns an errno
vmf_insert_page() - returns a VM_FAULT_* code

Quoted 7 lines

Given that the documentation for vm_insert_page() says:

* Usually this function is called from f_op->mmap() handler
* under mm->mmap_sem write-lock, so it can change vma->vm_flags.
* Caller must set VM_MIXEDMAP on vma if it wants to call this
* function from other places, for example from page-fault handler.

Quoted 7 lines

this says that the "usual" use method for vm_insert_page() is _outside_ of page fault handling - if it is used _inside_ page fault handling, then it states that additional fixups are required on the VMA. So I don't get why your patch commentry seems to be saying that users of vm_insert_page() outside of page fault handling all need to be patched - isn't this the use case that this function is defined to be handling?

The answer is yes best of my knowledge.

But as mentioned in change log ->

Going forward, the plan is to restrict future drivers not to use vm_insert_page ( *it will generate new errno to VM_FAULT_CODE mapping code for new drivers which were already cleaned up for existing drivers*) in #PF (page fault handler) context but to make use of vmf_insert_page which returns VMF_FAULT_CODE and that is not possible until both vm_insert_page and vmf_insert_page API exists.

But there are some consumers of vm_insert_page which use it outside #PF context. straight forward conversion of vm_insert_page to vmf_insert_page won't work there as those function calls expects errno not vm_fault_t in return.

Quoted 5 lines

If both {vm, vmf}_insert_page exists, vm_insert_page might be used for #PF context which we want to protect by removing/ replacing vm_insert_page with another similar/ wrapper API.

Is that the right answer of your question ? no ?

I think this is a bad plan. What we should rather do is examine the current users of vm_insert_page() and ask "What interface would better replace vm_insert_page()?"

As I've said to you before, I believe the right answer is to have a vm_insert_range() which takes an array of struct page pointers. That fits the majority of remaining users.

----

If we do want to rename vm_insert_page() to vm_insert_kmem_page(), then the right answer is to _just do that_. Not duplicate vm_insert_page() in its entirety. I don't see the point to doing that.

Quoted 5 lines · Matthew Wilcox

I think this is a bad plan. What we should rather do is examine the current users of vm_insert_page() and ask "What interface would better replace vm_insert_page()?"

Quoted 3 lines

As I've said to you before, I believe the right answer is to have a vm_insert_range() which takes an array of struct page pointers. That fits the majority of remaining users.

Ok, but it will take some time.
Is it a good idea to introduce the final vm_fault_t patch and then start working on vm_insert_range as it will be bit time consuming ?

Hi Souptick,

Quoted 11 lines · Souptick Joarder

Quoted 8 lines · Matthew Wilcox

I think this is a bad plan. What we should rather do is examine the current users of vm_insert_page() and ask "What interface would better replace vm_insert_page()?"

As I've said to you before, I believe the right answer is to have a vm_insert_range() which takes an array of struct page pointers. That fits the majority of remaining users.

Quoted 4 lines

Ok, but it will take some time.
Is it a good idea to introduce the final vm_fault_t patch and then start working on vm_insert_range as it will be bit time consuming ?

Well, why is there a rush? Development should be done in a patch series or a tree, and submitted as a whole, instead of sending partial patches.

Also, not sure if you saw my comments/review: if the interface is not going to change, why the name change? Why can't we simply keep using vm_insert_page?

Quoted 5 lines · Miguel Ojeda

Also, not sure if you saw my comments/review: if the interface is not going to change, why the name change? Why can't we simply keep using vm_insert_page?

yes, changing the name without changing the interface is a bad approach and this can't be taken. As Matthew mentioned, "vm_insert_range() which takes an array of struct page pointers. That fits the majority of remaining users" would be a better approach to fit this use case.

But yes, we can't keep vm_insert_page and vmf_insert_page together as it doesn't guarantee that future drivers will not use vm_insert_page in #PF context ( which will generate new errno to VM_FAULT_CODE).

Hi Souptick,

Quoted 15 lines · Souptick Joarder

Quoted 5 lines · Miguel Ojeda

Also, not sure if you saw my comments/review: if the interface is not going to change, why the name change? Why can't we simply keep using vm_insert_page?

yes, changing the name without changing the interface is a bad approach and this can't be taken. As Matthew mentioned, "vm_insert_range() which takes an array of struct page pointers. That fits the majority of remaining users" would be a better approach to fit this use case.

Quoted 4 lines

But yes, we can't keep vm_insert_page and vmf_insert_page together as it doesn't guarantee that future drivers will not use vm_insert_page in #PF context ( which will generate new errno to VM_FAULT_CODE).

Maybe I am hard of thinking, but aren't you planning to remove vm_insert_page with these changes? If yes, why you can't use the keep vm_insert_page name? In other words, keep returning what the drivers expect?

The final goal is to remove vm_insert_page by converting it to vmf_insert_page. But to do that we have to first introduce the new API which is similar to vm_insert_page (for non #PF). I tried this by introducing vm_insert_kmem_page ( * identical as vm_insert_page except API name *) in this patch. But this looks like a bad approach.

Hi Souptick,

Quoted 6 lines · Souptick Joarder

The final goal is to remove vm_insert_page by converting it to vmf_insert_page. But to do that we have to first introduce the new API which is similar to vm_insert_page (for non #PF). I tried this by introducing vm_insert_kmem_page ( * identical as vm_insert_page except API name *) in this patch. But this looks like a bad approach.

We are going in circles here. That you want to convert vm_insert_page to vmf_insert_page for the PF case is fine and understood. However, you don't *need* to introduce a new name for the remaining non-PF cases if the function is going to be the exact same thing as before. You say "The final goal is to remove vm_insert_page", but you haven't justified *why* you need to remove that name.

Now, if we want to rename the function for some reason (e.g. avoid confusion with vmf_insert_page), that is fine but is another topic. It may be or not a good idea, but it is orthogonal to the vmf_ work. Matthew, on this regard, told you that you shouldn't duplicate functions. If you want a rename, do so; but don't copy the code. In other words: nobody said introducing the vm_insert_kmem_page name is a bad idea -- what Matthew told you is that *duplicating* vm_insert_page just for that is bad.

Further, you are copying the code (if I understand your thought process) because you want to change the callers of non-PF first, and then do the "full conversion from vm_* to vmf_*". However, that is confusing, because there is no need to change non-PF callers of vm_insert_page since they don't care about the new vmf_* functions.

Instead, the proper way of doing this is:

1. Introduce the vmf_* API
2. Change all PF-users users to that (leaving all non-PF ones
untouched!) -- if this is too big, you can split this patch into several patches, one per subsystem, etc.

3. Remove the vm_* functions (except the ones that are still used in
non-PF contexts, e.g. vm_insert_page)

Then, optionally, if you want to rename the function for the remaining non-PF users:

4. Rename vm_insert_page (justifying why the current name is
confusing *on its own merits*).

Otherwise, if you want to pursue Matthew's idea:

4. Introduce the vm_insert_range (possibly leveraging
vm_insert_page, or not; you have to see what is best).
5. Replace those callers that can take advantage of vm_insert_range
6. Remove vm_insert_page and replace callers with vm_insert_range
(only if it is not worth to keep vm_insert_range, again justifying it *on its own merits*)

As you see, these are all logical step-by-step improvements, without duplicating functions temporarily, leaving temporary changes or changing current callers to new APIs for unrelated reasons (i.e. no need to introduce vm_insert_kmem_page simply to do a "conversion" to vmf_).

Cheers,
Miguel

Quoted 5 lines · Miguel Ojeda

1. Introduce the vmf_* API
2. Change all PF-users users to that (leaving all non-PF ones
untouched!) -- if this is too big, you can split this patch into several patches, one per subsystem, etc.

We are done with step 2. All the PF-users are converted to use vmf_insert_page. ( Ref - linux-next-20181005)

Quoted 9 lines

Otherwise, if you want to pursue Matthew's idea:

4. Introduce the vm_insert_range (possibly leveraging
vm_insert_page, or not; you have to see what is best).
5. Replace those callers that can take advantage of vm_insert_range
6. Remove vm_insert_page and replace callers with vm_insert_range
(only if it is not worth to keep vm_insert_range, again justifying it *on its own merits*)

Step 4 to 6, going to do it. It is part of plan now :-)

Quoted 9 lines · Souptick Joarder

Quoted 5 lines · Miguel Ojeda

1. Introduce the vmf_* API
2. Change all PF-users users to that (leaving all non-PF ones
untouched!) -- if this is too big, you can split this patch into several patches, one per subsystem, etc.

Quoted 2 lines

We are done with step 2. All the PF-users are converted to use vmf_insert_page. ( Ref - linux-next-20181005)

They are not supposed to be "steps". You did it with 70+ commits (!!) over the course of several months. Why a tree wasn't created, stuff developed there, and when done, submitted it for review?

Quoted 11 lines

Quoted 10 lines

Otherwise, if you want to pursue Matthew's idea:

4. Introduce the vm_insert_range (possibly leveraging
vm_insert_page, or not; you have to see what is best).
5. Replace those callers that can take advantage of vm_insert_range
6. Remove vm_insert_page and replace callers with vm_insert_range
(only if it is not worth to keep vm_insert_range, again justifying it *on its own merits*)

Quoted 2 lines

Step 4 to 6, going to do it. It is part of plan now :-)

Fine, but you haven't answered to the other parts of my email: you don't explain why you choose one alternative over the others, you simply keep changing the approach.

Quoted 3 lines · Miguel Ojeda

They are not supposed to be "steps". You did it with 70+ commits (!!) over the course of several months. Why a tree wasn't created, stuff developed there, and when done, submitted it for review?

Because we already have a plan for entire vm_fault_t migration and the * instruction * was to send one patch per driver.

Quoted 4 lines

Fine, but you haven't answered to the other parts of my email: you don't explain why you choose one alternative over the others, you simply keep changing the approach.

We are going in circles here. That you want to convert vm_insert_page to vmf_insert_page for the PF case is fine and understood. However, you don't *need* to introduce a new name for the remaining non-PF cases if the function is going to be the exact same thing as before. You say "The final goal is to remove vm_insert_page", but you haven't justified *why* you need to remove that name.

I think I have given that answer. If we don't remove vm_insert_page, future #PF caller will have option to use it. But those should be restricted. How are we going to restrict vm_insert_page in one half of kernel when other half is still using it ?? Is there any way ? ( I don't know)

Quoted 3 lines · Souptick Joarder

On Fri, Oct 5, 2018 at 11:39 PM Miguel Ojeda
<[email protected]> wrote:

Quoted 6 lines

Quoted 3 lines

They are not supposed to be "steps". You did it with 70+ commits (!!) over the course of several months. Why a tree wasn't created, stuff developed there, and when done, submitted it for review?

Because we already have a plan for entire vm_fault_t migration and the * instruction * was to send one patch per driver.

The instruction?

Quoted 13 lines

Quoted 5 lines

Fine, but you haven't answered to the other parts of my email: you don't explain why you choose one alternative over the others, you simply keep changing the approach.

We are going in circles here. That you want to convert vm_insert_page to vmf_insert_page for the PF case is fine and understood. However, you don't *need* to introduce a new name for the remaining non-PF cases if the function is going to be the exact same thing as before. You say "The final goal is to remove vm_insert_page", but you haven't justified *why* you need to remove that name.

Quoted 5 lines

I think I have given that answer. If we don't remove vm_insert_page, future #PF caller will have option to use it. But those should be restricted. How are we going to restrict vm_insert_page in one half of kernel when other half is still using it ?? Is there any way ? ( I don't know)

Ah, so that is what you are concerned about: future misuses. Well, I don't really see the problem. There are only ~18 calls to vm_insert_page() in the entire kernel: checking if people is using it properly for a while should be easy. As long as the new behavior is documented properly, it should be fine. If you are really concerned about mistakes being made, then fine, we can rename it as I suggested.

Now, the new vm_insert_range() is another topic. It simplifies a few of the callers and buys us the rename at the same time, so I am also OK with it.

As you see, I am not against the changes -- it is just that they should clearly justified. :-) It wasn't clear what your problem with the current vm_insert_page() is.

Cheers,
Miguel

Quoted 9 lines · Miguel Ojeda

Quoted 7 lines · Souptick Joarder

Quoted 3 lines · Miguel Ojeda

They are not supposed to be "steps". You did it with 70+ commits (!!) over the course of several months. Why a tree wasn't created, stuff developed there, and when done, submitted it for review?

Quoted 4 lines

Quoted 2 lines

Because we already have a plan for entire vm_fault_t migration and the * instruction * was to send one patch per driver.

The instruction?

Sorry for the delayed response.
Instruction from Matthew Wilcox who is supervising the entire vm_fault_t migration work :-)

Quoted 16 lines · Souptick Joarder

Quoted 13 lines · Miguel Ojeda

Quoted 9 lines · Souptick Joarder

Quoted 3 lines · Miguel Ojeda

They are not supposed to be "steps". You did it with 70+ commits (!!) over the course of several months. Why a tree wasn't created, stuff developed there, and when done, submitted it for review?

Because we already have a plan for entire vm_fault_t migration and the * instruction * was to send one patch per driver.

The instruction?

Quoted 3 lines

Sorry for the delayed response.
Instruction from Matthew Wilcox who is supervising the entire vm_fault_t migration work :-)

Hang on. That was for the initial vm_fault_t conversion in which each step was clearly an improvement. What you're looking at now is far from that.

Quoted 4 lines · Matthew Wilcox

Quoted 2 lines · Souptick Joarder

Instruction from Matthew Wilcox who is supervising the entire vm_fault_t migration work :-)

Quoted 3 lines

Hang on. That was for the initial vm_fault_t conversion in which each step was clearly an improvement. What you're looking at now is far from that.

Ok. But my understanding was, the approach of vm_insert_range comes into discussion as part of converting vm_insert_page into vmf_insert_page which is still part of original vm_fault_t conversion discussion. No ?

Quoted 9 lines · Souptick Joarder

Quoted 7 lines · Matthew Wilcox

Quoted 2 lines · Souptick Joarder

Instruction from Matthew Wilcox who is supervising the entire vm_fault_t migration work :-)

Hang on. That was for the initial vm_fault_t conversion in which each step was clearly an improvement. What you're looking at now is far from that.

Quoted 3 lines

Ok. But my understanding was, the approach of vm_insert_range comes into discussion as part of converting vm_insert_page into vmf_insert_page which is still part of original vm_fault_t conversion discussion. No ?

No. The initial part (converting all page fault methods to vm_fault_t) is done. What remains undone (looking at akpm's tree) is changing the typedef of vm_fault_t from int to unsigned int. That will prevent new page fault handlers with the wrong type from being added.

I don't necessarily want to get rid of vm_insert_page(). Maybe it will make sense to do that, and maybe not. What I do want to see is thought, and not "Matthew told me to do it", when I didn't.

Quoted 17 lines · Souptick Joarder

These are the approaches which could have been taken to handle this scenario -

* Replace vm_insert_page with vmf_insert_page and then write few extra lines of code to convert VM_FAULT_CODE to errno which makes driver users more complex ( also the reverse mapping errno to VM_FAULT_CODE have been cleaned up as part of vm_fault_t migration , not preferred to introduce anything similar again)

* Maintain both vm_insert_page and vmf_insert_page and use it in respective places. But it won't gurantee that vm_insert_page will never be used in #PF context.

* Introduce a similar API like vm_insert_page, convert all non #PF consumer to use it and finally remove vm_insert_page by converting it to vmf_insert_page.

Quoted 5 lines

And the 3rd approach was taken by introducing vm_insert_kmem_page().

In short, vmf_insert_page will be used in page fault handlers context and vm_insert_kmem_page will be used to map kernel memory to user vma outside page fault handlers context.

As far as I can tell, vm_insert_kmem_page() is line-for-line identical with vm_insert_page(). Seriously, here's a diff I just did:

(quoted diff) +5 -6

-static int insert_page(struct vm_area_struct *vma, unsigned long addr,
-                       struct page *page, pgprot_t prot)
+static int insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
+               struct page *page, pgprot_t prot)
-       /* Ok, finally just insert the thing.. */
-int vm_insert_page(struct vm_area_struct *vma, unsigned long addr,
+int vm_insert_kmem_page(struct vm_area_struct *vma, unsigned long addr,
-       return insert_page(vma, addr, page, vma->vm_page_prot);
+       return insert_kmem_page(vma, addr, page, vma->vm_page_prot);
-EXPORT_SYMBOL(vm_insert_page);
+EXPORT_SYMBOL(vm_insert_kmem_page);

What on earth are you trying to do?