[patch 1/3] mm: introduce follow_pte()

[patch 1/3] mm: introduce follow_pte()

[Johannes Weiner]

A generic readonly page table lookup helper to map an address space
and an address from it to a pte.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/memory.c |   37 +++++++++++++++++++++++++++++++++++++
 1 files changed, 37 insertions(+), 0 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index cf6873e..a621319 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3009,6 +3009,43 @@ int in_gate_area_no_task(unsigned long addr)
 
 #endif	/* __HAVE_ARCH_GATE_AREA */
 
+static int follow_pte(struct mm_struct *mm, unsigned long address,
+		pte_t **ptepp, spinlock_t **ptlp)
+{
+	pgd_t *pgd;
+	pud_t *pud;
+	pmd_t *pmd;
+	pte_t *ptep;
+
+	pgd = pgd_offset(mm, address);
+	if (pgd_none(*pgd) || unlikely(pgd_bad(*pgd)))
+		goto out;
+
+	pud = pud_offset(pgd, address);
+	if (pud_none(*pud) || unlikely(pud_bad(*pud)))
+		goto out;
+
+	pmd = pmd_offset(pud, address);
+	if (pmd_none(*pmd) || unlikely(pmd_bad(*pmd)))
+		goto out;
+
+	/* We cannot handle huge page PFN maps. Luckily they don't exist. */
+	if (pmd_huge(*pmd))
+		goto out;
+
+	ptep = pte_offset_map_lock(mm, pmd, address, ptlp);
+	if (!ptep)
+		goto out;
+	if (!pte_present(*ptep))
+		goto unlock;
+	*ptepp = ptep;
+	return 0;
+unlock:
+	pte_unmap_unlock(ptep, *ptlp);
+out:
+	return -EINVAL;
+}
+
 #ifdef CONFIG_HAVE_IOREMAP_PROT
 int follow_phys(struct vm_area_struct *vma,
 		unsigned long address, unsigned int flags,
-- 
1.6.2.1.135.gde769

Re: [patch 1/3] mm: introduce follow_pte()

[Andrew Morton]

On Mon,  4 May 2009 11:54:32 +0200
Johannes Weiner <hannes@cmpxchg.org> wrote:

> A generic readonly page table lookup helper to map an address space
> and an address from it to a pte.

umm, OK.

Is there actually some point to these three patches?  If so, what is it?

Re: [patch 1/3] mm: introduce follow_pte()

[Johannes Weiner]

On Tue, May 05, 2009 at 12:24:42PM -0700, Andrew Morton wrote:
> On Mon,  4 May 2009 11:54:32 +0200
> Johannes Weiner <hannes@cmpxchg.org> wrote:
> 
> > A generic readonly page table lookup helper to map an address space
> > and an address from it to a pte.
> 
> umm, OK.
> 
> Is there actually some point to these three patches?  If so, what is it?

Magnus needs to check for physical contiguity of a VMAs backing pages
to support zero-copy exportation of video data to userspace.

This series implements follow_pfn() so he can walk the VMA backing
pages and ensure their PFNs are in linear order.

[ This patch can be collapsed with 2/3, I just thought it would be
  easier to read the diffs when having them separate. ]

1/3 and 2/3: factor out the page table walk from follow_phys() into
follow_pte().

3/3: implement follow_pfn() on top of follow_pte().

Re: [patch 1/3] mm: introduce follow_pte()

[Andrew Morton]

On Tue, 5 May 2009 22:38:07 +0200
Johannes Weiner <hannes@cmpxchg.org> wrote:

> On Tue, May 05, 2009 at 12:24:42PM -0700, Andrew Morton wrote:
> > On Mon,  4 May 2009 11:54:32 +0200
> > Johannes Weiner <hannes@cmpxchg.org> wrote:
> > 
> > > A generic readonly page table lookup helper to map an address space
> > > and an address from it to a pte.
> > 
> > umm, OK.
> > 
> > Is there actually some point to these three patches?  If so, what is it?
> 
> Magnus needs to check for physical contiguity of a VMAs backing pages
> to support zero-copy exportation of video data to userspace.
> 
> This series implements follow_pfn() so he can walk the VMA backing
> pages and ensure their PFNs are in linear order.
> 
> [ This patch can be collapsed with 2/3, I just thought it would be
>   easier to read the diffs when having them separate. ]
> 
> 1/3 and 2/3: factor out the page table walk from follow_phys() into
> follow_pte().
> 
> 3/3: implement follow_pfn() on top of follow_pte().

So we could bundle these patches with Magnus's patchset, or we could
consider these three patches as a cleanup or something.

Given that 3/3 introduces an unused function, I'm inclined to sit tight
and await Magnus's work.

Re: [patch 1/3] mm: introduce follow_pte()

[Johannes Weiner]

On Tue, May 05, 2009 at 02:05:17PM -0700, Andrew Morton wrote:
> On Tue, 5 May 2009 22:38:07 +0200
> Johannes Weiner <hannes@cmpxchg.org> wrote:
> 
> > On Tue, May 05, 2009 at 12:24:42PM -0700, Andrew Morton wrote:
> > > On Mon,  4 May 2009 11:54:32 +0200
> > > Johannes Weiner <hannes@cmpxchg.org> wrote:
> > > 
> > > > A generic readonly page table lookup helper to map an address space
> > > > and an address from it to a pte.
> > > 
> > > umm, OK.
> > > 
> > > Is there actually some point to these three patches?  If so, what is it?
> > 
> > Magnus needs to check for physical contiguity of a VMAs backing pages
> > to support zero-copy exportation of video data to userspace.
> > 
> > This series implements follow_pfn() so he can walk the VMA backing
> > pages and ensure their PFNs are in linear order.
> > 
> > [ This patch can be collapsed with 2/3, I just thought it would be
> >   easier to read the diffs when having them separate. ]
> > 
> > 1/3 and 2/3: factor out the page table walk from follow_phys() into
> > follow_pte().
> > 
> > 3/3: implement follow_pfn() on top of follow_pte().
> 
> So we could bundle these patches with Magnus's patchset, or we could
> consider these three patches as a cleanup or something.
>
> Given that 3/3 introduces an unused function, I'm inclined to sit tight
> and await Magnus's work.

Yeah, I didn't see the video guys responding on Magnus' patch yet, so
let's wait for them.

Magnus, the actual conversion of your code should be trivial, could
you respin it on top of these three patches using follow_pfn() then?

	Thanks, Hannes

Re: [patch 1/3] mm: introduce follow_pte()

[Magnus Damm]

On Wed, May 6, 2009 at 6:21 AM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> On Tue, May 05, 2009 at 02:05:17PM -0700, Andrew Morton wrote:
>> On Tue, 5 May 2009 22:38:07 +0200
>> Johannes Weiner <hannes@cmpxchg.org> wrote:
>> > On Tue, May 05, 2009 at 12:24:42PM -0700, Andrew Morton wrote:
>> > > On Mon,  4 May 2009 11:54:32 +0200
>> > > Johannes Weiner <hannes@cmpxchg.org> wrote:
>> > >
>> > > > A generic readonly page table lookup helper to map an address space
>> > > > and an address from it to a pte.
>> > >
>> > > umm, OK.
>> > >
>> > > Is there actually some point to these three patches?  If so, what is it?
>> >
>> > Magnus needs to check for physical contiguity of a VMAs backing pages
>> > to support zero-copy exportation of video data to userspace.
>> >
>> > This series implements follow_pfn() so he can walk the VMA backing
>> > pages and ensure their PFNs are in linear order.
>> >
>> > [ This patch can be collapsed with 2/3, I just thought it would be
>> >   easier to read the diffs when having them separate. ]
>> >
>> > 1/3 and 2/3: factor out the page table walk from follow_phys() into
>> > follow_pte().
>> >
>> > 3/3: implement follow_pfn() on top of follow_pte().
>>
>> So we could bundle these patches with Magnus's patchset, or we could
>> consider these three patches as a cleanup or something.
>>
>> Given that 3/3 introduces an unused function, I'm inclined to sit tight
>> and await Magnus's work.
>
> Yeah, I didn't see the video guys responding on Magnus' patch yet, so
> let's wait for them.
>
> Magnus, the actual conversion of your code should be trivial, could
> you respin it on top of these three patches using follow_pfn() then?

So I tested the patches in -mm (1/3, 2/3, 3/3) together with the zero
copy patch and everything seems fine. Feel free to add acks from me,
least for patch 1/3 and 3/3 - i know too little about the generic case
to say anything about 2/3.

Acked-by: Magnus Damm <damm@igel.co.jp>

I'll send V3 of my zero copy patch in a little while. Thanks a lot for the help!

Cheers,

/ magnus

[patch 2/3] mm: use generic follow_pte() in follow_phys()

[Johannes Weiner]

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/memory.c |   37 ++++++-------------------------------
 1 files changed, 6 insertions(+), 31 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index a621319..aee167d 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3051,50 +3051,25 @@ int follow_phys(struct vm_area_struct *vma,
 		unsigned long address, unsigned int flags,
 		unsigned long *prot, resource_size_t *phys)
 {
-	pgd_t *pgd;
-	pud_t *pud;
-	pmd_t *pmd;
+	int ret = -EINVAL;
 	pte_t *ptep, pte;
 	spinlock_t *ptl;
-	resource_size_t phys_addr = 0;
-	struct mm_struct *mm = vma->vm_mm;
-	int ret = -EINVAL;
 
 	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
 		goto out;
 
-	pgd = pgd_offset(mm, address);
-	if (pgd_none(*pgd) || unlikely(pgd_bad(*pgd)))
-		goto out;
-
-	pud = pud_offset(pgd, address);
-	if (pud_none(*pud) || unlikely(pud_bad(*pud)))
-		goto out;
-
-	pmd = pmd_offset(pud, address);
-	if (pmd_none(*pmd) || unlikely(pmd_bad(*pmd)))
-		goto out;
-
-	/* We cannot handle huge page PFN maps. Luckily they don't exist. */
-	if (pmd_huge(*pmd))
+	if (follow_pte(vma->vm_mm, address, &ptep, &ptl))
 		goto out;
-
-	ptep = pte_offset_map_lock(mm, pmd, address, &ptl);
-	if (!ptep)
-		goto out;
-
 	pte = *ptep;
-	if (!pte_present(pte))
-		goto unlock;
+
 	if ((flags & FOLL_WRITE) && !pte_write(pte))
 		goto unlock;
-	phys_addr = pte_pfn(pte);
-	phys_addr <<= PAGE_SHIFT; /* Shift here to avoid overflow on PAE */
 
 	*prot = pgprot_val(pte_pgprot(pte));
-	*phys = phys_addr;
-	ret = 0;
+	/* Shift here to avoid overflow on PAE */
+	*phys = pte_pfn(pte) << PAGE_SHIFT;
 
+	ret = 0;
 unlock:
 	pte_unmap_unlock(ptep, ptl);
 out:
-- 
1.6.2.1.135.gde769

Re: [patch 2/3] mm: use generic follow_pte() in follow_phys()

[Johannes Weiner]

On Mon, May 04, 2009 at 11:54:33AM +0200, Johannes Weiner wrote:
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  mm/memory.c |   37 ++++++-------------------------------
>  1 files changed, 6 insertions(+), 31 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index a621319..aee167d 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3051,50 +3051,25 @@ int follow_phys(struct vm_area_struct *vma,
>  		unsigned long address, unsigned int flags,
>  		unsigned long *prot, resource_size_t *phys)
>  {
> -	pgd_t *pgd;
> -	pud_t *pud;
> -	pmd_t *pmd;
> +	int ret = -EINVAL;
>  	pte_t *ptep, pte;
>  	spinlock_t *ptl;
> -	resource_size_t phys_addr = 0;
> -	struct mm_struct *mm = vma->vm_mm;
> -	int ret = -EINVAL;
>  
>  	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
>  		goto out;
>  
> -	pgd = pgd_offset(mm, address);
> -	if (pgd_none(*pgd) || unlikely(pgd_bad(*pgd)))
> -		goto out;
> -
> -	pud = pud_offset(pgd, address);
> -	if (pud_none(*pud) || unlikely(pud_bad(*pud)))
> -		goto out;
> -
> -	pmd = pmd_offset(pud, address);
> -	if (pmd_none(*pmd) || unlikely(pmd_bad(*pmd)))
> -		goto out;
> -
> -	/* We cannot handle huge page PFN maps. Luckily they don't exist. */
> -	if (pmd_huge(*pmd))
> +	if (follow_pte(vma->vm_mm, address, &ptep, &ptl))
>  		goto out;
> -
> -	ptep = pte_offset_map_lock(mm, pmd, address, &ptl);
> -	if (!ptep)
> -		goto out;
> -
>  	pte = *ptep;
> -	if (!pte_present(pte))
> -		goto unlock;
> +
>  	if ((flags & FOLL_WRITE) && !pte_write(pte))
>  		goto unlock;
> -	phys_addr = pte_pfn(pte);
> -	phys_addr <<= PAGE_SHIFT; /* Shift here to avoid overflow on PAE */
>  
>  	*prot = pgprot_val(pte_pgprot(pte));
> -	*phys = phys_addr;
> -	ret = 0;
> +	/* Shift here to avoid overflow on PAE */
> +	*phys = pte_pfn(pte) << PAGE_SHIFT;

Yuk, had my head in the butt here.  Changed to

	*phys = (resource_size_t)pte_pfn(pte) << PAGE_SHIFT;

---
From: Johannes Weiner <hannes@cmpxchg.org>
Subject: mm: use generic follow_pte() in follow_phys()

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/memory.c |   36 +++++-------------------------------
 1 files changed, 5 insertions(+), 31 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index a621319..c047950 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3051,50 +3051,24 @@ int follow_phys(struct vm_area_struct *vma,
 		unsigned long address, unsigned int flags,
 		unsigned long *prot, resource_size_t *phys)
 {
-	pgd_t *pgd;
-	pud_t *pud;
-	pmd_t *pmd;
+	int ret = -EINVAL;
 	pte_t *ptep, pte;
 	spinlock_t *ptl;
-	resource_size_t phys_addr = 0;
-	struct mm_struct *mm = vma->vm_mm;
-	int ret = -EINVAL;
 
 	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
 		goto out;
 
-	pgd = pgd_offset(mm, address);
-	if (pgd_none(*pgd) || unlikely(pgd_bad(*pgd)))
-		goto out;
-
-	pud = pud_offset(pgd, address);
-	if (pud_none(*pud) || unlikely(pud_bad(*pud)))
-		goto out;
-
-	pmd = pmd_offset(pud, address);
-	if (pmd_none(*pmd) || unlikely(pmd_bad(*pmd)))
-		goto out;
-
-	/* We cannot handle huge page PFN maps. Luckily they don't exist. */
-	if (pmd_huge(*pmd))
+	if (follow_pte(vma->vm_mm, address, &ptep, &ptl))
 		goto out;
-
-	ptep = pte_offset_map_lock(mm, pmd, address, &ptl);
-	if (!ptep)
-		goto out;
-
 	pte = *ptep;
-	if (!pte_present(pte))
-		goto unlock;
+
 	if ((flags & FOLL_WRITE) && !pte_write(pte))
 		goto unlock;
-	phys_addr = pte_pfn(pte);
-	phys_addr <<= PAGE_SHIFT; /* Shift here to avoid overflow on PAE */
 
 	*prot = pgprot_val(pte_pgprot(pte));
-	*phys = phys_addr;
-	ret = 0;
+	*phys = (resource_size_t)pte_pfn(pte) << PAGE_SHIFT;
 
+	ret = 0;
 unlock:
 	pte_unmap_unlock(ptep, ptl);
 out:
-- 
1.6.2.1.135.gde769

[patch 3/3] mm: introduce follow_pfn()

[Johannes Weiner]

Analoguous to follow_phys(), add a helper that looks up the PFN
instead.  It also only allows IO mappings or PFN mappings.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 include/linux/mm.h |    2 ++
 mm/memory.c        |   19 +++++++++++++++++++
 2 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index bff1f0d..1cca8b6 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -794,6 +794,8 @@ int copy_page_range(struct mm_struct *dst, struct mm_struct *src,
 			struct vm_area_struct *vma);
 void unmap_mapping_range(struct address_space *mapping,
 		loff_t const holebegin, loff_t const holelen, int even_cows);
+int follow_pfn(struct vm_area_struct *vma, unsigned long address,
+	unsigned long *pfn);
 int follow_phys(struct vm_area_struct *vma, unsigned long address,
 		unsigned int flags, unsigned long *prot, resource_size_t *phys);
 int generic_access_phys(struct vm_area_struct *vma, unsigned long addr,
diff --git a/mm/memory.c b/mm/memory.c
index aee167d..05fc8e5 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3046,6 +3046,25 @@ out:
 	return -EINVAL;
 }
 
+int follow_pfn(struct vm_area_struct *vma, unsigned long address,
+	unsigned long *pfn)
+{
+	int ret = -EINVAL;
+	spinlock_t *ptl;
+	pte_t *ptep;
+
+	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
+		return ret;
+
+	ret = follow_pte(vma->vm_mm, address, &ptep, &ptl);
+	if (ret)
+		return ret;
+	*pfn = pte_pfn(*ptep);
+	pte_unmap_unlock(ptep, ptl);
+	return 0;
+}
+EXPORT_SYMBOL(follow_pfn);
+
 #ifdef CONFIG_HAVE_IOREMAP_PROT
 int follow_phys(struct vm_area_struct *vma,
 		unsigned long address, unsigned int flags,
-- 
1.6.2.1.135.gde769

Re: [patch 3/3] mm: introduce follow_pfn()

[Christoph Hellwig]

On Mon, May 04, 2009 at 11:54:34AM +0200, Johannes Weiner wrote:
> Analoguous to follow_phys(), add a helper that looks up the PFN
> instead.  It also only allows IO mappings or PFN mappings.

A kerneldoc describing what it does and the limitations would be
extremly helpful.

[patch 3/3 v2] mm: introduce follow_pfn()

[Johannes Weiner]

Analoguous to follow_phys(), add a helper that looks up the PFN at a
user virtual address in an IO mapping or a raw PFN mapping.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 include/linux/mm.h |    2 ++
 mm/memory.c        |   29 +++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 0 deletions(-)

On Mon, May 04, 2009 at 07:08:41AM -0400, Christoph Hellwig wrote:
> On Mon, May 04, 2009 at 11:54:34AM +0200, Johannes Weiner wrote:
> > Analoguous to follow_phys(), add a helper that looks up the PFN
> > instead.  It also only allows IO mappings or PFN mappings.
> 
> A kerneldoc describing what it does and the limitations would be
> extremly helpful.

Agreed.  How is this?

diff --git a/include/linux/mm.h b/include/linux/mm.h
index bff1f0d..1cca8b6 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -794,6 +794,8 @@ int copy_page_range(struct mm_struct *dst, struct mm_struct *src,
 			struct vm_area_struct *vma);
 void unmap_mapping_range(struct address_space *mapping,
 		loff_t const holebegin, loff_t const holelen, int even_cows);
+int follow_pfn(struct vm_area_struct *vma, unsigned long address,
+	unsigned long *pfn);
 int follow_phys(struct vm_area_struct *vma, unsigned long address,
 		unsigned int flags, unsigned long *prot, resource_size_t *phys);
 int generic_access_phys(struct vm_area_struct *vma, unsigned long addr,
diff --git a/mm/memory.c b/mm/memory.c
index c047950..f86aee1 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3046,6 +3046,35 @@ out:
 	return -EINVAL;
 }
 
+/**
+ * follow_pfn - look up PFN at a user virtual address
+ * @vma: memory mapping
+ * @address: user virtual address
+ * @pfn: location to store found PFN
+ *
+ * Only IO mappings and raw PFN mappings are allowed.
+ *
+ * Returns zero and the pfn at @pfn on success, -ve otherwise.
+ */
+int follow_pfn(struct vm_area_struct *vma, unsigned long address,
+	unsigned long *pfn)
+{
+	int ret = -EINVAL;
+	spinlock_t *ptl;
+	pte_t *ptep;
+
+	if (!(vma->vm_flags & (VM_IO | VM_PFNMAP)))
+		return ret;
+
+	ret = follow_pte(vma->vm_mm, address, &ptep, &ptl);
+	if (ret)
+		return ret;
+	*pfn = pte_pfn(*ptep);
+	pte_unmap_unlock(ptep, ptl);
+	return 0;
+}
+EXPORT_SYMBOL(follow_pfn);
+
 #ifdef CONFIG_HAVE_IOREMAP_PROT
 int follow_phys(struct vm_area_struct *vma,
 		unsigned long address, unsigned int flags,
-- 
1.6.2.1.135.gde769

Re: [patch 3/3 v2] mm: introduce follow_pfn()

[Christoph Hellwig]

Looks good to me.