[PATCH v2] mm: simplify find_vma_prev

[PATCH v2] mm: simplify find_vma_prev

[kosaki.motohiro]

From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

commit 297c5eee37 (mm: make the vma list be doubly linked) added
vm_prev member into vm_area_struct. Therefore we can simplify
find_vma_prev() by using it. Also, this change help to improve
page fault performance because it has strong locality of reference.

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 mm/mmap.c |   40 +++++++++++-----------------------------
 1 files changed, 11 insertions(+), 29 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index eae90af..a84539b 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1603,39 +1603,21 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
 
 EXPORT_SYMBOL(find_vma);
 
-/* Same as find_vma, but also return a pointer to the previous VMA in *pprev. */
+/*
+ * Same as find_vma, but also return a pointer to the previous VMA in *pprev.
+ * Note: pprev is set to NULL when return value is NULL.
+ */
 struct vm_area_struct *
-find_vma_prev(struct mm_struct *mm, unsigned long addr,
-			struct vm_area_struct **pprev)
+find_vma_prev(struct mm_struct *mm, unsigned long addr, struct vm_area_struct **pprev)
 {
-	struct vm_area_struct *vma = NULL, *prev = NULL;
-	struct rb_node *rb_node;
-	if (!mm)
-		goto out;
-
-	/* Guard against addr being lower than the first VMA */
-	vma = mm->mmap;
-
-	/* Go through the RB tree quickly. */
-	rb_node = mm->mm_rb.rb_node;
-
-	while (rb_node) {
-		struct vm_area_struct *vma_tmp;
-		vma_tmp = rb_entry(rb_node, struct vm_area_struct, vm_rb);
+	struct vm_area_struct *vma;
 
-		if (addr < vma_tmp->vm_end) {
-			rb_node = rb_node->rb_left;
-		} else {
-			prev = vma_tmp;
-			if (!prev->vm_next || (addr < prev->vm_next->vm_end))
-				break;
-			rb_node = rb_node->rb_right;
-		}
-	}
+	*pprev = NULL;
+	vma = find_vma(mm, addr);
+	if (vma)
+		*pprev = vma->vm_prev;
 
-out:
-	*pprev = prev;
-	return prev ? prev->vm_next : vma;
+	return vma;
 }
 
 /*
-- 
1.7.1

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

Re: [PATCH v2] mm: simplify find_vma_prev

[Joe Perches]

On Fri, 2011-12-09 at 16:23 -0500, kosaki.motohiro@gmail.com wrote:
> commit 297c5eee37 (mm: make the vma list be doubly linked) added
> vm_prev member into vm_area_struct. Therefore we can simplify
> find_vma_prev() by using it. Also, this change help to improve
> page fault performance because it has strong locality of reference.

trivia:

> diff --git a/mm/mmap.c b/mm/mmap.c
[]
> @@ -1603,39 +1603,21 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
>  
>  EXPORT_SYMBOL(find_vma);
>  
> -/* Same as find_vma, but also return a pointer to the previous VMA in *pprev. */
> +/*
> + * Same as find_vma, but also return a pointer to the previous VMA in *pprev.
> + * Note: pprev is set to NULL when return value is NULL.
> + */
>  struct vm_area_struct *
> -find_vma_prev(struct mm_struct *mm, unsigned long addr,
> -			struct vm_area_struct **pprev)

> +find_vma_prev(struct mm_struct *mm, unsigned long addr, struct vm_area_struct **pprev)

eh.  This declaration change seems gratuitous and it exceeds 80 columns.

> +	*pprev = NULL;
> +	vma = find_vma(mm, addr);
> +	if (vma)
> +		*pprev = vma->vm_prev;

There's no need to possibly set *pprev twice.

Maybe
{
	struct vm_area_struct *vma = find_vma(mm, addr);

	*pprev = vma ? vma->vm_prev : NULL;
or
	if (vma)
		*pprev = vma->vm_prev;
	else
		*pprev = NULL;

	return vma;
}
 
> -out:
> -	*pprev = prev;
> -	return prev ? prev->vm_next : vma;
> +	return vma;
>  }
>  
>  /*



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

[PATCH v3] mm: simplify find_vma_prev

[kosaki.motohiro]

From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

commit 297c5eee37 (mm: make the vma list be doubly linked) added
vm_prev member into vm_area_struct. Therefore we can simplify
find_vma_prev() by using it. Also, this change help to improve
page fault performance because it has strong locality of reference.

Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 mm/mmap.c |   36 ++++++++----------------------------
 1 files changed, 8 insertions(+), 28 deletions(-)

diff --git a/mm/mmap.c b/mm/mmap.c
index eae90af..b9c0241 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -1603,39 +1603,19 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
 
 EXPORT_SYMBOL(find_vma);
 
-/* Same as find_vma, but also return a pointer to the previous VMA in *pprev. */
+/*
+ * Same as find_vma, but also return a pointer to the previous VMA in *pprev.
+ * Note: pprev is set to NULL when return value is NULL.
+ */
 struct vm_area_struct *
 find_vma_prev(struct mm_struct *mm, unsigned long addr,
 			struct vm_area_struct **pprev)
 {
-	struct vm_area_struct *vma = NULL, *prev = NULL;
-	struct rb_node *rb_node;
-	if (!mm)
-		goto out;
-
-	/* Guard against addr being lower than the first VMA */
-	vma = mm->mmap;
-
-	/* Go through the RB tree quickly. */
-	rb_node = mm->mm_rb.rb_node;
-
-	while (rb_node) {
-		struct vm_area_struct *vma_tmp;
-		vma_tmp = rb_entry(rb_node, struct vm_area_struct, vm_rb);
-
-		if (addr < vma_tmp->vm_end) {
-			rb_node = rb_node->rb_left;
-		} else {
-			prev = vma_tmp;
-			if (!prev->vm_next || (addr < prev->vm_next->vm_end))
-				break;
-			rb_node = rb_node->rb_right;
-		}
-	}
+	struct vm_area_struct *vma;
 
-out:
-	*pprev = prev;
-	return prev ? prev->vm_next : vma;
+	vma = find_vma(mm, addr);
+	*pprev = vma ? vma->vm_prev : NULL;
+	return vma;
 }
 
 /*
-- 
1.7.1

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

Re: [PATCH v3] mm: simplify find_vma_prev

[KAMEZAWA Hiroyuki]

On Fri,  9 Dec 2011 17:48:40 -0500
kosaki.motohiro@gmail.com wrote:

> From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> 
> commit 297c5eee37 (mm: make the vma list be doubly linked) added
> vm_prev member into vm_area_struct. Therefore we can simplify
> find_vma_prev() by using it. Also, this change help to improve
> page fault performance because it has strong locality of reference.
> 
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

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

Re: [PATCH v3] mm: simplify find_vma_prev

[KAMEZAWA Hiroyuki]

On Mon, 12 Dec 2011 09:49:30 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Fri,  9 Dec 2011 17:48:40 -0500
> kosaki.motohiro@gmail.com wrote:
> 
> > From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > 
> > commit 297c5eee37 (mm: make the vma list be doubly linked) added
> > vm_prev member into vm_area_struct. Therefore we can simplify
> > find_vma_prev() by using it. Also, this change help to improve
> > page fault performance because it has strong locality of reference.
> > 
> > Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> 
> Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 

Hmm, your work remind me of a patch I tried in past.
Here is a refleshed one...how do you think ?

==

Re: [PATCH v3] mm: simplify find_vma_prev

[KOSAKI Motohiro]

(12/12/11 4:27 AM), KAMEZAWA Hiroyuki wrote:
> On Mon, 12 Dec 2011 09:49:30 +0900
> KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>  wrote:
>
>> On Fri,  9 Dec 2011 17:48:40 -0500
>> kosaki.motohiro@gmail.com wrote:
>>
>>> From: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
>>>
>>> commit 297c5eee37 (mm: make the vma list be doubly linked) added
>>> vm_prev member into vm_area_struct. Therefore we can simplify
>>> find_vma_prev() by using it. Also, this change help to improve
>>> page fault performance because it has strong locality of reference.
>>>
>>> Signed-off-by: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
>>
>> Reviewed-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>>
>
> Hmm, your work remind me of a patch I tried in past.
> Here is a refleshed one...how do you think ?
>
> ==
>  From c0261936fc01322d06425731d33f38b2021e8067 Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
> Date: Mon, 12 Dec 2011 18:31:19 +0900
> Subject: [PATCH] per thread vma cache.
>
> This is a toy patch. How do you think ?
>
> This is a patch for per-thread mmap_cache without heavy atomic ops.
>
> I'm sure overhead of find_vma() is pretty small in usual application
> and this will not show good improvement. But I think, if we need
> to have cache of vma, it should be per thread rather than per mm.

Agreed. per-thread is better.


> This patch adds thread->mmap_cache, a pointer for vm_area_struct
> and update it appropriately. Because we have no refcnt on vm_area_struct,
> thread->mmap_cache may be a stale pointer. This patch detects stale
> pointer by checking
>
>      - thread->mmap_cache is one of SLABs in vm_area_cachep.
>      - thread->mmap_cache->vm_mm == mm.
>
> vma->vm_mm will be cleared before kmem_cache_free() by this patch.

Do you mean the cache can make mishit with unrelated vma when freed vma 
was reused?
If so, it is most tricky part of this patch, I strongly hope you write
a comment more.

Thank you.

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

Re: [PATCH v3] mm: simplify find_vma_prev

[KAMEZAWA Hiroyuki]

On Mon, 12 Dec 2011 10:31:57 -0500
KOSAKI Motohiro <kosaki.motohiro@gmail.com> wrote:

> (12/12/11 4:27 AM), KAMEZAWA Hiroyuki wrote:
> > On Mon, 12 Dec 2011 09:49:30 +0900
> > KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>  wrote:
> >
> >> On Fri,  9 Dec 2011 17:48:40 -0500
> >> kosaki.motohiro@gmail.com wrote:
> >>
> >>> From: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
> >>>
> >>> commit 297c5eee37 (mm: make the vma list be doubly linked) added
> >>> vm_prev member into vm_area_struct. Therefore we can simplify
> >>> find_vma_prev() by using it. Also, this change help to improve
> >>> page fault performance because it has strong locality of reference.
> >>>
> >>> Signed-off-by: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
> >>
> >> Reviewed-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
> >>
> >
> > Hmm, your work remind me of a patch I tried in past.
> > Here is a refleshed one...how do you think ?
> >
> > ==
> >  From c0261936fc01322d06425731d33f38b2021e8067 Mon Sep 17 00:00:00 2001
> > From: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
> > Date: Mon, 12 Dec 2011 18:31:19 +0900
> > Subject: [PATCH] per thread vma cache.
> >
> > This is a toy patch. How do you think ?
> >
> > This is a patch for per-thread mmap_cache without heavy atomic ops.
> >
> > I'm sure overhead of find_vma() is pretty small in usual application
> > and this will not show good improvement. But I think, if we need
> > to have cache of vma, it should be per thread rather than per mm.
> 
> Agreed. per-thread is better.
> 
> 
> > This patch adds thread->mmap_cache, a pointer for vm_area_struct
> > and update it appropriately. Because we have no refcnt on vm_area_struct,
> > thread->mmap_cache may be a stale pointer. This patch detects stale
> > pointer by checking
> >
> >      - thread->mmap_cache is one of SLABs in vm_area_cachep.
> >      - thread->mmap_cache->vm_mm == mm.
> >
> > vma->vm_mm will be cleared before kmem_cache_free() by this patch.
> 
> Do you mean the cache can make mishit with unrelated vma when freed vma 
> was reused?

yes.

> If so, it is most tricky part of this patch, I strongly hope you write
> a comment more.
> 

Sure.
-Kame




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

Re: [PATCH v3] mm: simplify find_vma_prev

[Michal Hocko]

On Fri 09-12-11 17:48:40, kosaki.motohiro@gmail.com wrote:
> From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> 
> commit 297c5eee37 (mm: make the vma list be doubly linked) added
> vm_prev member into vm_area_struct. Therefore we can simplify
> find_vma_prev() by using it. Also, this change help to improve
> page fault performance because it has strong locality of reference.
> 
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> ---
>  mm/mmap.c |   36 ++++++++----------------------------
>  1 files changed, 8 insertions(+), 28 deletions(-)
> 
> diff --git a/mm/mmap.c b/mm/mmap.c
> index eae90af..b9c0241 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1603,39 +1603,19 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
>  
>  EXPORT_SYMBOL(find_vma);
>  
> -/* Same as find_vma, but also return a pointer to the previous VMA in *pprev. */
> +/*
> + * Same as find_vma, but also return a pointer to the previous VMA in *pprev.
> + * Note: pprev is set to NULL when return value is NULL.
> + */
>  struct vm_area_struct *
>  find_vma_prev(struct mm_struct *mm, unsigned long addr,
>  			struct vm_area_struct **pprev)
>  {
> -	struct vm_area_struct *vma = NULL, *prev = NULL;
> -	struct rb_node *rb_node;
> -	if (!mm)
> -		goto out;
> -
> -	/* Guard against addr being lower than the first VMA */
> -	vma = mm->mmap;

Why have you removed this guard? Previously we had pprev==NULL and
returned mm->mmap.
This seems like a semantic change without any explanation. Could you
clarify?

> -
> -	/* Go through the RB tree quickly. */
> -	rb_node = mm->mm_rb.rb_node;
> -
> -	while (rb_node) {
> -		struct vm_area_struct *vma_tmp;
> -		vma_tmp = rb_entry(rb_node, struct vm_area_struct, vm_rb);
> -
> -		if (addr < vma_tmp->vm_end) {
> -			rb_node = rb_node->rb_left;
> -		} else {
> -			prev = vma_tmp;
> -			if (!prev->vm_next || (addr < prev->vm_next->vm_end))
> -				break;
> -			rb_node = rb_node->rb_right;
> -		}
> -	}
> +	struct vm_area_struct *vma;
>  
> -out:
> -	*pprev = prev;
> -	return prev ? prev->vm_next : vma;
> +	vma = find_vma(mm, addr);
> +	*pprev = vma ? vma->vm_prev : NULL;
> +	return vma;
>  }
>  
>  /*
> -- 
> 1.7.1
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

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

Re: [PATCH v3] mm: simplify find_vma_prev

[KOSAKI Motohiro]

(12/12/11 8:26 AM), Michal Hocko wrote:
> On Fri 09-12-11 17:48:40, kosaki.motohiro@gmail.com wrote:
>> From: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
>>
>> commit 297c5eee37 (mm: make the vma list be doubly linked) added
>> vm_prev member into vm_area_struct. Therefore we can simplify
>> find_vma_prev() by using it. Also, this change help to improve
>> page fault performance because it has strong locality of reference.
>>
>> Signed-off-by: KOSAKI Motohiro<kosaki.motohiro@jp.fujitsu.com>
>> ---
>>   mm/mmap.c |   36 ++++++++----------------------------
>>   1 files changed, 8 insertions(+), 28 deletions(-)
>>
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index eae90af..b9c0241 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -1603,39 +1603,19 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
>>
>>   EXPORT_SYMBOL(find_vma);
>>
>> -/* Same as find_vma, but also return a pointer to the previous VMA in *pprev. */
>> +/*
>> + * Same as find_vma, but also return a pointer to the previous VMA in *pprev.
>> + * Note: pprev is set to NULL when return value is NULL.
>> + */
>>   struct vm_area_struct *
>>   find_vma_prev(struct mm_struct *mm, unsigned long addr,
>>   			struct vm_area_struct **pprev)
>>   {
>> -	struct vm_area_struct *vma = NULL, *prev = NULL;
>> -	struct rb_node *rb_node;
>> -	if (!mm)
>> -		goto out;
>> -
>> -	/* Guard against addr being lower than the first VMA */
>> -	vma = mm->mmap;
>
> Why have you removed this guard? Previously we had pprev==NULL and
> returned mm->mmap.
> This seems like a semantic change without any explanation. Could you
> clarify?

IIUC, find_vma_prev() is module unexported and none of known caller use
pprev==NULL. Thus, I thought it can be also simplified. Am I missing 
something?

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

Re: [PATCH v3] mm: simplify find_vma_prev

[Michal Hocko]

On Mon 12-12-11 14:26:16, Michal Hocko wrote:
> On Fri 09-12-11 17:48:40, kosaki.motohiro@gmail.com wrote:
> > From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > 
> > commit 297c5eee37 (mm: make the vma list be doubly linked) added
> > vm_prev member into vm_area_struct. Therefore we can simplify
> > find_vma_prev() by using it. Also, this change help to improve
> > page fault performance because it has strong locality of reference.
> > 
> > Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> > ---
> >  mm/mmap.c |   36 ++++++++----------------------------
> >  1 files changed, 8 insertions(+), 28 deletions(-)
> > 
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index eae90af..b9c0241 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -1603,39 +1603,19 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
> >  
> >  EXPORT_SYMBOL(find_vma);
> >  
> > -/* Same as find_vma, but also return a pointer to the previous VMA in *pprev. */
> > +/*
> > + * Same as find_vma, but also return a pointer to the previous VMA in *pprev.
> > + * Note: pprev is set to NULL when return value is NULL.
> > + */
> >  struct vm_area_struct *
> >  find_vma_prev(struct mm_struct *mm, unsigned long addr,
> >  			struct vm_area_struct **pprev)
> >  {
> > -	struct vm_area_struct *vma = NULL, *prev = NULL;
> > -	struct rb_node *rb_node;
> > -	if (!mm)
> > -		goto out;
> > -
> > -	/* Guard against addr being lower than the first VMA */
> > -	vma = mm->mmap;
> 
> Why have you removed this guard? Previously we had pprev==NULL and
> returned mm->mmap.
> This seems like a semantic change without any explanation. Could you
> clarify?

Scratch that. I have misread the code. find_vma will return mm->mmap if
the given address is bellow all vmas. Sorry about noise.

The only concern left would be the caching. Are you sure this will not
break some workloads which benefit from mmap_cache usage and would
interfere with find_vma_prev callers now? Anyway this could be fixed
trivially.

Thanks
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

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

Re: [PATCH v3] mm: simplify find_vma_prev

[KOSAKI Motohiro]

>> Why have you removed this guard? Previously we had pprev==NULL and
>> returned mm->mmap.
>> This seems like a semantic change without any explanation. Could you
>> clarify?
>
> Scratch that. I have misread the code. find_vma will return mm->mmap if
> the given address is bellow all vmas. Sorry about noise.
>
> The only concern left would be the caching. Are you sure this will not
> break some workloads which benefit from mmap_cache usage and would
> interfere with find_vma_prev callers now? Anyway this could be fixed
> trivially.

Here is callers list.

find_vma_prev     115 arch/ia64/mm/fault.c      vma =
find_vma_prev(mm, address, &prev_vma);
find_vma_prev     183 arch/parisc/mm/fault.c    vma =
find_vma_prev(mm, address, &prev_vma);
find_vma_prev     229 arch/tile/mm/hugetlbpage.c                vma =
find_vma_prev(mm, addr, &prev_vma);
find_vma_prev     336 arch/x86/mm/hugetlbpage.c                 if
(!(vma = find_vma_prev(mm, addr, &prev_vma)))
find_vma_prev     388 mm/madvise.c      vma =
find_vma_prev(current->mm, start, &prev);
find_vma_prev     642 mm/mempolicy.c    vma = find_vma_prev(mm, start, &prev);
find_vma_prev     388 mm/mlock.c        vma =
find_vma_prev(current->mm, start, &prev);
find_vma_prev     265 mm/mprotect.c     vma =
find_vma_prev(current->mm, start, &prev);

In short, find_find_prev() is only used from page fault, madvise, mbind, mlock
and mprotect. And page fault is only performance impact callsite because other
don't used frequently on regular workload.

So, I wouldn't say, this patch has zero negative impact, but I think
it is enough
small and benefit is enough much.

Thanks.

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

Re: [PATCH v3] mm: simplify find_vma_prev

[KOSAKI Motohiro]

(12/12/11 2:24 PM), KOSAKI Motohiro wrote:
>>> Why have you removed this guard? Previously we had pprev==NULL and
>>> returned mm->mmap.
>>> This seems like a semantic change without any explanation. Could you
>>> clarify?
>>
>> Scratch that. I have misread the code. find_vma will return mm->mmap if
>> the given address is bellow all vmas. Sorry about noise.
>>
>> The only concern left would be the caching. Are you sure this will not
>> break some workloads which benefit from mmap_cache usage and would
>> interfere with find_vma_prev callers now? Anyway this could be fixed
>> trivially.
>
> Here is callers list.
>
> find_vma_prev     115 arch/ia64/mm/fault.c      vma =
> find_vma_prev(mm, address,&prev_vma);
> find_vma_prev     183 arch/parisc/mm/fault.c    vma =
> find_vma_prev(mm, address,&prev_vma);
> find_vma_prev     229 arch/tile/mm/hugetlbpage.c                vma =
> find_vma_prev(mm, addr,&prev_vma);
> find_vma_prev     336 arch/x86/mm/hugetlbpage.c                 if
> (!(vma = find_vma_prev(mm, addr,&prev_vma)))
> find_vma_prev     388 mm/madvise.c      vma =
> find_vma_prev(current->mm, start,&prev);
> find_vma_prev     642 mm/mempolicy.c    vma = find_vma_prev(mm, start,&prev);
> find_vma_prev     388 mm/mlock.c        vma =
> find_vma_prev(current->mm, start,&prev);
> find_vma_prev     265 mm/mprotect.c     vma =
> find_vma_prev(current->mm, start,&prev);
>
> In short, find_find_prev() is only used from page fault, madvise, mbind, mlock
> and mprotect. And page fault is only performance impact callsite because other
> don't used frequently on regular workload.
>
> So, I wouldn't say, this patch has zero negative impact, but I think
> it is enough
> small and benefit is enough much.

In addition, other callsite (i.e. madvise, mbind, mlock and mprotect) are
used from syscall. then, an optimal behavior depend on syscall argument.
IOW, we and the kernel can't know it on ahead.

Therefore, this change may increase some applications performance a bit 
and may decrease another some applications. I can reasonably guess the
former are much than latter because many app have locality. but I can't
prove it.

Anyway, the impact is enough small, I think. They are rare than page fault.

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

Re: [PATCH v2] mm: simplify find_vma_prev

[KOSAKI Motohiro]

>> diff --git a/mm/mmap.c b/mm/mmap.c
> []
>> @@ -1603,39 +1603,21 @@ struct vm_area_struct *find_vma(struct mm_struct *mm, unsigned long addr)
>>
>>  EXPORT_SYMBOL(find_vma);
>>
>> -/* Same as find_vma, but also return a pointer to the previous VMA in *pprev. */
>> +/*
>> + * Same as find_vma, but also return a pointer to the previous VMA in *pprev.
>> + * Note: pprev is set to NULL when return value is NULL.
>> + */
>>  struct vm_area_struct *
>> -find_vma_prev(struct mm_struct *mm, unsigned long addr,
>> -                     struct vm_area_struct **pprev)
>
>> +find_vma_prev(struct mm_struct *mm, unsigned long addr, struct vm_area_struct **pprev)
>
> eh.  This declaration change seems gratuitous and it exceeds 80 columns.
>
>> +     *pprev = NULL;
>> +     vma = find_vma(mm, addr);
>> +     if (vma)
>> +             *pprev = vma->vm_prev;
>
> There's no need to possibly set *pprev twice.
>
> Maybe
> {
>        struct vm_area_struct *vma = find_vma(mm, addr);
>
>        *pprev = vma ? vma->vm_prev : NULL;
> or
>        if (vma)
>                *pprev = vma->vm_prev;
>        else
>                *pprev = NULL;
>
>        return vma;

Thank you for reviewing. Updated.

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