* [PATCH] rmap : tidy the code
@ 2009-09-28 9:03 Huang Shijie
2009-09-28 10:44 ` Hugh Dickins
0 siblings, 1 reply; 5+ messages in thread
From: Huang Shijie @ 2009-09-28 9:03 UTC (permalink / raw)
To: akpm; +Cc: hugh.dickins, linux-mm, Huang Shijie
Introduce is_page_mapped_in_vma() to merge the vma_address() and
page_check_address().
Make the rmap codes more simple.
Signed-off-by: Huang Shijie <shijie8@gmail.com>
---
mm/rmap.c | 59 ++++++++++++++++++++++++++++-------------------------------
1 files changed, 28 insertions(+), 31 deletions(-)
diff --git a/mm/rmap.c b/mm/rmap.c
index 28aafe2..69e7314 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -307,6 +307,27 @@ pte_t *page_check_address(struct page *page, struct mm_struct *mm,
return NULL;
}
+/*
+ * This helper function checks whether a page is mapped in a VMA.
+ * On success returns 1 with pte mapped and locked.
+ */
+static inline bool
+is_page_mapped_in_vma(struct page *page, struct vm_area_struct *vma,
+ unsigned long *addr, pte_t **ptep, spinlock_t **ptlp, int sync)
+{
+ unsigned long address;
+
+ address = vma_address(page, vma);
+ if (address == -EFAULT)
+ return 0;
+ *ptep = page_check_address(page, vma->vm_mm, address, ptlp, sync);
+ if (!(*ptep))
+ return 0;
+
+ *addr = address;
+ return 1;
+}
+
/**
* page_mapped_in_vma - check whether a page is really mapped in a VMA
* @page: the page to test
@@ -322,14 +343,9 @@ int page_mapped_in_vma(struct page *page, struct vm_area_struct *vma)
pte_t *pte;
spinlock_t *ptl;
- address = vma_address(page, vma);
- if (address == -EFAULT) /* out of vma range */
- return 0;
- pte = page_check_address(page, vma->vm_mm, address, &ptl, 1);
- if (!pte) /* the page is not in this mm */
+ if (!is_page_mapped_in_vma(page, vma, &address, &pte, &ptl, 1))
return 0;
pte_unmap_unlock(pte, ptl);
-
return 1;
}
@@ -348,14 +364,8 @@ static int page_referenced_one(struct page *page,
spinlock_t *ptl;
int referenced = 0;
- address = vma_address(page, vma);
- if (address == -EFAULT)
- goto out;
-
- pte = page_check_address(page, mm, address, &ptl, 0);
- if (!pte)
- goto out;
-
+ if (!is_page_mapped_in_vma(page, vma, &address, &pte, &ptl, 0))
+ return 0;
/*
* Don't want to elevate referenced for mlocked page that gets this far,
* in order that it progresses to try_to_unmap and is moved to the
@@ -388,7 +398,6 @@ static int page_referenced_one(struct page *page,
out_unmap:
(*mapcount)--;
pte_unmap_unlock(pte, ptl);
-out:
if (referenced)
*vm_flags |= vma->vm_flags;
return referenced;
@@ -543,13 +552,8 @@ static int page_mkclean_one(struct page *page, struct vm_area_struct *vma)
spinlock_t *ptl;
int ret = 0;
- address = vma_address(page, vma);
- if (address == -EFAULT)
- goto out;
-
- pte = page_check_address(page, mm, address, &ptl, 1);
- if (!pte)
- goto out;
+ if (!is_page_mapped_in_vma(page, vma, &address, &pte, &ptl, 1))
+ return 0;
if (pte_dirty(*pte) || pte_write(*pte)) {
pte_t entry;
@@ -563,7 +567,6 @@ static int page_mkclean_one(struct page *page, struct vm_area_struct *vma)
}
pte_unmap_unlock(pte, ptl);
-out:
return ret;
}
@@ -770,13 +773,8 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
spinlock_t *ptl;
int ret = SWAP_AGAIN;
- address = vma_address(page, vma);
- if (address == -EFAULT)
- goto out;
-
- pte = page_check_address(page, mm, address, &ptl, 0);
- if (!pte)
- goto out;
+ if (!is_page_mapped_in_vma(page, vma, &address, &pte, &ptl, 0))
+ return 0;
/*
* If the page is mlock()d, we cannot swap it out.
@@ -855,7 +853,6 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
out_unmap:
pte_unmap_unlock(pte, ptl);
-out:
return ret;
}
--
1.6.0.6
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] rmap : tidy the code
2009-09-28 9:03 [PATCH] rmap : tidy the code Huang Shijie
@ 2009-09-28 10:44 ` Hugh Dickins
2009-09-28 11:30 ` Nikita Danilov
0 siblings, 1 reply; 5+ messages in thread
From: Hugh Dickins @ 2009-09-28 10:44 UTC (permalink / raw)
To: Huang Shijie; +Cc: Nikita Danilov, akpm, linux-mm
On Mon, 28 Sep 2009, Huang Shijie wrote:
> Introduce is_page_mapped_in_vma() to merge the vma_address() and
> page_check_address().
>
> Make the rmap codes more simple.
There is indeed a recurring pattern there; but personally, I prefer
that recurring pattern, to introducing another multi-argument layer.
I think it would make more sense to do the vma_address() inside (a
differently named) page_check_address(); but that would still have
to return the address, so I'll probably prefer what we have now.
(And that seems to have been Nikita's preference when he introduced
page_check_address(), to keep the vma_address() part of it separate.)
Other opinions?
Hugh
>
> Signed-off-by: Huang Shijie <shijie8@gmail.com>
> ---
> mm/rmap.c | 59 ++++++++++++++++++++++++++++-------------------------------
> 1 files changed, 28 insertions(+), 31 deletions(-)
>
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 28aafe2..69e7314 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -307,6 +307,27 @@ pte_t *page_check_address(struct page *page, struct mm_struct *mm,
> return NULL;
> }
>
> +/*
> + * This helper function checks whether a page is mapped in a VMA.
> + * On success returns 1 with pte mapped and locked.
> + */
> +static inline bool
> +is_page_mapped_in_vma(struct page *page, struct vm_area_struct *vma,
> + unsigned long *addr, pte_t **ptep, spinlock_t **ptlp, int sync)
> +{
> + unsigned long address;
> +
> + address = vma_address(page, vma);
> + if (address == -EFAULT)
> + return 0;
> + *ptep = page_check_address(page, vma->vm_mm, address, ptlp, sync);
> + if (!(*ptep))
> + return 0;
> +
> + *addr = address;
> + return 1;
> +}
> +
> /**
> * page_mapped_in_vma - check whether a page is really mapped in a VMA
> * @page: the page to test
> @@ -322,14 +343,9 @@ int page_mapped_in_vma(struct page *page, struct vm_area_struct *vma)
> pte_t *pte;
> spinlock_t *ptl;
>
> - address = vma_address(page, vma);
> - if (address == -EFAULT) /* out of vma range */
> - return 0;
> - pte = page_check_address(page, vma->vm_mm, address, &ptl, 1);
> - if (!pte) /* the page is not in this mm */
> + if (!is_page_mapped_in_vma(page, vma, &address, &pte, &ptl, 1))
> return 0;
> pte_unmap_unlock(pte, ptl);
> -
> return 1;
> }
>
> @@ -348,14 +364,8 @@ static int page_referenced_one(struct page *page,
> spinlock_t *ptl;
> int referenced = 0;
>
> - address = vma_address(page, vma);
> - if (address == -EFAULT)
> - goto out;
> -
> - pte = page_check_address(page, mm, address, &ptl, 0);
> - if (!pte)
> - goto out;
> -
> + if (!is_page_mapped_in_vma(page, vma, &address, &pte, &ptl, 0))
> + return 0;
> /*
> * Don't want to elevate referenced for mlocked page that gets this far,
> * in order that it progresses to try_to_unmap and is moved to the
> @@ -388,7 +398,6 @@ static int page_referenced_one(struct page *page,
> out_unmap:
> (*mapcount)--;
> pte_unmap_unlock(pte, ptl);
> -out:
> if (referenced)
> *vm_flags |= vma->vm_flags;
> return referenced;
> @@ -543,13 +552,8 @@ static int page_mkclean_one(struct page *page, struct vm_area_struct *vma)
> spinlock_t *ptl;
> int ret = 0;
>
> - address = vma_address(page, vma);
> - if (address == -EFAULT)
> - goto out;
> -
> - pte = page_check_address(page, mm, address, &ptl, 1);
> - if (!pte)
> - goto out;
> + if (!is_page_mapped_in_vma(page, vma, &address, &pte, &ptl, 1))
> + return 0;
>
> if (pte_dirty(*pte) || pte_write(*pte)) {
> pte_t entry;
> @@ -563,7 +567,6 @@ static int page_mkclean_one(struct page *page, struct vm_area_struct *vma)
> }
>
> pte_unmap_unlock(pte, ptl);
> -out:
> return ret;
> }
>
> @@ -770,13 +773,8 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
> spinlock_t *ptl;
> int ret = SWAP_AGAIN;
>
> - address = vma_address(page, vma);
> - if (address == -EFAULT)
> - goto out;
> -
> - pte = page_check_address(page, mm, address, &ptl, 0);
> - if (!pte)
> - goto out;
> + if (!is_page_mapped_in_vma(page, vma, &address, &pte, &ptl, 0))
> + return 0;
>
> /*
> * If the page is mlock()d, we cannot swap it out.
> @@ -855,7 +853,6 @@ static int try_to_unmap_one(struct page *page, struct vm_area_struct *vma,
>
> out_unmap:
> pte_unmap_unlock(pte, ptl);
> -out:
> return ret;
> }
>
> --
> 1.6.0.6
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rmap : tidy the code
2009-09-28 10:44 ` Hugh Dickins
@ 2009-09-28 11:30 ` Nikita Danilov
2009-09-29 2:06 ` Huang Shijie
0 siblings, 1 reply; 5+ messages in thread
From: Nikita Danilov @ 2009-09-28 11:30 UTC (permalink / raw)
To: Hugh Dickins; +Cc: Huang Shijie, akpm, linux-mm
2009/9/28 Hugh Dickins <hugh.dickins@tiscali.co.uk>:
> On Mon, 28 Sep 2009, Huang Shijie wrote:
>
>> Introduce is_page_mapped_in_vma() to merge the vma_address() and
>> page_check_address().
>>
>> Make the rmap codes more simple.
>
> There is indeed a recurring pattern there; but personally, I prefer
> that recurring pattern, to introducing another multi-argument layer.
>
> I think it would make more sense to do the vma_address() inside (a
> differently named) page_check_address(); but that would still have
> to return the address, so I'll probably prefer what we have now.
>
> (And that seems to have been Nikita's preference when he introduced
> page_check_address(), to keep the vma_address() part of it separate.)
>
Indeed, I tried to minimize the number of parameters and to avoid
making "address" an output parameter. But on the other hand, there
were only 2 page_check_address() call-sites back then. Now there are 5
of them, so adding a parameter is more justifiable.
> Other opinions?
I agree that adding EFAULT check into page_check_address() is better.
The only call-site that does not call vma_address() before
page_check_address() is __xip_unmap() and it explicitly BUG_ON()s on
the same condition.
>
> Hugh
>
Nikita.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rmap : tidy the code
2009-09-28 11:30 ` Nikita Danilov
@ 2009-09-29 2:06 ` Huang Shijie
2009-09-29 10:28 ` Hugh Dickins
0 siblings, 1 reply; 5+ messages in thread
From: Huang Shijie @ 2009-09-29 2:06 UTC (permalink / raw)
To: Nikita Danilov; +Cc: Hugh Dickins, akpm, linux-mm
Nikita Danilov wrote:
> I agree that adding EFAULT check into page_check_address() is better.
> The only call-site that does not call vma_address() before
> page_check_address() is __xip_unmap() and it explicitly BUG_ON()s on
> the same condition.
>
>
If adding vma_address() into page_check_address() , it's bad for
__xip_unmap to change its code.
__xip_unmap must fake a page to satisfy the new "page_check_address()".
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] rmap : tidy the code
2009-09-29 2:06 ` Huang Shijie
@ 2009-09-29 10:28 ` Hugh Dickins
0 siblings, 0 replies; 5+ messages in thread
From: Hugh Dickins @ 2009-09-29 10:28 UTC (permalink / raw)
To: Huang Shijie; +Cc: Nikita Danilov, akpm, linux-mm
On Tue, 29 Sep 2009, Huang Shijie wrote:
> Nikita Danilov wrote:
> > I agree that adding EFAULT check into page_check_address() is better.
> > The only call-site that does not call vma_address() before
> > page_check_address() is __xip_unmap() and it explicitly BUG_ON()s on
> > the same condition.
> >
> >
> If adding vma_address() into page_check_address() , it's bad for
> __xip_unmap to change its code.
> __xip_unmap must fake a page to satisfy the new "page_check_address()".
You're right, I'd missed that too.
Hugh
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-09-29 10:06 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-09-28 9:03 [PATCH] rmap : tidy the code Huang Shijie
2009-09-28 10:44 ` Hugh Dickins
2009-09-28 11:30 ` Nikita Danilov
2009-09-29 2:06 ` Huang Shijie
2009-09-29 10:28 ` Hugh Dickins
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.