linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] slub: fix check_bytes() for slub debugging
@ 2011-08-07  9:30 Akinobu Mita
  2011-08-08 21:24 ` Marcin Slusarz
  2011-08-09  3:10 ` Eric Dumazet
  0 siblings, 2 replies; 10+ messages in thread
From: Akinobu Mita @ 2011-08-07  9:30 UTC (permalink / raw)
  To: linux-kernel
  Cc: Akinobu Mita, Christoph Lameter, Pekka Enberg, Matt Mackall,
	linux-mm

The check_bytes() function is used by slub debugging.  It returns a pointer
to the first unmatching byte for a character in the given memory area.

If the character for matching byte is greater than 0x80, check_bytes()
doesn't work.  Becuase 64-bit pattern is generated as below.

	value64 = value | value << 8 | value << 16 | value << 24;
	value64 = value64 | value64 << 32;

The integer promotions are performed and sign-extended as the type of value
is u8.  The upper 32 bits of value64 is 0xffffffff in the first line, and
the second line has no effect.

This fixes the 64-bit pattern generation.

Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
Cc: Christoph Lameter <cl@linux-foundation.org>
Cc: Pekka Enberg <penberg@kernel.org>
Cc: Matt Mackall <mpm@selenic.com>
Cc: linux-mm@kvack.org
---
 mm/slub.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index eb5a8f9..5695f92 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -701,7 +701,7 @@ static u8 *check_bytes(u8 *start, u8 value, unsigned int bytes)
 		return check_bytes8(start, value, bytes);
 
 	value64 = value | value << 8 | value << 16 | value << 24;
-	value64 = value64 | value64 << 32;
+	value64 = (value64 & 0xffffffff) | value64 << 32;
 	prefix = 8 - ((unsigned long)start) % 8;
 
 	if (prefix) {
-- 
1.7.4.4

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] slub: fix check_bytes() for slub debugging
  2011-08-07  9:30 [PATCH] slub: fix check_bytes() for slub debugging Akinobu Mita
@ 2011-08-08 21:24 ` Marcin Slusarz
  2011-08-09  1:08   ` Akinobu Mita
  2011-08-09  3:10 ` Eric Dumazet
  1 sibling, 1 reply; 10+ messages in thread
From: Marcin Slusarz @ 2011-08-08 21:24 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-kernel, Christoph Lameter, Pekka Enberg, Matt Mackall,
	linux-mm

On Sun, Aug 07, 2011 at 06:30:38PM +0900, Akinobu Mita wrote:
> The check_bytes() function is used by slub debugging.  It returns a pointer
> to the first unmatching byte for a character in the given memory area.
> 
> If the character for matching byte is greater than 0x80, check_bytes()
> doesn't work.  Becuase 64-bit pattern is generated as below.
> 
> 	value64 = value | value << 8 | value << 16 | value << 24;
> 	value64 = value64 | value64 << 32;
> 
> The integer promotions are performed and sign-extended as the type of value
> is u8.  The upper 32 bits of value64 is 0xffffffff in the first line, and
> the second line has no effect.
> 
> This fixes the 64-bit pattern generation.

Thank you. I'm a bit ashamed about this bug... I introduced this bug, so:
Reviewed-by: Marcin Slusarz <marcin.slusarz@gmail.com>

I tested your patch to check if performance improvements of commit
c4089f98e943ff445665dea49c190657b34ccffe come from this bug or not.
And forunately they aren't - performance is exactly the same.

How did you find it?

Marcin

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] slub: fix check_bytes() for slub debugging
  2011-08-08 21:24 ` Marcin Slusarz
@ 2011-08-09  1:08   ` Akinobu Mita
  0 siblings, 0 replies; 10+ messages in thread
From: Akinobu Mita @ 2011-08-09  1:08 UTC (permalink / raw)
  To: Marcin Slusarz
  Cc: linux-kernel, Christoph Lameter, Pekka Enberg, Matt Mackall,
	linux-mm

2011/8/9 Marcin Slusarz <marcin.slusarz@gmail.com>:

> I tested your patch to check if performance improvements of commit
> c4089f98e943ff445665dea49c190657b34ccffe come from this bug or not.
> And forunately they aren't - performance is exactly the same.

That's good to know.

> How did you find it?

When I tried to reuse it in mm/debug-pagealloc.c, I realized that
check_bytes() didn't work with 0xaa.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] slub: fix check_bytes() for slub debugging
  2011-08-07  9:30 [PATCH] slub: fix check_bytes() for slub debugging Akinobu Mita
  2011-08-08 21:24 ` Marcin Slusarz
@ 2011-08-09  3:10 ` Eric Dumazet
  2011-08-09  3:33   ` Eric Dumazet
  1 sibling, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2011-08-09  3:10 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-kernel, Christoph Lameter, Pekka Enberg, Matt Mackall,
	linux-mm

Le dimanche 07 aoA>>t 2011 A  18:30 +0900, Akinobu Mita a A(C)crit :
> The check_bytes() function is used by slub debugging.  It returns a pointer
> to the first unmatching byte for a character in the given memory area.
> 
> If the character for matching byte is greater than 0x80, check_bytes()
> doesn't work.  Becuase 64-bit pattern is generated as below.
> 
> 	value64 = value | value << 8 | value << 16 | value << 24;
> 	value64 = value64 | value64 << 32;
> 
> The integer promotions are performed and sign-extended as the type of value
> is u8.  The upper 32 bits of value64 is 0xffffffff in the first line, and
> the second line has no effect.
> 
> This fixes the 64-bit pattern generation.
> 
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Cc: Christoph Lameter <cl@linux-foundation.org>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: Matt Mackall <mpm@selenic.com>
> Cc: linux-mm@kvack.org
> ---
>  mm/slub.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index eb5a8f9..5695f92 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -701,7 +701,7 @@ static u8 *check_bytes(u8 *start, u8 value, unsigned int bytes)
>  		return check_bytes8(start, value, bytes);
>  
>  	value64 = value | value << 8 | value << 16 | value << 24;
> -	value64 = value64 | value64 << 32;
> +	value64 = (value64 & 0xffffffff) | value64 << 32;
>  	prefix = 8 - ((unsigned long)start) % 8;
>  
>  	if (prefix) {

Still buggy I am afraid. Could we use the following ?


	value64 = value;
	value64 |= value64 << 8;
	value64 |= value64 << 16;
	value64 |= value64 << 32;



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] slub: fix check_bytes() for slub debugging
  2011-08-09  3:10 ` Eric Dumazet
@ 2011-08-09  3:33   ` Eric Dumazet
  2011-08-09  9:38     ` Akinobu Mita
  0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2011-08-09  3:33 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-kernel, Christoph Lameter, Pekka Enberg, Matt Mackall,
	linux-mm

Le mardi 09 aoA>>t 2011 A  05:10 +0200, Eric Dumazet a A(C)crit :
> Le dimanche 07 aoA>>t 2011 A  18:30 +0900, Akinobu Mita a A(C)crit :
> > The check_bytes() function is used by slub debugging.  It returns a pointer
> > to the first unmatching byte for a character in the given memory area.
> > 
> > If the character for matching byte is greater than 0x80, check_bytes()
> > doesn't work.  Becuase 64-bit pattern is generated as below.
> > 
> > 	value64 = value | value << 8 | value << 16 | value << 24;
> > 	value64 = value64 | value64 << 32;
> > 
> > The integer promotions are performed and sign-extended as the type of value
> > is u8.  The upper 32 bits of value64 is 0xffffffff in the first line, and
> > the second line has no effect.
> > 
> > This fixes the 64-bit pattern generation.
> > 
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > Cc: Christoph Lameter <cl@linux-foundation.org>
> > Cc: Pekka Enberg <penberg@kernel.org>
> > Cc: Matt Mackall <mpm@selenic.com>
> > Cc: linux-mm@kvack.org
> > ---
> >  mm/slub.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/mm/slub.c b/mm/slub.c
> > index eb5a8f9..5695f92 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -701,7 +701,7 @@ static u8 *check_bytes(u8 *start, u8 value, unsigned int bytes)
> >  		return check_bytes8(start, value, bytes);
> >  
> >  	value64 = value | value << 8 | value << 16 | value << 24;
> > -	value64 = value64 | value64 << 32;
> > +	value64 = (value64 & 0xffffffff) | value64 << 32;
> >  	prefix = 8 - ((unsigned long)start) % 8;
> >  
> >  	if (prefix) {
> 
> Still buggy I am afraid. Could we use the following ?
> 
> 
> 	value64 = value;
> 	value64 |= value64 << 8;
> 	value64 |= value64 << 16;
> 	value64 |= value64 << 32;
> 
> 

Well, 'buggy' was not well chosen.

Another possibility would be to use a multiply if arch has a fast
multiplier...


	value64 = value;
#if defined(ARCH_HAS_FAST_MULTIPLIER) && BITS_PER_LONG == 64
	value64 *= 0x0101010101010101;
#elif defined(ARCH_HAS_FAST_MULTIPLIER)
	value64 *= 0x01010101;
	value64 |= value64 << 32;
#else
	value64 |= value64 << 8;
	value64 |= value64 << 16;
	value64 |= value64 << 32;
#endif




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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] slub: fix check_bytes() for slub debugging
  2011-08-09  3:33   ` Eric Dumazet
@ 2011-08-09  9:38     ` Akinobu Mita
  2011-08-09  9:43       ` Pekka Enberg
  2011-08-09  9:46       ` Eric Dumazet
  0 siblings, 2 replies; 10+ messages in thread
From: Akinobu Mita @ 2011-08-09  9:38 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: linux-kernel, Christoph Lameter, Pekka Enberg, Matt Mackall,
	linux-mm

2011/8/9 Eric Dumazet <eric.dumazet@gmail.com>:

>> > diff --git a/mm/slub.c b/mm/slub.c
>> > index eb5a8f9..5695f92 100644
>> > --- a/mm/slub.c
>> > +++ b/mm/slub.c
>> > @@ -701,7 +701,7 @@ static u8 *check_bytes(u8 *start, u8 value, unsigned int bytes)
>> >             return check_bytes8(start, value, bytes);
>> >
>> >     value64 = value | value << 8 | value << 16 | value << 24;
>> > -   value64 = value64 | value64 << 32;
>> > +   value64 = (value64 & 0xffffffff) | value64 << 32;
>> >     prefix = 8 - ((unsigned long)start) % 8;
>> >
>> >     if (prefix) {
>>
>> Still buggy I am afraid. Could we use the following ?
>>
>>
>>       value64 = value;
>>       value64 |= value64 << 8;
>>       value64 |= value64 << 16;
>>       value64 |= value64 << 32;
>>
>>
>
> Well, 'buggy' was not well chosen.
>
> Another possibility would be to use a multiply if arch has a fast
> multiplier...
>
>
>        value64 = value;
> #if defined(ARCH_HAS_FAST_MULTIPLIER) && BITS_PER_LONG == 64
>        value64 *= 0x0101010101010101;
> #elif defined(ARCH_HAS_FAST_MULTIPLIER)
>        value64 *= 0x01010101;
>        value64 |= value64 << 32;
> #else
>        value64 |= value64 << 8;
>        value64 |= value64 << 16;
>        value64 |= value64 << 32;
> #endif

I don't really care about which one should be used.  So tell me if I need
to resend it with this improvement.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] slub: fix check_bytes() for slub debugging
  2011-08-09  9:38     ` Akinobu Mita
@ 2011-08-09  9:43       ` Pekka Enberg
  2011-08-09  9:54         ` Eric Dumazet
  2011-08-09  9:46       ` Eric Dumazet
  1 sibling, 1 reply; 10+ messages in thread
From: Pekka Enberg @ 2011-08-09  9:43 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: Eric Dumazet, linux-kernel, Christoph Lameter, Matt Mackall,
	linux-mm

On Tue, Aug 9, 2011 at 12:38 PM, Akinobu Mita <akinobu.mita@gmail.com> wrote:
> 2011/8/9 Eric Dumazet <eric.dumazet@gmail.com>:
>
>>> > diff --git a/mm/slub.c b/mm/slub.c
>>> > index eb5a8f9..5695f92 100644
>>> > --- a/mm/slub.c
>>> > +++ b/mm/slub.c
>>> > @@ -701,7 +701,7 @@ static u8 *check_bytes(u8 *start, u8 value, unsigned int bytes)
>>> >             return check_bytes8(start, value, bytes);
>>> >
>>> >     value64 = value | value << 8 | value << 16 | value << 24;
>>> > -   value64 = value64 | value64 << 32;
>>> > +   value64 = (value64 & 0xffffffff) | value64 << 32;
>>> >     prefix = 8 - ((unsigned long)start) % 8;
>>> >
>>> >     if (prefix) {
>>>
>>> Still buggy I am afraid. Could we use the following ?
>>>
>>>
>>>       value64 = value;
>>>       value64 |= value64 << 8;
>>>       value64 |= value64 << 16;
>>>       value64 |= value64 << 32;
>>>
>>>
>>
>> Well, 'buggy' was not well chosen.
>>
>> Another possibility would be to use a multiply if arch has a fast
>> multiplier...
>>
>>
>>        value64 = value;
>> #if defined(ARCH_HAS_FAST_MULTIPLIER) && BITS_PER_LONG == 64
>>        value64 *= 0x0101010101010101;
>> #elif defined(ARCH_HAS_FAST_MULTIPLIER)
>>        value64 *= 0x01010101;
>>        value64 |= value64 << 32;
>> #else
>>        value64 |= value64 << 8;
>>        value64 |= value64 << 16;
>>        value64 |= value64 << 32;
>> #endif
>
> I don't really care about which one should be used.  So tell me if I need
> to resend it with this improvement.

I'm confused. What was wrong with your original patch?

                        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>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] slub: fix check_bytes() for slub debugging
  2011-08-09  9:38     ` Akinobu Mita
  2011-08-09  9:43       ` Pekka Enberg
@ 2011-08-09  9:46       ` Eric Dumazet
  2011-08-09 21:46         ` Marcin Slusarz
  1 sibling, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2011-08-09  9:46 UTC (permalink / raw)
  To: Akinobu Mita
  Cc: linux-kernel, Christoph Lameter, Pekka Enberg, Matt Mackall,
	linux-mm

Le mardi 09 aoA>>t 2011 A  18:38 +0900, Akinobu Mita a A(C)crit :
> 2011/8/9 Eric Dumazet <eric.dumazet@gmail.com>:
> 
> >> > diff --git a/mm/slub.c b/mm/slub.c
> >> > index eb5a8f9..5695f92 100644
> >> > --- a/mm/slub.c
> >> > +++ b/mm/slub.c
> >> > @@ -701,7 +701,7 @@ static u8 *check_bytes(u8 *start, u8 value, unsigned int bytes)
> >> >             return check_bytes8(start, value, bytes);
> >> >
> >> >     value64 = value | value << 8 | value << 16 | value << 24;
> >> > -   value64 = value64 | value64 << 32;
> >> > +   value64 = (value64 & 0xffffffff) | value64 << 32;
> >> >     prefix = 8 - ((unsigned long)start) % 8;
> >> >
> >> >     if (prefix) {
> >>
> >> Still buggy I am afraid. Could we use the following ?
> >>
> >>
> >>       value64 = value;
> >>       value64 |= value64 << 8;
> >>       value64 |= value64 << 16;
> >>       value64 |= value64 << 32;
> >>
> >>
> >
> > Well, 'buggy' was not well chosen.
> >
> > Another possibility would be to use a multiply if arch has a fast
> > multiplier...
> >
> >
> >        value64 = value;
> > #if defined(ARCH_HAS_FAST_MULTIPLIER) && BITS_PER_LONG == 64
> >        value64 *= 0x0101010101010101;
> > #elif defined(ARCH_HAS_FAST_MULTIPLIER)
> >        value64 *= 0x01010101;
> >        value64 |= value64 << 32;
> > #else
> >        value64 |= value64 << 8;
> >        value64 |= value64 << 16;
> >        value64 |= value64 << 32;
> > #endif
> 
> I don't really care about which one should be used.  So tell me if I need
> to resend it with this improvement.

It would be nice to fix all bugs while we review this code.

Lets push your patch and I'll submit a patch for next kernel.

For example, following code is suboptimal :

        prefix = 8 - ((unsigned long)start) % 8;

        if (prefix) {
                u8 *r = check_bytes8(start, value, prefix);
                if (r)
                        return r;
                start += prefix;
                bytes -= prefix;
        }


Since we always have prefix = 8 if 'start' is longword aligned, so we
call check_bytes8() at least once with 8 bytes to compare...

Also, 32bit arches should be taken into account properly.

Oh well...



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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] slub: fix check_bytes() for slub debugging
  2011-08-09  9:43       ` Pekka Enberg
@ 2011-08-09  9:54         ` Eric Dumazet
  0 siblings, 0 replies; 10+ messages in thread
From: Eric Dumazet @ 2011-08-09  9:54 UTC (permalink / raw)
  To: Pekka Enberg
  Cc: Akinobu Mita, linux-kernel, Christoph Lameter, Matt Mackall,
	linux-mm

Le mardi 09 aoA>>t 2011 A  12:43 +0300, Pekka Enberg a A(C)crit :

> I'm confused. What was wrong with your original patch?
> 

Patch is good. Some future improvements will follow.


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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] slub: fix check_bytes() for slub debugging
  2011-08-09  9:46       ` Eric Dumazet
@ 2011-08-09 21:46         ` Marcin Slusarz
  0 siblings, 0 replies; 10+ messages in thread
From: Marcin Slusarz @ 2011-08-09 21:46 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Akinobu Mita, linux-kernel, Christoph Lameter, Pekka Enberg,
	Matt Mackall, linux-mm

On Tue, Aug 09, 2011 at 11:46:09AM +0200, Eric Dumazet wrote:
> Le mardi 09 aoA>>t 2011 A  18:38 +0900, Akinobu Mita a A(C)crit :
> > 2011/8/9 Eric Dumazet <eric.dumazet@gmail.com>:
> > 
> > >> > diff --git a/mm/slub.c b/mm/slub.c
> > >> > index eb5a8f9..5695f92 100644
> > >> > --- a/mm/slub.c
> > >> > +++ b/mm/slub.c
> > >> > @@ -701,7 +701,7 @@ static u8 *check_bytes(u8 *start, u8 value, unsigned int bytes)
> > >> >             return check_bytes8(start, value, bytes);
> > >> >
> > >> >     value64 = value | value << 8 | value << 16 | value << 24;
> > >> > -   value64 = value64 | value64 << 32;
> > >> > +   value64 = (value64 & 0xffffffff) | value64 << 32;
> > >> >     prefix = 8 - ((unsigned long)start) % 8;
> > >> >
> > >> >     if (prefix) {
> > >>
> > >> Still buggy I am afraid. Could we use the following ?
> > >>
> > >>
> > >>       value64 = value;
> > >>       value64 |= value64 << 8;
> > >>       value64 |= value64 << 16;
> > >>       value64 |= value64 << 32;
> > >>
> > >>
> > >
> > > Well, 'buggy' was not well chosen.
> > >
> > > Another possibility would be to use a multiply if arch has a fast
> > > multiplier...
> > >
> > >
> > >        value64 = value;
> > > #if defined(ARCH_HAS_FAST_MULTIPLIER) && BITS_PER_LONG == 64
> > >        value64 *= 0x0101010101010101;
> > > #elif defined(ARCH_HAS_FAST_MULTIPLIER)
> > >        value64 *= 0x01010101;
> > >        value64 |= value64 << 32;
> > > #else
> > >        value64 |= value64 << 8;
> > >        value64 |= value64 << 16;
> > >        value64 |= value64 << 32;
> > > #endif
> > 
> > I don't really care about which one should be used.  So tell me if I need
> > to resend it with this improvement.
> 
> It would be nice to fix all bugs while we review this code.
> 
> Lets push your patch and I'll submit a patch for next kernel.
> 
> For example, following code is suboptimal :
> 
>         prefix = 8 - ((unsigned long)start) % 8;
> 
>         if (prefix) {
>                 u8 *r = check_bytes8(start, value, prefix);
>                 if (r)
>                         return r;
>                 start += prefix;
>                 bytes -= prefix;
>         }
> 
> 
> Since we always have prefix = 8 if 'start' is longword aligned, so we
> call check_bytes8() at least once with 8 bytes to compare...

Yeah. 
 
> Also, 32bit arches should be taken into account properly.

At least on x86_32 reading 8 bytes is faster than 4 (I benchmarked it - IIRC
reading 8 bytes speeds up by a factor of ~5 and reading 4 only by ~3.5) 


Marcin

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2011-08-09 21:46 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-08-07  9:30 [PATCH] slub: fix check_bytes() for slub debugging Akinobu Mita
2011-08-08 21:24 ` Marcin Slusarz
2011-08-09  1:08   ` Akinobu Mita
2011-08-09  3:10 ` Eric Dumazet
2011-08-09  3:33   ` Eric Dumazet
2011-08-09  9:38     ` Akinobu Mita
2011-08-09  9:43       ` Pekka Enberg
2011-08-09  9:54         ` Eric Dumazet
2011-08-09  9:46       ` Eric Dumazet
2011-08-09 21:46         ` Marcin Slusarz

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).