[RFC,RESEND,2/3] mm: Add an apply_to_pfn_range interface
diff mbox series

Message ID 20190321132140.114878-3-thellstrom@vmware.com
State New
Headers show
Series
  • mm modifications / helpers for emulated GPU coherent memory
Related show

Commit Message

Thomas Hellstrom March 21, 2019, 1:22 p.m. UTC
This is basically apply_to_page_range with added functionality:
Allocating missing parts of the page table becomes optional, which
means that the function can be guaranteed not to error if allocation
is disabled. Also passing of the closure struct and callback function
becomes different and more in line with how things are done elsewhere.

Finally we keep apply_to_page_range as a wrapper around apply_to_pfn_range

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Matthew Wilcox <willy@infradead.org>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Rik van Riel <riel@surriel.com>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Michal Hocko <mhocko@suse.com>
Cc: Huang Ying <ying.huang@intel.com>
Cc: Souptick Joarder <jrdr.linux@gmail.com>
Cc: "Jérôme Glisse" <jglisse@redhat.com>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
---
 include/linux/mm.h |  10 ++++
 mm/memory.c        | 121 +++++++++++++++++++++++++++++++++------------
 2 files changed, 99 insertions(+), 32 deletions(-)

Comments

Jerome Glisse March 21, 2019, 1:52 p.m. UTC | #1
On Thu, Mar 21, 2019 at 01:22:35PM +0000, Thomas Hellstrom wrote:
> This is basically apply_to_page_range with added functionality:
> Allocating missing parts of the page table becomes optional, which
> means that the function can be guaranteed not to error if allocation
> is disabled. Also passing of the closure struct and callback function
> becomes different and more in line with how things are done elsewhere.
> 
> Finally we keep apply_to_page_range as a wrapper around apply_to_pfn_range

The apply_to_page_range() is dangerous API it does not follow other
mm patterns like mmu notifier. It is suppose to be use in arch code
or vmalloc or similar thing but not in regular driver code. I see
it has crept out of this and is being use by few device driver. I am
not sure we should encourage that.

> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> Cc: Will Deacon <will.deacon@arm.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Rik van Riel <riel@surriel.com>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Michal Hocko <mhocko@suse.com>
> Cc: Huang Ying <ying.huang@intel.com>
> Cc: Souptick Joarder <jrdr.linux@gmail.com>
> Cc: "Jérôme Glisse" <jglisse@redhat.com>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> ---
>  include/linux/mm.h |  10 ++++
>  mm/memory.c        | 121 +++++++++++++++++++++++++++++++++------------
>  2 files changed, 99 insertions(+), 32 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 80bb6408fe73..b7dd4ddd6efb 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -2632,6 +2632,16 @@ typedef int (*pte_fn_t)(pte_t *pte, pgtable_t token, unsigned long addr,
>  extern int apply_to_page_range(struct mm_struct *mm, unsigned long address,
>  			       unsigned long size, pte_fn_t fn, void *data);
>  
> +struct pfn_range_apply;
> +typedef int (*pter_fn_t)(pte_t *pte, pgtable_t token, unsigned long addr,
> +			 struct pfn_range_apply *closure);
> +struct pfn_range_apply {
> +	struct mm_struct *mm;
> +	pter_fn_t ptefn;
> +	unsigned int alloc;
> +};
> +extern int apply_to_pfn_range(struct pfn_range_apply *closure,
> +			      unsigned long address, unsigned long size);
>  
>  #ifdef CONFIG_PAGE_POISONING
>  extern bool page_poisoning_enabled(void);
> diff --git a/mm/memory.c b/mm/memory.c
> index dcd80313cf10..0feb7191c2d2 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1938,18 +1938,17 @@ int vm_iomap_memory(struct vm_area_struct *vma, phys_addr_t start, unsigned long
>  }
>  EXPORT_SYMBOL(vm_iomap_memory);
>  
> -static int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd,
> -				     unsigned long addr, unsigned long end,
> -				     pte_fn_t fn, void *data)
> +static int apply_to_pte_range(struct pfn_range_apply *closure, pmd_t *pmd,
> +			      unsigned long addr, unsigned long end)
>  {
>  	pte_t *pte;
>  	int err;
>  	pgtable_t token;
>  	spinlock_t *uninitialized_var(ptl);
>  
> -	pte = (mm == &init_mm) ?
> +	pte = (closure->mm == &init_mm) ?
>  		pte_alloc_kernel(pmd, addr) :
> -		pte_alloc_map_lock(mm, pmd, addr, &ptl);
> +		pte_alloc_map_lock(closure->mm, pmd, addr, &ptl);
>  	if (!pte)
>  		return -ENOMEM;
>  
> @@ -1960,86 +1959,103 @@ static int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd,
>  	token = pmd_pgtable(*pmd);
>  
>  	do {
> -		err = fn(pte++, token, addr, data);
> +		err = closure->ptefn(pte++, token, addr, closure);
>  		if (err)
>  			break;
>  	} while (addr += PAGE_SIZE, addr != end);
>  
>  	arch_leave_lazy_mmu_mode();
>  
> -	if (mm != &init_mm)
> +	if (closure->mm != &init_mm)
>  		pte_unmap_unlock(pte-1, ptl);
>  	return err;
>  }
>  
> -static int apply_to_pmd_range(struct mm_struct *mm, pud_t *pud,
> -				     unsigned long addr, unsigned long end,
> -				     pte_fn_t fn, void *data)
> +static int apply_to_pmd_range(struct pfn_range_apply *closure, pud_t *pud,
> +			      unsigned long addr, unsigned long end)
>  {
>  	pmd_t *pmd;
>  	unsigned long next;
> -	int err;
> +	int err = 0;
>  
>  	BUG_ON(pud_huge(*pud));
>  
> -	pmd = pmd_alloc(mm, pud, addr);
> +	pmd = pmd_alloc(closure->mm, pud, addr);
>  	if (!pmd)
>  		return -ENOMEM;
> +
>  	do {
>  		next = pmd_addr_end(addr, end);
> -		err = apply_to_pte_range(mm, pmd, addr, next, fn, data);
> +		if (!closure->alloc && pmd_none_or_clear_bad(pmd))
> +			continue;
> +		err = apply_to_pte_range(closure, pmd, addr, next);
>  		if (err)
>  			break;
>  	} while (pmd++, addr = next, addr != end);
>  	return err;
>  }
>  
> -static int apply_to_pud_range(struct mm_struct *mm, p4d_t *p4d,
> -				     unsigned long addr, unsigned long end,
> -				     pte_fn_t fn, void *data)
> +static int apply_to_pud_range(struct pfn_range_apply *closure, p4d_t *p4d,
> +			      unsigned long addr, unsigned long end)
>  {
>  	pud_t *pud;
>  	unsigned long next;
> -	int err;
> +	int err = 0;
>  
> -	pud = pud_alloc(mm, p4d, addr);
> +	pud = pud_alloc(closure->mm, p4d, addr);
>  	if (!pud)
>  		return -ENOMEM;
> +
>  	do {
>  		next = pud_addr_end(addr, end);
> -		err = apply_to_pmd_range(mm, pud, addr, next, fn, data);
> +		if (!closure->alloc && pud_none_or_clear_bad(pud))
> +			continue;
> +		err = apply_to_pmd_range(closure, pud, addr, next);
>  		if (err)
>  			break;
>  	} while (pud++, addr = next, addr != end);
>  	return err;
>  }
>  
> -static int apply_to_p4d_range(struct mm_struct *mm, pgd_t *pgd,
> -				     unsigned long addr, unsigned long end,
> -				     pte_fn_t fn, void *data)
> +static int apply_to_p4d_range(struct pfn_range_apply *closure, pgd_t *pgd,
> +			      unsigned long addr, unsigned long end)
>  {
>  	p4d_t *p4d;
>  	unsigned long next;
> -	int err;
> +	int err = 0;
>  
> -	p4d = p4d_alloc(mm, pgd, addr);
> +	p4d = p4d_alloc(closure->mm, pgd, addr);
>  	if (!p4d)
>  		return -ENOMEM;
> +
>  	do {
>  		next = p4d_addr_end(addr, end);
> -		err = apply_to_pud_range(mm, p4d, addr, next, fn, data);
> +		if (!closure->alloc && p4d_none_or_clear_bad(p4d))
> +			continue;
> +		err = apply_to_pud_range(closure, p4d, addr, next);
>  		if (err)
>  			break;
>  	} while (p4d++, addr = next, addr != end);
>  	return err;
>  }
>  
> -/*
> - * Scan a region of virtual memory, filling in page tables as necessary
> - * and calling a provided function on each leaf page table.
> +/**
> + * apply_to_pfn_range - Scan a region of virtual memory, calling a provided
> + * function on each leaf page table entry
> + * @closure: Details about how to scan and what function to apply
> + * @addr: Start virtual address
> + * @size: Size of the region
> + *
> + * If @closure->alloc is set to 1, the function will fill in the page table
> + * as necessary. Otherwise it will skip non-present parts.
> + *
> + * Returns: Zero on success. If the provided function returns a non-zero status,
> + * the page table walk will terminate and that status will be returned.
> + * If @closure->alloc is set to 1, then this function may also return memory
> + * allocation errors arising from allocating page table memory.
>   */
> -int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
> -			unsigned long size, pte_fn_t fn, void *data)
> +int apply_to_pfn_range(struct pfn_range_apply *closure,
> +		       unsigned long addr, unsigned long size)
>  {
>  	pgd_t *pgd;
>  	unsigned long next;
> @@ -2049,16 +2065,57 @@ int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
>  	if (WARN_ON(addr >= end))
>  		return -EINVAL;
>  
> -	pgd = pgd_offset(mm, addr);
> +	pgd = pgd_offset(closure->mm, addr);
>  	do {
>  		next = pgd_addr_end(addr, end);
> -		err = apply_to_p4d_range(mm, pgd, addr, next, fn, data);
> +		if (!closure->alloc && pgd_none_or_clear_bad(pgd))
> +			continue;
> +		err = apply_to_p4d_range(closure, pgd, addr, next);
>  		if (err)
>  			break;
>  	} while (pgd++, addr = next, addr != end);
>  
>  	return err;
>  }
> +EXPORT_SYMBOL_GPL(apply_to_pfn_range);
> +
> +struct page_range_apply {
> +	struct pfn_range_apply pter;
> +	pte_fn_t fn;
> +	void *data;
> +};
> +
> +/*
> + * Callback wrapper to enable use of apply_to_pfn_range for
> + * the apply_to_page_range interface
> + */
> +static int apply_to_page_range_wrapper(pte_t *pte, pgtable_t token,
> +				       unsigned long addr,
> +				       struct pfn_range_apply *pter)
> +{
> +	struct page_range_apply *pra =
> +		container_of(pter, typeof(*pra), pter);
> +
> +	return pra->fn(pte, token, addr, pra->data);
> +}
> +
> +/*
> + * Scan a region of virtual memory, filling in page tables as necessary
> + * and calling a provided function on each leaf page table.
> + */
> +int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
> +			unsigned long size, pte_fn_t fn, void *data)
> +{
> +	struct page_range_apply pra = {
> +		.pter = {.mm = mm,
> +			 .alloc = 1,
> +			 .ptefn = apply_to_page_range_wrapper },
> +		.fn = fn,
> +		.data = data
> +	};
> +
> +	return apply_to_pfn_range(&pra.pter, addr, size);
> +}
>  EXPORT_SYMBOL_GPL(apply_to_page_range);
>  
>  /*
> -- 
> 2.19.0.rc1
>
Thomas Hellstrom March 21, 2019, 7:59 p.m. UTC | #2
On Thu, 2019-03-21 at 09:52 -0400, Jerome Glisse wrote:
> On Thu, Mar 21, 2019 at 01:22:35PM +0000, Thomas Hellstrom wrote:
> > This is basically apply_to_page_range with added functionality:
> > Allocating missing parts of the page table becomes optional, which
> > means that the function can be guaranteed not to error if
> > allocation
> > is disabled. Also passing of the closure struct and callback
> > function
> > becomes different and more in line with how things are done
> > elsewhere.
> > 
> > Finally we keep apply_to_page_range as a wrapper around
> > apply_to_pfn_range
> 
> The apply_to_page_range() is dangerous API it does not follow other
> mm patterns like mmu notifier. It is suppose to be use in arch code
> or vmalloc or similar thing but not in regular driver code. I see
> it has crept out of this and is being use by few device driver. I am
> not sure we should encourage that.

I can certainly remove the EXPORT of the new apply_to_pfn_range() which
will make sure its use stays within the mm code. I don't expect any
additional usage except for the two address-space utilities.

I'm looking for examples to see how it could be more in line with the
rest of the mm code. The main difference from the pattern in, for
example, page_mkclean() seems to be that it's lacking the
mmu_notifier_invalidate_start() and mmu_notifier_invalidate_end()?
Perhaps the intention is to have the pte leaf functions notify on pte
updates? How does this relate to arch_enter_lazy_mmu() which is called
outside of the page table locks? The documentation appears a bit
scarce...

> 
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Cc: Matthew Wilcox <willy@infradead.org>
> > Cc: Will Deacon <will.deacon@arm.com>
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Cc: Rik van Riel <riel@surriel.com>
> > Cc: Minchan Kim <minchan@kernel.org>
> > Cc: Michal Hocko <mhocko@suse.com>
> > Cc: Huang Ying <ying.huang@intel.com>
> > Cc: Souptick Joarder <jrdr.linux@gmail.com>
> > Cc: "Jérôme Glisse" <jglisse@redhat.com>
> > Cc: linux-mm@kvack.org
> > Cc: linux-kernel@vger.kernel.org
> > Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
> > ---
> >  include/linux/mm.h |  10 ++++
> >  mm/memory.c        | 121 +++++++++++++++++++++++++++++++++------
> > ------
> >  2 files changed, 99 insertions(+), 32 deletions(-)
> > 
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 80bb6408fe73..b7dd4ddd6efb 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -2632,6 +2632,16 @@ typedef int (*pte_fn_t)(pte_t *pte,
> > pgtable_t token, unsigned long addr,
> >  extern int apply_to_page_range(struct mm_struct *mm, unsigned long
> > address,
> >  			       unsigned long size, pte_fn_t fn, void
> > *data);
> >  
> > +struct pfn_range_apply;
> > +typedef int (*pter_fn_t)(pte_t *pte, pgtable_t token, unsigned
> > long addr,
> > +			 struct pfn_range_apply *closure);
> > +struct pfn_range_apply {
> > +	struct mm_struct *mm;
> > +	pter_fn_t ptefn;
> > +	unsigned int alloc;
> > +};
> > +extern int apply_to_pfn_range(struct pfn_range_apply *closure,
> > +			      unsigned long address, unsigned long
> > size);
> >  
> >  #ifdef CONFIG_PAGE_POISONING
> >  extern bool page_poisoning_enabled(void);
> > diff --git a/mm/memory.c b/mm/memory.c
> > index dcd80313cf10..0feb7191c2d2 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -1938,18 +1938,17 @@ int vm_iomap_memory(struct vm_area_struct
> > *vma, phys_addr_t start, unsigned long
> >  }
> >  EXPORT_SYMBOL(vm_iomap_memory);
> >  
> > -static int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd,
> > -				     unsigned long addr, unsigned long
> > end,
> > -				     pte_fn_t fn, void *data)
> > +static int apply_to_pte_range(struct pfn_range_apply *closure,
> > pmd_t *pmd,
> > +			      unsigned long addr, unsigned long end)
> >  {
> >  	pte_t *pte;
> >  	int err;
> >  	pgtable_t token;
> >  	spinlock_t *uninitialized_var(ptl);
> >  
> > -	pte = (mm == &init_mm) ?
> > +	pte = (closure->mm == &init_mm) ?
> >  		pte_alloc_kernel(pmd, addr) :
> > -		pte_alloc_map_lock(mm, pmd, addr, &ptl);
> > +		pte_alloc_map_lock(closure->mm, pmd, addr, &ptl);
> >  	if (!pte)
> >  		return -ENOMEM;
> >  
> > @@ -1960,86 +1959,103 @@ static int apply_to_pte_range(struct
> > mm_struct *mm, pmd_t *pmd,
> >  	token = pmd_pgtable(*pmd);
> >  
> >  	do {
> > -		err = fn(pte++, token, addr, data);
> > +		err = closure->ptefn(pte++, token, addr, closure);
> >  		if (err)
> >  			break;
> >  	} while (addr += PAGE_SIZE, addr != end);
> >  
> >  	arch_leave_lazy_mmu_mode();
> >  
> > -	if (mm != &init_mm)
> > +	if (closure->mm != &init_mm)
> >  		pte_unmap_unlock(pte-1, ptl);
> >  	return err;
> >  }
> >  
> > -static int apply_to_pmd_range(struct mm_struct *mm, pud_t *pud,
> > -				     unsigned long addr, unsigned long
> > end,
> > -				     pte_fn_t fn, void *data)
> > +static int apply_to_pmd_range(struct pfn_range_apply *closure,
> > pud_t *pud,
> > +			      unsigned long addr, unsigned long end)
> >  {
> >  	pmd_t *pmd;
> >  	unsigned long next;
> > -	int err;
> > +	int err = 0;
> >  
> >  	BUG_ON(pud_huge(*pud));
> >  
> > -	pmd = pmd_alloc(mm, pud, addr);
> > +	pmd = pmd_alloc(closure->mm, pud, addr);
> >  	if (!pmd)
> >  		return -ENOMEM;
> > +
> >  	do {
> >  		next = pmd_addr_end(addr, end);
> > -		err = apply_to_pte_range(mm, pmd, addr, next, fn,
> > data);
> > +		if (!closure->alloc && pmd_none_or_clear_bad(pmd))
> > +			continue;
> > +		err = apply_to_pte_range(closure, pmd, addr, next);
> >  		if (err)
> >  			break;
> >  	} while (pmd++, addr = next, addr != end);
> >  	return err;
> >  }
> >  
> > -static int apply_to_pud_range(struct mm_struct *mm, p4d_t *p4d,
> > -				     unsigned long addr, unsigned long
> > end,
> > -				     pte_fn_t fn, void *data)
> > +static int apply_to_pud_range(struct pfn_range_apply *closure,
> > p4d_t *p4d,
> > +			      unsigned long addr, unsigned long end)
> >  {
> >  	pud_t *pud;
> >  	unsigned long next;
> > -	int err;
> > +	int err = 0;
> >  
> > -	pud = pud_alloc(mm, p4d, addr);
> > +	pud = pud_alloc(closure->mm, p4d, addr);
> >  	if (!pud)
> >  		return -ENOMEM;
> > +
> >  	do {
> >  		next = pud_addr_end(addr, end);
> > -		err = apply_to_pmd_range(mm, pud, addr, next, fn,
> > data);
> > +		if (!closure->alloc && pud_none_or_clear_bad(pud))
> > +			continue;
> > +		err = apply_to_pmd_range(closure, pud, addr, next);
> >  		if (err)
> >  			break;
> >  	} while (pud++, addr = next, addr != end);
> >  	return err;
> >  }
> >  
> > -static int apply_to_p4d_range(struct mm_struct *mm, pgd_t *pgd,
> > -				     unsigned long addr, unsigned long
> > end,
> > -				     pte_fn_t fn, void *data)
> > +static int apply_to_p4d_range(struct pfn_range_apply *closure,
> > pgd_t *pgd,
> > +			      unsigned long addr, unsigned long end)
> >  {
> >  	p4d_t *p4d;
> >  	unsigned long next;
> > -	int err;
> > +	int err = 0;
> >  
> > -	p4d = p4d_alloc(mm, pgd, addr);
> > +	p4d = p4d_alloc(closure->mm, pgd, addr);
> >  	if (!p4d)
> >  		return -ENOMEM;
> > +
> >  	do {
> >  		next = p4d_addr_end(addr, end);
> > -		err = apply_to_pud_range(mm, p4d, addr, next, fn,
> > data);
> > +		if (!closure->alloc && p4d_none_or_clear_bad(p4d))
> > +			continue;
> > +		err = apply_to_pud_range(closure, p4d, addr, next);
> >  		if (err)
> >  			break;
> >  	} while (p4d++, addr = next, addr != end);
> >  	return err;
> >  }
> >  
> > -/*
> > - * Scan a region of virtual memory, filling in page tables as
> > necessary
> > - * and calling a provided function on each leaf page table.
> > +/**
> > + * apply_to_pfn_range - Scan a region of virtual memory, calling a
> > provided
> > + * function on each leaf page table entry
> > + * @closure: Details about how to scan and what function to apply
> > + * @addr: Start virtual address
> > + * @size: Size of the region
> > + *
> > + * If @closure->alloc is set to 1, the function will fill in the
> > page table
> > + * as necessary. Otherwise it will skip non-present parts.
> > + *
> > + * Returns: Zero on success. If the provided function returns a
> > non-zero status,
> > + * the page table walk will terminate and that status will be
> > returned.
> > + * If @closure->alloc is set to 1, then this function may also
> > return memory
> > + * allocation errors arising from allocating page table memory.
> >   */
> > -int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
> > -			unsigned long size, pte_fn_t fn, void *data)
> > +int apply_to_pfn_range(struct pfn_range_apply *closure,
> > +		       unsigned long addr, unsigned long size)
> >  {
> >  	pgd_t *pgd;
> >  	unsigned long next;
> > @@ -2049,16 +2065,57 @@ int apply_to_page_range(struct mm_struct
> > *mm, unsigned long addr,
> >  	if (WARN_ON(addr >= end))
> >  		return -EINVAL;
> >  
> > -	pgd = pgd_offset(mm, addr);
> > +	pgd = pgd_offset(closure->mm, addr);
> >  	do {
> >  		next = pgd_addr_end(addr, end);
> > -		err = apply_to_p4d_range(mm, pgd, addr, next, fn,
> > data);
> > +		if (!closure->alloc && pgd_none_or_clear_bad(pgd))
> > +			continue;
> > +		err = apply_to_p4d_range(closure, pgd, addr, next);
> >  		if (err)
> >  			break;
> >  	} while (pgd++, addr = next, addr != end);
> >  
> >  	return err;
> >  }
> > +EXPORT_SYMBOL_GPL(apply_to_pfn_range);
> > +
> > +struct page_range_apply {
> > +	struct pfn_range_apply pter;
> > +	pte_fn_t fn;
> > +	void *data;
> > +};
> > +
> > +/*
> > + * Callback wrapper to enable use of apply_to_pfn_range for
> > + * the apply_to_page_range interface
> > + */
> > +static int apply_to_page_range_wrapper(pte_t *pte, pgtable_t
> > token,
> > +				       unsigned long addr,
> > +				       struct pfn_range_apply *pter)
> > +{
> > +	struct page_range_apply *pra =
> > +		container_of(pter, typeof(*pra), pter);
> > +
> > +	return pra->fn(pte, token, addr, pra->data);
> > +}
> > +
> > +/*
> > + * Scan a region of virtual memory, filling in page tables as
> > necessary
> > + * and calling a provided function on each leaf page table.
> > + */
> > +int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
> > +			unsigned long size, pte_fn_t fn, void *data)
> > +{
> > +	struct page_range_apply pra = {
> > +		.pter = {.mm = mm,
> > +			 .alloc = 1,
> > +			 .ptefn = apply_to_page_range_wrapper },
> > +		.fn = fn,
> > +		.data = data
> > +	};
> > +
> > +	return apply_to_pfn_range(&pra.pter, addr, size);
> > +}
> >  EXPORT_SYMBOL_GPL(apply_to_page_range);
> >  
> >  /*
> > -- 
> > 2.19.0.rc1
> >
Jerome Glisse March 21, 2019, 8:24 p.m. UTC | #3
On Thu, Mar 21, 2019 at 07:59:35PM +0000, Thomas Hellstrom wrote:
> On Thu, 2019-03-21 at 09:52 -0400, Jerome Glisse wrote:
> > On Thu, Mar 21, 2019 at 01:22:35PM +0000, Thomas Hellstrom wrote:
> > > This is basically apply_to_page_range with added functionality:
> > > Allocating missing parts of the page table becomes optional, which
> > > means that the function can be guaranteed not to error if
> > > allocation
> > > is disabled. Also passing of the closure struct and callback
> > > function
> > > becomes different and more in line with how things are done
> > > elsewhere.
> > > 
> > > Finally we keep apply_to_page_range as a wrapper around
> > > apply_to_pfn_range
> > 
> > The apply_to_page_range() is dangerous API it does not follow other
> > mm patterns like mmu notifier. It is suppose to be use in arch code
> > or vmalloc or similar thing but not in regular driver code. I see
> > it has crept out of this and is being use by few device driver. I am
> > not sure we should encourage that.
> 
> I can certainly remove the EXPORT of the new apply_to_pfn_range() which
> will make sure its use stays within the mm code. I don't expect any
> additional usage except for the two address-space utilities.
> 
> I'm looking for examples to see how it could be more in line with the
> rest of the mm code. The main difference from the pattern in, for
> example, page_mkclean() seems to be that it's lacking the
> mmu_notifier_invalidate_start() and mmu_notifier_invalidate_end()?
> Perhaps the intention is to have the pte leaf functions notify on pte
> updates? How does this relate to arch_enter_lazy_mmu() which is called
> outside of the page table locks? The documentation appears a bit
> scarce...

Best is to use something like walk_page_range() and have proper mmu
notifier in the callback. The apply_to_page_range() is broken for
huge page (THP) and other things like that. Thought you should not
have THP within mmap of a device file (at least i do not thing any
driver does that).

Cheers,
Jérôme

Patch
diff mbox series

diff --git a/include/linux/mm.h b/include/linux/mm.h
index 80bb6408fe73..b7dd4ddd6efb 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2632,6 +2632,16 @@  typedef int (*pte_fn_t)(pte_t *pte, pgtable_t token, unsigned long addr,
 extern int apply_to_page_range(struct mm_struct *mm, unsigned long address,
 			       unsigned long size, pte_fn_t fn, void *data);
 
+struct pfn_range_apply;
+typedef int (*pter_fn_t)(pte_t *pte, pgtable_t token, unsigned long addr,
+			 struct pfn_range_apply *closure);
+struct pfn_range_apply {
+	struct mm_struct *mm;
+	pter_fn_t ptefn;
+	unsigned int alloc;
+};
+extern int apply_to_pfn_range(struct pfn_range_apply *closure,
+			      unsigned long address, unsigned long size);
 
 #ifdef CONFIG_PAGE_POISONING
 extern bool page_poisoning_enabled(void);
diff --git a/mm/memory.c b/mm/memory.c
index dcd80313cf10..0feb7191c2d2 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1938,18 +1938,17 @@  int vm_iomap_memory(struct vm_area_struct *vma, phys_addr_t start, unsigned long
 }
 EXPORT_SYMBOL(vm_iomap_memory);
 
-static int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd,
-				     unsigned long addr, unsigned long end,
-				     pte_fn_t fn, void *data)
+static int apply_to_pte_range(struct pfn_range_apply *closure, pmd_t *pmd,
+			      unsigned long addr, unsigned long end)
 {
 	pte_t *pte;
 	int err;
 	pgtable_t token;
 	spinlock_t *uninitialized_var(ptl);
 
-	pte = (mm == &init_mm) ?
+	pte = (closure->mm == &init_mm) ?
 		pte_alloc_kernel(pmd, addr) :
-		pte_alloc_map_lock(mm, pmd, addr, &ptl);
+		pte_alloc_map_lock(closure->mm, pmd, addr, &ptl);
 	if (!pte)
 		return -ENOMEM;
 
@@ -1960,86 +1959,103 @@  static int apply_to_pte_range(struct mm_struct *mm, pmd_t *pmd,
 	token = pmd_pgtable(*pmd);
 
 	do {
-		err = fn(pte++, token, addr, data);
+		err = closure->ptefn(pte++, token, addr, closure);
 		if (err)
 			break;
 	} while (addr += PAGE_SIZE, addr != end);
 
 	arch_leave_lazy_mmu_mode();
 
-	if (mm != &init_mm)
+	if (closure->mm != &init_mm)
 		pte_unmap_unlock(pte-1, ptl);
 	return err;
 }
 
-static int apply_to_pmd_range(struct mm_struct *mm, pud_t *pud,
-				     unsigned long addr, unsigned long end,
-				     pte_fn_t fn, void *data)
+static int apply_to_pmd_range(struct pfn_range_apply *closure, pud_t *pud,
+			      unsigned long addr, unsigned long end)
 {
 	pmd_t *pmd;
 	unsigned long next;
-	int err;
+	int err = 0;
 
 	BUG_ON(pud_huge(*pud));
 
-	pmd = pmd_alloc(mm, pud, addr);
+	pmd = pmd_alloc(closure->mm, pud, addr);
 	if (!pmd)
 		return -ENOMEM;
+
 	do {
 		next = pmd_addr_end(addr, end);
-		err = apply_to_pte_range(mm, pmd, addr, next, fn, data);
+		if (!closure->alloc && pmd_none_or_clear_bad(pmd))
+			continue;
+		err = apply_to_pte_range(closure, pmd, addr, next);
 		if (err)
 			break;
 	} while (pmd++, addr = next, addr != end);
 	return err;
 }
 
-static int apply_to_pud_range(struct mm_struct *mm, p4d_t *p4d,
-				     unsigned long addr, unsigned long end,
-				     pte_fn_t fn, void *data)
+static int apply_to_pud_range(struct pfn_range_apply *closure, p4d_t *p4d,
+			      unsigned long addr, unsigned long end)
 {
 	pud_t *pud;
 	unsigned long next;
-	int err;
+	int err = 0;
 
-	pud = pud_alloc(mm, p4d, addr);
+	pud = pud_alloc(closure->mm, p4d, addr);
 	if (!pud)
 		return -ENOMEM;
+
 	do {
 		next = pud_addr_end(addr, end);
-		err = apply_to_pmd_range(mm, pud, addr, next, fn, data);
+		if (!closure->alloc && pud_none_or_clear_bad(pud))
+			continue;
+		err = apply_to_pmd_range(closure, pud, addr, next);
 		if (err)
 			break;
 	} while (pud++, addr = next, addr != end);
 	return err;
 }
 
-static int apply_to_p4d_range(struct mm_struct *mm, pgd_t *pgd,
-				     unsigned long addr, unsigned long end,
-				     pte_fn_t fn, void *data)
+static int apply_to_p4d_range(struct pfn_range_apply *closure, pgd_t *pgd,
+			      unsigned long addr, unsigned long end)
 {
 	p4d_t *p4d;
 	unsigned long next;
-	int err;
+	int err = 0;
 
-	p4d = p4d_alloc(mm, pgd, addr);
+	p4d = p4d_alloc(closure->mm, pgd, addr);
 	if (!p4d)
 		return -ENOMEM;
+
 	do {
 		next = p4d_addr_end(addr, end);
-		err = apply_to_pud_range(mm, p4d, addr, next, fn, data);
+		if (!closure->alloc && p4d_none_or_clear_bad(p4d))
+			continue;
+		err = apply_to_pud_range(closure, p4d, addr, next);
 		if (err)
 			break;
 	} while (p4d++, addr = next, addr != end);
 	return err;
 }
 
-/*
- * Scan a region of virtual memory, filling in page tables as necessary
- * and calling a provided function on each leaf page table.
+/**
+ * apply_to_pfn_range - Scan a region of virtual memory, calling a provided
+ * function on each leaf page table entry
+ * @closure: Details about how to scan and what function to apply
+ * @addr: Start virtual address
+ * @size: Size of the region
+ *
+ * If @closure->alloc is set to 1, the function will fill in the page table
+ * as necessary. Otherwise it will skip non-present parts.
+ *
+ * Returns: Zero on success. If the provided function returns a non-zero status,
+ * the page table walk will terminate and that status will be returned.
+ * If @closure->alloc is set to 1, then this function may also return memory
+ * allocation errors arising from allocating page table memory.
  */
-int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
-			unsigned long size, pte_fn_t fn, void *data)
+int apply_to_pfn_range(struct pfn_range_apply *closure,
+		       unsigned long addr, unsigned long size)
 {
 	pgd_t *pgd;
 	unsigned long next;
@@ -2049,16 +2065,57 @@  int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
 	if (WARN_ON(addr >= end))
 		return -EINVAL;
 
-	pgd = pgd_offset(mm, addr);
+	pgd = pgd_offset(closure->mm, addr);
 	do {
 		next = pgd_addr_end(addr, end);
-		err = apply_to_p4d_range(mm, pgd, addr, next, fn, data);
+		if (!closure->alloc && pgd_none_or_clear_bad(pgd))
+			continue;
+		err = apply_to_p4d_range(closure, pgd, addr, next);
 		if (err)
 			break;
 	} while (pgd++, addr = next, addr != end);
 
 	return err;
 }
+EXPORT_SYMBOL_GPL(apply_to_pfn_range);
+
+struct page_range_apply {
+	struct pfn_range_apply pter;
+	pte_fn_t fn;
+	void *data;
+};
+
+/*
+ * Callback wrapper to enable use of apply_to_pfn_range for
+ * the apply_to_page_range interface
+ */
+static int apply_to_page_range_wrapper(pte_t *pte, pgtable_t token,
+				       unsigned long addr,
+				       struct pfn_range_apply *pter)
+{
+	struct page_range_apply *pra =
+		container_of(pter, typeof(*pra), pter);
+
+	return pra->fn(pte, token, addr, pra->data);
+}
+
+/*
+ * Scan a region of virtual memory, filling in page tables as necessary
+ * and calling a provided function on each leaf page table.
+ */
+int apply_to_page_range(struct mm_struct *mm, unsigned long addr,
+			unsigned long size, pte_fn_t fn, void *data)
+{
+	struct page_range_apply pra = {
+		.pter = {.mm = mm,
+			 .alloc = 1,
+			 .ptefn = apply_to_page_range_wrapper },
+		.fn = fn,
+		.data = data
+	};
+
+	return apply_to_pfn_range(&pra.pter, addr, size);
+}
 EXPORT_SYMBOL_GPL(apply_to_page_range);
 
 /*