[patch] slub: fix a code merge error

[patch] slub: fix a code merge error

[Shaohua Li]

Looks there is a merge error in the slub tree. DEACTIVATE_TO_TAIL != 1.
And this will cause performance regression.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>

diff --git a/mm/slub.c b/mm/slub.c
index 7d2a996..60e16c4 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1904,7 +1904,8 @@ static void unfreeze_partials(struct kmem_cache *s)
 				if (l == M_PARTIAL)
 					remove_partial(n, page);
 				else
-					add_partial(n, page, 1);
+					add_partial(n, page,
+						DEACTIVATE_TO_TAIL);
 
 				l = m;
 			}


--
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] slub: fix a code merge error

[Christoph Lameter]

On Thu, 10 Nov 2011, Shaohua Li wrote:

> Looks there is a merge error in the slub tree. DEACTIVATE_TO_TAIL != 1.
> And this will cause performance regression.

Thanks.

Acked-by: Christoph Lameter <cl@linux.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] slub: fix a code merge error

[David Rientjes]

On Thu, 10 Nov 2011, Shaohua Li wrote:

> Looks there is a merge error in the slub tree. DEACTIVATE_TO_TAIL != 1.
> And this will cause performance regression.
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 7d2a996..60e16c4 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -1904,7 +1904,8 @@ static void unfreeze_partials(struct kmem_cache *s)
>  				if (l == M_PARTIAL)
>  					remove_partial(n, page);
>  				else
> -					add_partial(n, page, 1);
> +					add_partial(n, page,
> +						DEACTIVATE_TO_TAIL);
>  
>  				l = m;
>  			}

Acked-by: David Rientjes <rientjes@google.com>

Not sure where the "merge error" is, though, this is how it was proposed 
on linux-mm each time the patch was posted.  Probably needs a better title 
and changelog.

--
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] slub: fix a code merge error

[Pekka Enberg]

On Thu, Nov 10, 2011 at 10:18 PM, David Rientjes <rientjes@google.com> wrote:
> On Thu, 10 Nov 2011, Shaohua Li wrote:
>
>> Looks there is a merge error in the slub tree. DEACTIVATE_TO_TAIL != 1.
>> And this will cause performance regression.
>>
>> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 7d2a996..60e16c4 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -1904,7 +1904,8 @@ static void unfreeze_partials(struct kmem_cache *s)
>>                               if (l == M_PARTIAL)
>>                                       remove_partial(n, page);
>>                               else
>> -                                     add_partial(n, page, 1);
>> +                                     add_partial(n, page,
>> +                                             DEACTIVATE_TO_TAIL);
>>
>>                               l = m;
>>                       }
>
> Acked-by: David Rientjes <rientjes@google.com>
>
> Not sure where the "merge error" is, though, this is how it was proposed
> on linux-mm each time the patch was posted.  Probably needs a better title
> and changelog.

Indeed. Please resend with proper subject and changelog with
Christoph's and David's ACKs included.

--
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] slub: fix a code merge error

[Shaohua Li]

On Fri, 2011-11-11 at 04:30 +0800, Pekka Enberg wrote:
> On Thu, Nov 10, 2011 at 10:18 PM, David Rientjes <rientjes@google.com> wrote:
> > On Thu, 10 Nov 2011, Shaohua Li wrote:
> >
> >> Looks there is a merge error in the slub tree. DEACTIVATE_TO_TAIL != 1.
> >> And this will cause performance regression.
> >>
> >> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> >>
> >> diff --git a/mm/slub.c b/mm/slub.c
> >> index 7d2a996..60e16c4 100644
> >> --- a/mm/slub.c
> >> +++ b/mm/slub.c
> >> @@ -1904,7 +1904,8 @@ static void unfreeze_partials(struct kmem_cache *s)
> >>                               if (l == M_PARTIAL)
> >>                                       remove_partial(n, page);
> >>                               else
> >> -                                     add_partial(n, page, 1);
> >> +                                     add_partial(n, page,
> >> +                                             DEACTIVATE_TO_TAIL);
> >>
> >>                               l = m;
> >>                       }
> >
> > Acked-by: David Rientjes <rientjes@google.com>
> >
> > Not sure where the "merge error" is, though, this is how it was proposed
> > on linux-mm each time the patch was posted.  Probably needs a better title
> > and changelog.
> 
> Indeed. Please resend with proper subject and changelog with
> Christoph's and David's ACKs included.

Subject: slub: use correct parameter to add a page to partial list tail

unfreeze_partials() needs add the page to partial list tail, since such page
hasn't too many free objects. We now explictly use DEACTIVATE_TO_TAIL for this,
while DEACTIVATE_TO_TAIL != 1. This will cause performance regression (eg, more
lock contention in node->list_lock) without below fix.

Signed-off-by: Shaohua Li <shaohua.li@intel.com>
Acked-by: Christoph Lameter <cl@linux.com>
Acked-by: David Rientjes <rientjes@google.com>

diff --git a/mm/slub.c b/mm/slub.c
index 7d2a996..60e16c4 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1904,7 +1904,8 @@ static void unfreeze_partials(struct kmem_cache *s)
 				if (l == M_PARTIAL)
 					remove_partial(n, page);
 				else
-					add_partial(n, page, 1);
+					add_partial(n, page,
+						DEACTIVATE_TO_TAIL);
 
 				l = m;
 			}


--
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] slub: fix a code merge error

[alex shi]

>
> Indeed. Please resend with proper subject and changelog with
> Christoph's and David's ACKs included.

Pekka:

SLUB stat attribute was designed for stat accounting only. I checked
the total 24 attributes that used now. All of them used in stat() only
except the DEACTIVATE_TO_HEAD/TAIL.

And in fact, in the most of using scenarios the DEACTIVATE_TO_TAIL
make reader confuse, TO_TAIL is correct but not for DEACTIVATE.

Further more, CL also regretted this after he acked the original
patches for this attribute mis-usages. He said "don't think we want
this patch any more."
http://permalink.gmane.org/gmane.linux.kernel.mm/67653 and want to use
a comment instead of this confusing usage.
https://lkml.org/lkml/2011/8/29/187

So, as to this regression, from my viewpoint, reverting the
DEACTIVATE_TO_TAIL incorrect usage(commit 136333d104) is a better way.
 :)

--
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] slub: fix a code merge error

[Shaohua Li]

On Mon, 2011-11-14 at 09:56 +0800, alex shi wrote:
> >
> > Indeed. Please resend with proper subject and changelog with
> > Christoph's and David's ACKs included.
> 
> Pekka:
> 
> SLUB stat attribute was designed for stat accounting only. I checked
> the total 24 attributes that used now. All of them used in stat() only
> except the DEACTIVATE_TO_HEAD/TAIL.
it's an enum, it can be used in any case if proper used.

> And in fact, in the most of using scenarios the DEACTIVATE_TO_TAIL
> make reader confuse, TO_TAIL is correct but not for DEACTIVATE.
please look at the comments where DEACTIVATE_TO_TAIL is defined.

> Further more, CL also regretted this after he acked the original
> patches for this attribute mis-usages. He said "don't think we want
> this patch any more."
> http://permalink.gmane.org/gmane.linux.kernel.mm/67653 and want to use
> a comment instead of this confusing usage.
> https://lkml.org/lkml/2011/8/29/187
> 
> So, as to this regression, from my viewpoint, reverting the
> DEACTIVATE_TO_TAIL incorrect usage(commit 136333d104) is a better way.
>  :)
using 0/1 is insane and can easily cause problems. Using a name can help
avoid such insane. This is exactly what commit 136333d104 tries to do.
This is a general C programming skill everybody should already know.

--
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] slub: fix a code merge error

[Pekka Enberg]

>> Indeed. Please resend with proper subject and changelog with
>> Christoph's and David's ACKs included.

On Mon, 14 Nov 2011, alex shi wrote:
> SLUB stat attribute was designed for stat accounting only. I checked
> the total 24 attributes that used now. All of them used in stat() only
> except the DEACTIVATE_TO_HEAD/TAIL.
>
> And in fact, in the most of using scenarios the DEACTIVATE_TO_TAIL
> make reader confuse, TO_TAIL is correct but not for DEACTIVATE.
>
> Further more, CL also regretted this after he acked the original
> patches for this attribute mis-usages. He said "don't think we want
> this patch any more."
> http://permalink.gmane.org/gmane.linux.kernel.mm/67653 and want to use
> a comment instead of this confusing usage.
> https://lkml.org/lkml/2011/8/29/187
>
> So, as to this regression, from my viewpoint, reverting the
> DEACTIVATE_TO_TAIL incorrect usage(commit 136333d104) is a better way.
> :)

The enum is fine. I don't see any reason to revert the whole commit if 
Shaohua's patch fixes the problem.

 			Pekka

--
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] slub: fix a code merge error

[David Rientjes]

On Tue, 15 Nov 2011, Pekka Enberg wrote:

> > SLUB stat attribute was designed for stat accounting only. I checked
> > the total 24 attributes that used now. All of them used in stat() only
> > except the DEACTIVATE_TO_HEAD/TAIL.
> > 
> > And in fact, in the most of using scenarios the DEACTIVATE_TO_TAIL
> > make reader confuse, TO_TAIL is correct but not for DEACTIVATE.
> > 
> > Further more, CL also regretted this after he acked the original
> > patches for this attribute mis-usages. He said "don't think we want
> > this patch any more."
> > http://permalink.gmane.org/gmane.linux.kernel.mm/67653 and want to use
> > a comment instead of this confusing usage.
> > https://lkml.org/lkml/2011/8/29/187
> > 
> > So, as to this regression, from my viewpoint, reverting the
> > DEACTIVATE_TO_TAIL incorrect usage(commit 136333d104) is a better way.
> > :)
> 
> The enum is fine. I don't see any reason to revert the whole commit if
> Shaohua's patch fixes the problem.
> 

It's a slight optimization since "tail" can be set in deactivate_slab() 
and be passed to add_partial() without doing something like !!tail or 
converting it to a boolean as well as using it when calling stat().

--
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>