Sparc32: random invalid instruction occourances on sparc32 (sun4c)

Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[Mark Fortescue]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2404 bytes --]

Hi all,

I think I have found the cause of the problem.

Commit b46b8f19c9cd435ecac4d9d12b39d78c137ecd66 partially fixed alignment 
issues but does not ensure that all 64bit alignment requirements of 
sparc32 are met. Tests have shown that the redzone2 word can become 
misallignd.

I am currently working on a posible fix.

Regards
 	Mark Fortescue.

On Tue, 3 Jul 2007, Michal Piotrowski wrote:

> Hi all,
>
> Here is a list of some known regressions in 2.6.22-rc7.
>
> Feel free to add new regressions/remove fixed etc.
> http://kernelnewbies.org/known_regressions
>
> List of Aces
>
> Name                    Regressions fixed since 21-Jun-2007
> Hugh Dickins                           2
> Andi Kleen                             1
> Andrew Morton                          1
> Benjamin Herrenschmidt                 1
> Björn Steinbrink                       1
> Bjorn Helgaas                          1
> Jean Delvare                           1
> Olaf Hering                            1
> Siddha, Suresh B                       1
> Trent Piepho                           1
> Ville Syrjälä                          1
>
>
>
> FS
>
> Subject    : 2.6.22-rc4-git5 reiserfs: null ptr deref.
> References : http://lkml.org/lkml/2007/6/13/322
> Submitter  : Randy Dunlap <randy.dunlap@oracle.com>
> Handled-By : Vladimir V. Saveliev <vs@namesys.com>
> Status     : problem is being debugged
>
>
>
> IDE
>
> Subject    : 2.6.22-rcX: hda: lost interrupt
> References : http://lkml.org/lkml/2007/6/29/121
> Submitter  : David Chinner <dgc@sgi.com>
> Status     : unknown
>
>
>
> Sparc64
>
> Subject    : random invalid instruction occourances on sparc32 (sun4c)
> References : http://lkml.org/lkml/2007/6/17/111
> Submitter  : Mark Fortescue <mark@mtfhpc.demon.co.uk>
> Status     : problem is being debugged
>
> Subject    : 2.6.22-rc broke X on Ultra5
> References : http://lkml.org/lkml/2007/5/22/78
> Submitter  : Mikael Pettersson <mikpe@it.uu.se>
> Handled-By : David Miller <davem@davemloft.net>
> Status     : problem is being debugged
>
>
>
> Regards,
> Michal
>
> --
> LOG
> http://www.stardust.webpages.pl/log/
> -
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[Mark Fortescue]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN; format=flowed, Size: 3186 bytes --]

Hi all,

I have tested a solution to the random invalid instruction occourances on 
sparc32 (sun4c). I have attached the patch as my mail client messes up 
patch files.

The problem apears to be an alignment error of the redzone2 word, caused 
by the userword not being forced onto a 64bit boundary. My solution is to 
increase the size of the user word (BYTES_PER_WORD) to 64bits in order to 
force the correct alignment of the user word (and hence the redzone2 
word). My solution also works on platforms where sizeof(unsigned long 
long) is 128bits and requires 128bit alignment.

Regards
 	Mark Fortescue.

On Tue, 3 Jul 2007, Mark Fortescue wrote:

> Hi all,
>
> I think I have found the cause of the problem.
>
> Commit b46b8f19c9cd435ecac4d9d12b39d78c137ecd66 partially fixed alignment 
> issues but does not ensure that all 64bit alignment requirements of sparc32 
> are met. Tests have shown that the redzone2 word can become misallignd.
>
> I am currently working on a posible fix.
>
> Regards
> 	Mark Fortescue.
>
> On Tue, 3 Jul 2007, Michal Piotrowski wrote:
>
>> Hi all,
>> 
>> Here is a list of some known regressions in 2.6.22-rc7.
>> 
>> Feel free to add new regressions/remove fixed etc.
>> http://kernelnewbies.org/known_regressions
>> 
>> List of Aces
>> 
>> Name                    Regressions fixed since 21-Jun-2007
>> Hugh Dickins                           2
>> Andi Kleen                             1
>> Andrew Morton                          1
>> Benjamin Herrenschmidt                 1
>> Björn Steinbrink                       1
>> Bjorn Helgaas                          1
>> Jean Delvare                           1
>> Olaf Hering                            1
>> Siddha, Suresh B                       1
>> Trent Piepho                           1
>> Ville Syrjälä                          1
>> 
>> 
>> 
>> FS
>> 
>> Subject    : 2.6.22-rc4-git5 reiserfs: null ptr deref.
>> References : http://lkml.org/lkml/2007/6/13/322
>> Submitter  : Randy Dunlap <randy.dunlap@oracle.com>
>> Handled-By : Vladimir V. Saveliev <vs@namesys.com>
>> Status     : problem is being debugged
>> 
>> 
>> 
>> IDE
>> 
>> Subject    : 2.6.22-rcX: hda: lost interrupt
>> References : http://lkml.org/lkml/2007/6/29/121
>> Submitter  : David Chinner <dgc@sgi.com>
>> Status     : unknown
>> 
>> 
>> 
>> Sparc64
>> 
>> Subject    : random invalid instruction occourances on sparc32 (sun4c)
>> References : http://lkml.org/lkml/2007/6/17/111
>> Submitter  : Mark Fortescue <mark@mtfhpc.demon.co.uk>
>> Status     : problem is being debugged
>> 
>> Subject    : 2.6.22-rc broke X on Ultra5
>> References : http://lkml.org/lkml/2007/5/22/78
>> Submitter  : Mikael Pettersson <mikpe@it.uu.se>
>> Handled-By : David Miller <davem@davemloft.net>
>> Status     : problem is being debugged
>> 
>> 
>> 
>> Regards,
>> Michal
>> 
>> --
>> LOG
>> http://www.stardust.webpages.pl/log/
>> -
>> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[-- Attachment #2: Type: TEXT/PLAIN, Size: 2480 bytes --]

From: Mark Fortescue <mark@mtfhpc.demon.co.uk>

Verious alignment fixes in the SLAB alocator that increased the size
of the RedZone words failed to ensure that RedZone word 2 is aligned
on a 64bit boundary. This has resulted in random invalid instruction
occourances on Sparc32 (sun4c).
By increasing the size of the User Word (BYTES_PER_WORD) to 64bits
seams to ensure that correct alignment is maintained but assumes that:
   sizeof (void *) <= sizeof (unsigned dlong long) 

Signed-off-by: Mark Fortescue <mark@mtfhpc.demon.co.uk>
---
Alternative solutions would involve correcting the size caculations on
lines 2175 to 2275 and may also involve additional changes to the
calculations to get a pointer to the RedZone Word 2.
--- linux-2.6/mm/slab.c	2007-07-03 17:35:07.000000000 +0100
+++ linux-test/mm/slab.c	2007-07-03 19:05:19.000000000 +0100
@@ -136,7 +136,8 @@
 #endif
 
 /* Shouldn't this be in a header file somewhere? */
-#define	BYTES_PER_WORD		sizeof(void *)
+/* Fix alignment of redzone2. Assumes sizeof (void*) <= sizeof (unsigned long long) */
+#define	BYTES_PER_WORD		sizeof(unsigned long long)
 
 #ifndef cache_line_size
 #define cache_line_size()	L1_CACHE_BYTES
@@ -538,7 +539,7 @@ static unsigned long long *dbg_redzone1(
 {
 	BUG_ON(!(cachep->flags & SLAB_RED_ZONE));
 	return (unsigned long long*) (objp + obj_offset(cachep) -
-				      sizeof(unsigned long long));
+				      BYTES_PER_WORD);
 }
 
 static unsigned long long *dbg_redzone2(struct kmem_cache *cachep, void *objp)
@@ -546,10 +547,9 @@ static unsigned long long *dbg_redzone2(
 	BUG_ON(!(cachep->flags & SLAB_RED_ZONE));
 	if (cachep->flags & SLAB_STORE_USER)
 		return (unsigned long long *)(objp + cachep->buffer_size -
-					      sizeof(unsigned long long) -
-					      BYTES_PER_WORD);
+					      2 * BYTES_PER_WORD);
 	return (unsigned long long *) (objp + cachep->buffer_size -
-				       sizeof(unsigned long long));
+				       BYTES_PER_WORD);
 }
 
 static void **dbg_userword(struct kmem_cache *cachep, void *objp)
@@ -2256,8 +2256,8 @@ kmem_cache_create (const char *name, siz
 	 */
 	if (flags & SLAB_RED_ZONE) {
 		/* add space for red zone words */
-		cachep->obj_offset += sizeof(unsigned long long);
-		size += 2 * sizeof(unsigned long long);
+		cachep->obj_offset += BYTES_PER_WORD;
+		size += 2 * BYTES_PER_WORD;
 	}
 	if (flags & SLAB_STORE_USER) {
 		/* user store requires one word storage behind the end of

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[David Woodhouse]

On Tue, 2007-07-03 at 19:57 +0100, Mark Fortescue wrote:
> > Commit b46b8f19c9cd435ecac4d9d12b39d78c137ecd66 partially fixed alignment 
> > issues but does not ensure that all 64bit alignment requirements of sparc32 
> > are met. Tests have shown that the redzone2 word can become misallignd.

Oops, sorry about that. I'm not sure about your patch though -- I think
I'd prefer to keep the redzone misaligned (and hence _right_ next to the
real data), and just deal with it.

typedef unsigned long long __aligned__((BYTES_PER_WORD)) redzone_t;

-- 
dwmw2

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

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[Mark Fortescue]

Hi David,

The problem is that sun4c Sparc32 can't handle un-aligned variables so 
having a 64bit readzone word that is not aligned on a 64bit boundary is a 
problem.

In addition, having looked at the size calculations, it looks to me as if 
not all of them got updated to handle 64bit redzone words. This may be part of 
the problem. By making BYTES_PER_WORD 64bit aligned (Sparc32) this is 
nolonger an issue.

Regards
 	Mark Fortescue.

On Tue, 3 Jul 2007, David Woodhouse wrote:

> On Tue, 2007-07-03 at 19:57 +0100, Mark Fortescue wrote:
>>> Commit b46b8f19c9cd435ecac4d9d12b39d78c137ecd66 partially fixed alignment
>>> issues but does not ensure that all 64bit alignment requirements of sparc32
>>> are met. Tests have shown that the redzone2 word can become misallignd.
>
> Oops, sorry about that. I'm not sure about your patch though -- I think
> I'd prefer to keep the redzone misaligned (and hence _right_ next to the
> real data), and just deal with it.
>
> typedef unsigned long long __aligned__((BYTES_PER_WORD)) redzone_t;
>
> -- 
> dwmw2
>
> -
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[David Woodhouse]

On Tue, 2007-07-03 at 22:25 +0100, Mark Fortescue wrote:
> The problem is that sun4c Sparc32 can't handle un-aligned variables so 
> having a 64bit readzone word that is not aligned on a 64bit boundary is a 
> problem.

Surely, it can. You just have to tell the compiler that it's not
properly aligned, and it'll emit code to cope. Hence the suggestion that
you use 'unsigned long long __attribute__((aligned(BYTES_PER_WORD))'.
But it's probably better just to make sure it remains aligned; you're
right.

> In addition, having looked at the size calculations, it looks to me as if 
> not all of them got updated to handle 64bit redzone words. 

Really? Other than the alignment of the second redzone, what's wrong?
Remember that the 'user word' is still not necessarily 64-bit. And in
fact I suspect that's what is causing the problem -- your object _size_
will be aligned to 8 bytes, including the user word, and then we look
for the second redzone word 12 bytes before the end of the object.

Does this fix it?

diff --git a/mm/slab.c b/mm/slab.c
index 6d65cf4..3b15671 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -547,7 +547,7 @@ static unsigned long long *dbg_redzone2(struct kmem_cache *cachep, void *objp)
 	if (cachep->flags & SLAB_STORE_USER)
 		return (unsigned long long *)(objp + cachep->buffer_size -
 					      sizeof(unsigned long long) -
-					      BYTES_PER_WORD);
+					      max(BYTES_PER_WORD, __alignof__(unsigned long long)));
 	return (unsigned long long *) (objp + cachep->buffer_size -
 				       sizeof(unsigned long long));
 }
@@ -2262,9 +2262,14 @@ kmem_cache_create (const char *name, size_t size, size_t align,
 	}
 	if (flags & SLAB_STORE_USER) {
 		/* user store requires one word storage behind the end of
-		 * the real object.
+		 * the real object. But if the second red zone must be
+		 * aligned 'better' than that, allow for it.
 		 */
-		size += BYTES_PER_WORD;
+		if (flags & SLAB_RED_ZONE
+		    && BYTES_PER_WORD < __alignof__(unsigned long long))
+			size += __alignof__(unsigned long long);
+		else
+			size += BYTES_PER_WORD;
 	}
 #if FORCED_DEBUG && defined(CONFIG_DEBUG_PAGEALLOC)
 	if (size >= malloc_sizes[INDEX_L3 + 1].cs_size


-- 
dwmw2

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

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[Mark Fortescue]

Hi David,

I will try out your patch shortly.

On Tue, 3 Jul 2007, David Woodhouse wrote:

> On Tue, 2007-07-03 at 22:25 +0100, Mark Fortescue wrote:
>> The problem is that sun4c Sparc32 can't handle un-aligned variables so
>> having a 64bit readzone word that is not aligned on a 64bit boundary is a
>> problem.
>
> Surely, it can. You just have to tell the compiler that it's not
> properly aligned, and it'll emit code to cope. Hence the suggestion that
> you use 'unsigned long long __attribute__((aligned(BYTES_PER_WORD))'.
> But it's probably better just to make sure it remains aligned; you're
> right.
>
>> In addition, having looked at the size calculations, it looks to me as if
>> not all of them got updated to handle 64bit redzone words.
>
> Really? Other than the alignment of the second redzone, what's wrong?
> Remember that the 'user word' is still not necessarily 64-bit. And in
> fact I suspect that's what is causing the problem -- your object _size_
> will be aligned to 8 bytes, including the user word, and then we look
> for the second redzone word 12 bytes before the end of the object.
>

Yes, the user word is a 32bit word and this is part of the issue.

I may be wrong about the size calculations but if you take a look at lines 
2174 to 2188 and 2207 to 2203, reading the comments suggest to me that 
these need to be changed to match the changes to the RedZone words. 
Failing to change these means that 32bit aligned access of the 64bit 
RedZone words is still posible and this will kill sun4c.

For the 64bit RedZone word to be 64bit aligned (required by sun4c), the 
User word must be 64bit aligned. I don't see where in your patch, this is 
enforced.

> Does this fix it?
>
> diff --git a/mm/slab.c b/mm/slab.c
> index 6d65cf4..3b15671 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -547,7 +547,7 @@ static unsigned long long *dbg_redzone2(struct kmem_cache *cachep, void *objp)
> 	if (cachep->flags & SLAB_STORE_USER)
> 		return (unsigned long long *)(objp + cachep->buffer_size -
> 					      sizeof(unsigned long long) -
> -					      BYTES_PER_WORD);
> +					      max(BYTES_PER_WORD, __alignof__(unsigned long long)));
> 	return (unsigned long long *) (objp + cachep->buffer_size -
> 				       sizeof(unsigned long long));
> }
> @@ -2262,9 +2262,14 @@ kmem_cache_create (const char *name, size_t size, size_t align,
> 	}
> 	if (flags & SLAB_STORE_USER) {
> 		/* user store requires one word storage behind the end of
> -		 * the real object.
> +		 * the real object. But if the second red zone must be
> +		 * aligned 'better' than that, allow for it.
> 		 */
> -		size += BYTES_PER_WORD;
> +		if (flags & SLAB_RED_ZONE
> +		    && BYTES_PER_WORD < __alignof__(unsigned long long))
> +			size += __alignof__(unsigned long long);
> +		else
> +			size += BYTES_PER_WORD;
> 	}
> #if FORCED_DEBUG && defined(CONFIG_DEBUG_PAGEALLOC)
> 	if (size >= malloc_sizes[INDEX_L3 + 1].cs_size
>
>
> -- 
> dwmw2
>
>

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

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[David Woodhouse]

On Tue, 2007-07-03 at 23:47 +0100, Mark Fortescue wrote:
> Hi David,
> 
> I will try out your patch shortly.

Thanks.

> I may be wrong about the size calculations but if you take a look at lines 
> 2174 to 2188 and 2207 to 2203, reading the comments suggest to me that 
> these need to be changed to match the changes to the RedZone words. 
> Failing to change these means that 32bit aligned access of the 64bit 
> RedZone words is still posible and this will kill sun4c.

Why do we need more than the existing:

	if (flags & SLAB_RED_ZONE || flags & SLAB_STORE_USER)
		ralign = __alignof__(unsigned long long);

> For the 64bit RedZone word to be 64bit aligned (required by sun4c), the 
> User word must be 64bit aligned. I don't see where in your patch, this is 
> enforced.

Where __alignof__(long long) > BYTES_PER_WORD my patch should lead to
this layout (32-bit words):

    [ redzone1 bits 63-32 ]
    [ redzone1 bits 31-0  ]
    [    ... object ...   ]
    [    ... object ...   ]
    [ redzone2 bits 63-32 ]
    [ redzone2 bits 31-0  ]
    [        unused       ]
    [      user word      ]

The user word is a 32-bit value; there's no requirement for _it_ to be
aligned.

Hm, actually I think my patch may be incomplete -- I need to adjust the
size of the actual object too. This patch should be better...

diff --git a/mm/slab.c b/mm/slab.c
index a9c4472..8081c07 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -547,7 +547,7 @@ static unsigned long long *dbg_redzone2(struct kmem_cache *cachep, void *objp)
 	if (cachep->flags & SLAB_STORE_USER)
 		return (unsigned long long *)(objp + cachep->buffer_size -
 					      sizeof(unsigned long long) -
-					      BYTES_PER_WORD);
+					      max(BYTES_PER_WORD, __alignof__(unsigned long long)));
 	return (unsigned long long *) (objp + cachep->buffer_size -
 				       sizeof(unsigned long long));
 }
@@ -2223,8 +2223,11 @@ kmem_cache_create (const char *name, size_t size, size_t align,
 	 * overridden by architecture or caller mandated alignment if either
 	 * is greater than BYTES_PER_WORD.
 	 */
-	if (flags & SLAB_RED_ZONE || flags & SLAB_STORE_USER)
+	if (flags & SLAB_RED_ZONE || flags & SLAB_STORE_USER) {
 		ralign = __alignof__(unsigned long long);
+		size += (__alignof__(unsigned long long) - 1);
+		size &= ~(__alignof__(unsigned long long) - 1);
+	}
 
 	/* 2) arch mandated alignment */
 	if (ralign < ARCH_SLAB_MINALIGN) {
@@ -2261,9 +2264,14 @@ kmem_cache_create (const char *name, size_t size, size_t align,
 	}
 	if (flags & SLAB_STORE_USER) {
 		/* user store requires one word storage behind the end of
-		 * the real object.
+		 * the real object. But if the second red zone must be
+		 * aligned 'better' than that, allow for it.
 		 */
-		size += BYTES_PER_WORD;
+		if (flags & SLAB_RED_ZONE
+		    && BYTES_PER_WORD < __alignof__(unsigned long long))
+			size += __alignof__(unsigned long long);
+		else
+			size += BYTES_PER_WORD;
 	}
 #if FORCED_DEBUG && defined(CONFIG_DEBUG_PAGEALLOC)
 	if (size >= malloc_sizes[INDEX_L3 + 1].cs_size


-- 
dwmw2

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

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[Mark Fortescue]

Hi David,

I tried the previous patch and it looks like it fixes the issue however 
one of the test builds I did caused depmod to use up all available memory 
(40M - kernel memory) before taking out the kernel with the oom killer.
At present, I do not know if it is a depmod issue or a kernel issue.
I will have to do some more tests later on to day.

I have looked at the latest patch below and am I am still not sure about 
two areas. Please take a look at my offering based on your latest 
patch (included here to it will probably get mangled).

Note the change to lines 2178 to 2185. I have also changed/moved the 
alignment of size (see lines 2197 to 2206) based on your changes.

--- linux-2.6/mm/slab.c	2007-07-03 19:09:48.000000000 +0100
+++ linux-test/mm/slab.c	2007-07-04 04:14:15.000000000 +0100
@@ -137,6 +137,7 @@

  /* Shouldn't this be in a header file somewhere? */
  #define	BYTES_PER_WORD		sizeof(void *)
+#define RED_ZONE_ALIGN_MASK	(max(__alignof__(void *), __alignof(unsigned long long)) - 1)

  #ifndef cache_line_size
  #define cache_line_size()	L1_CACHE_BYTES
@@ -547,7 +548,7 @@ static unsigned long long *dbg_redzone2(
  	if (cachep->flags & SLAB_STORE_USER)
  		return (unsigned long long *)(objp + cachep->buffer_size -
  					      sizeof(unsigned long long) -
-					      BYTES_PER_WORD);
+					      max(BYTES_PER_WORD, __alignof__(unsigned long long)));
  	return (unsigned long long *) (objp + cachep->buffer_size -
  				       sizeof(unsigned long long));
  }
@@ -2178,7 +2179,8 @@ kmem_cache_create (const char *name, siz
  	 * above the next power of two: caches with object sizes just above a
  	 * power of two have a significant amount of internal fragmentation.
  	 */
-	if (size < 4096 || fls(size - 1) == fls(size-1 + 3 * BYTES_PER_WORD))
+	if (size < 4096 || fls(size - 1) == fls(size-1 + 2 * sizeof(unsigned long long) +
+						max(BYTES_PER_WORD, __alignof__(unsigned long long))))
  		flags |= SLAB_RED_ZONE | SLAB_STORE_USER;
  	if (!(flags & SLAB_DESTROY_BY_RCU))
  		flags |= SLAB_POISON;
@@ -2197,9 +2199,9 @@ kmem_cache_create (const char *name, siz
  	 * unaligned accesses for some archs when redzoning is used, and makes
  	 * sure any on-slab bufctl's are also correctly aligned.
  	 */
-	if (size & (BYTES_PER_WORD - 1)) {
-		size += (BYTES_PER_WORD - 1);
-		size &= ~(BYTES_PER_WORD - 1);
+	if (size & RED_ZONE_ALIGN_MASK) {
+		size += RED_ZONE_ALIGN_MASK;
+		size &= ~RED_ZONE_ALIGN_MASK;
  	}

  	/* calculate the final buffer alignment: */
@@ -2261,9 +2263,14 @@ kmem_cache_create (const char *name, siz
  	}
  	if (flags & SLAB_STORE_USER) {
  		/* user store requires one word storage behind the end of
-		 * the real object.
+		 * the real object. But if the second red zone must be
+		 * aligned 'better' than that, allow for it.
  		 */
-		size += BYTES_PER_WORD;
+		if (flags & SLAB_RED_ZONE
+		    && BYTES_PER_WORD < __alignof__(unsigned long long))
+			size += __alignof__(unsigned long long);
+		else
+			size += BYTES_PER_WORD;
  	}
  #if FORCED_DEBUG && defined(CONFIG_DEBUG_PAGEALLOC)
  	if (size >= malloc_sizes[INDEX_L3 + 1].cs_size

---

Let me know if you would like an un-mangled copy of the patch as an 
attachement.

Regards
 	Mark Fortescue.

On Tue, 3 Jul 2007, David Woodhouse wrote:

> On Tue, 2007-07-03 at 23:47 +0100, Mark Fortescue wrote:
>> Hi David,
>>
>> I will try out your patch shortly.
>
> Thanks.
>
>> I may be wrong about the size calculations but if you take a look at lines
>> 2174 to 2188 and 2207 to 2203, reading the comments suggest to me that
>> these need to be changed to match the changes to the RedZone words.
>> Failing to change these means that 32bit aligned access of the 64bit
>> RedZone words is still posible and this will kill sun4c.
>
> Why do we need more than the existing:
>
> 	if (flags & SLAB_RED_ZONE || flags & SLAB_STORE_USER)
> 		ralign = __alignof__(unsigned long long);
>
>> For the 64bit RedZone word to be 64bit aligned (required by sun4c), the
>> User word must be 64bit aligned. I don't see where in your patch, this is
>> enforced.
>
> Where __alignof__(long long) > BYTES_PER_WORD my patch should lead to
> this layout (32-bit words):
>
>    [ redzone1 bits 63-32 ]
>    [ redzone1 bits 31-0  ]
>    [    ... object ...   ]
>    [    ... object ...   ]
>    [ redzone2 bits 63-32 ]
>    [ redzone2 bits 31-0  ]
>    [        unused       ]
>    [      user word      ]
>
> The user word is a 32-bit value; there's no requirement for _it_ to be
> aligned.
>
> Hm, actually I think my patch may be incomplete -- I need to adjust the
> size of the actual object too. This patch should be better...
>
> diff --git a/mm/slab.c b/mm/slab.c
> index a9c4472..8081c07 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -547,7 +547,7 @@ static unsigned long long *dbg_redzone2(struct kmem_cache *cachep, void *objp)
> 	if (cachep->flags & SLAB_STORE_USER)
> 		return (unsigned long long *)(objp + cachep->buffer_size -
> 					      sizeof(unsigned long long) -
> -					      BYTES_PER_WORD);
> +					      max(BYTES_PER_WORD, __alignof__(unsigned long long)));
> 	return (unsigned long long *) (objp + cachep->buffer_size -
> 				       sizeof(unsigned long long));
> }
> @@ -2223,8 +2223,11 @@ kmem_cache_create (const char *name, size_t size, size_t align,
> 	 * overridden by architecture or caller mandated alignment if either
> 	 * is greater than BYTES_PER_WORD.
> 	 */
> -	if (flags & SLAB_RED_ZONE || flags & SLAB_STORE_USER)
> +	if (flags & SLAB_RED_ZONE || flags & SLAB_STORE_USER) {
> 		ralign = __alignof__(unsigned long long);
> +		size += (__alignof__(unsigned long long) - 1);
> +		size &= ~(__alignof__(unsigned long long) - 1);
> +	}
>
> 	/* 2) arch mandated alignment */
> 	if (ralign < ARCH_SLAB_MINALIGN) {
> @@ -2261,9 +2264,14 @@ kmem_cache_create (const char *name, size_t size, size_t align,
> 	}
> 	if (flags & SLAB_STORE_USER) {
> 		/* user store requires one word storage behind the end of
> -		 * the real object.
> +		 * the real object. But if the second red zone must be
> +		 * aligned 'better' than that, allow for it.
> 		 */
> -		size += BYTES_PER_WORD;
> +		if (flags & SLAB_RED_ZONE
> +		    && BYTES_PER_WORD < __alignof__(unsigned long long))
> +			size += __alignof__(unsigned long long);
> +		else
> +			size += BYTES_PER_WORD;
> 	}
> #if FORCED_DEBUG && defined(CONFIG_DEBUG_PAGEALLOC)
> 	if (size >= malloc_sizes[INDEX_L3 + 1].cs_size
>
>
> -- 
> dwmw2
>
>

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

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[David Woodhouse]

On Wed, 2007-07-04 at 04:27 +0100, Mark Fortescue wrote:
> I tried the previous patch and it looks like it fixes the issue however 
> one of the test builds I did caused depmod to use up all available memory 
> (40M - kernel memory) before taking out the kernel with the oom killer.
> At present, I do not know if it is a depmod issue or a kernel issue.
> I will have to do some more tests later on to day.

That's almost certain to be an unrelated issue.

> I have looked at the latest patch below and am I am still not sure about 
> two areas. Please take a look at my offering based on your latest 
> patch (included here to it will probably get mangled).
> 
> Note the change to lines 2178 to 2185. I have also changed/moved the 
> alignment of size (see lines 2197 to 2206) based on your changes. 

The first looks correct; well spotted. The second is just cosmetic but
also seems correct. I might be inclined to make the #define
'RED_ZONE_ALIGN' and use it in the other places too.

-- 
dwmw2

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

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[Mark Fortescue]

Hi David,

Another related point that may also need to be considered is that I think 
I am correct in saying that on ARM and on the 64bit platforms, sizeof 
(unsigned long long) is 16 (128bits).

Should the RedZone words be specified as __u64 not the unsigned long long 
used or does the alignment need to be that of unsigned long long ?

Regards
 	Mark Fortescue.

  On Tue, 3 Jul 2007, David Woodhouse wrote:

> On Wed, 2007-07-04 at 04:27 +0100, Mark Fortescue wrote:
>> I tried the previous patch and it looks like it fixes the issue however
>> one of the test builds I did caused depmod to use up all available memory
>> (40M - kernel memory) before taking out the kernel with the oom killer.
>> At present, I do not know if it is a depmod issue or a kernel issue.
>> I will have to do some more tests later on to day.
>
> That's almost certain to be an unrelated issue.
>
>> I have looked at the latest patch below and am I am still not sure about
>> two areas. Please take a look at my offering based on your latest
>> patch (included here to it will probably get mangled).
>>
>> Note the change to lines 2178 to 2185. I have also changed/moved the
>> alignment of size (see lines 2197 to 2206) based on your changes.
>
> The first looks correct; well spotted. The second is just cosmetic but
> also seems correct. I might be inclined to make the #define
> 'RED_ZONE_ALIGN' and use it in the other places too.
>
> -- 
> dwmw2
>
> -
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[David Woodhouse]

On Wed, 2007-07-04 at 11:27 +0100, Mark Fortescue wrote:
> Another related point that may also need to be considered is that I think 
> I am correct in saying that on ARM and on the 64bit platforms, sizeof 
> (unsigned long long) is 16 (128bits).

No, it's always 64 bits.

> Should the RedZone words be specified as __u64 not the unsigned long long 
> used or does the alignment need to be that of unsigned long long ?

You have to play silly buggers with printk formats (%lx vs. %llx) if you
do that. And you have to make a choice about using proper C types or the
Linux-specific nonsense. 'unsigned long long' is just easier all round.

-- 
dwmw2

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

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[Mark Fortescue]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 206 bytes --]

Hi David,

I have sucessfully tested the attached patch on Sparc32:sun4c.

Is there any chance of this getting sufficiantly tested on other 
architectures to get in into v2.6.22 ?

Regards
 	Mark Fortescue.

[-- Attachment #2: Type: TEXT/PLAIN, Size: 3129 bytes --]

From: Mark Fortescue <mark@mtfhpc.demon.co.uk>, David Woodhouse <dwmw2@infradead.org>

Verious alignment fixes in the SLAB alocator that increased the size
of the RedZone words failed to ensure that RedZone word 2 is aligned
on a 64bit boundary. This resulted in random invalid instruction
occourances on Sparc32 (sun4c).
These additional changes should ensure that the RedZone words are
correctly aligned on a 64bit boundary for architectures that require this.

Signed-off-by: Mark Fortescue <mark@mtfhpc.demon.co.uk>
---
Tested on Sparc32:sun4c
Additional testing desirable on PowerPC32, Sparc64, PowerPC64 and any
other platforms where alignment changes may be of concirn.
--- linux-2.6/mm/slab.c	2007-07-03 19:09:48.000000000 +0100
+++ linux-test/mm/slab.c	2007-07-04 04:14:15.000000000 +0100
@@ -137,6 +137,7 @@
 
 /* Shouldn't this be in a header file somewhere? */
 #define	BYTES_PER_WORD		sizeof(void *)
+#define	RED_ZONE_ALIGN		(max(__alignof__(void *), __alignof(unsigned long long)) - 1)
 
 #ifndef cache_line_size
 #define cache_line_size()	L1_CACHE_BYTES
@@ -547,7 +548,7 @@ static unsigned long long *dbg_redzone2(
 	if (cachep->flags & SLAB_STORE_USER)
 		return (unsigned long long *)(objp + cachep->buffer_size -
 					      sizeof(unsigned long long) -
-					      BYTES_PER_WORD);
+					      max(BYTES_PER_WORD, __alignof__(unsigned long long)));
 	return (unsigned long long *) (objp + cachep->buffer_size -
 				       sizeof(unsigned long long));
 }
@@ -2178,7 +2179,8 @@ kmem_cache_create (const char *name, siz
 	 * above the next power of two: caches with object sizes just above a
 	 * power of two have a significant amount of internal fragmentation.
 	 */
-	if (size < 4096 || fls(size - 1) == fls(size-1 + 3 * BYTES_PER_WORD))
+	if (size < 4096 || fls(size - 1) == fls(size-1 + 2 * sizeof(unsigned long long) +
+						max(BYTES_PER_WORD, __alignof__(unsigned long long))))
 		flags |= SLAB_RED_ZONE | SLAB_STORE_USER;
 	if (!(flags & SLAB_DESTROY_BY_RCU))
 		flags |= SLAB_POISON;
@@ -2197,9 +2199,9 @@ kmem_cache_create (const char *name, siz
 	 * unaligned accesses for some archs when redzoning is used, and makes
 	 * sure any on-slab bufctl's are also correctly aligned.
 	 */
-	if (size & (BYTES_PER_WORD - 1)) {
-		size += (BYTES_PER_WORD - 1);
-		size &= ~(BYTES_PER_WORD - 1);
+	if (size & RED_ZONE_ALIGN) {
+		size += RED_ZONE_ALIGN;
+		size &= ~RED_ZONE_ALIGN;
 	}
 
 	/* calculate the final buffer alignment: */
@@ -2261,9 +2263,14 @@ kmem_cache_create (const char *name, siz
 	}
 	if (flags & SLAB_STORE_USER) {
 		/* user store requires one word storage behind the end of
-		 * the real object.
+		 * the real object. But if the second red zone must be
+		 * aligned 'better' than that, allow for it.
 		 */
-		size += BYTES_PER_WORD;
+		if (flags & SLAB_RED_ZONE
+		    && BYTES_PER_WORD < __alignof__(unsigned long long))
+			size += __alignof__(unsigned long long);
+		else
+			size += BYTES_PER_WORD;
 	}
 #if FORCED_DEBUG && defined(CONFIG_DEBUG_PAGEALLOC)
 	if (size >= malloc_sizes[INDEX_L3 + 1].cs_size

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[David Miller, David Woodhouse]

> On Tue, 2007-07-03 at 19:57 +0100, Mark Fortescue wrote:
> > > Commit b46b8f19c9cd435ecac4d9d12b39d78c137ecd66 partially fixed alignment 
> > > issues but does not ensure that all 64bit alignment requirements of sparc32 
> > > are met. Tests have shown that the redzone2 word can become misallignd.
> 
> Oops, sorry about that. I'm not sure about your patch though -- I think
> I'd prefer to keep the redzone misaligned (and hence _right_ next to the
> real data), and just deal with it.
> 
> typedef unsigned long long __aligned__((BYTES_PER_WORD)) redzone_t;

Please don't use get_unaligned() or whatever to fix this, it's
going to generate the byte-at-a-time accesses on sparc64
which doesn't need it since the redzone will be aligned.

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

Re: [PATCH] Re: Sparc32: random invalid instruction occourances on sparc32 (sun4c)

[David Woodhouse]

On Tue, 2007-07-03 at 14:41 -0700, David Miller wrote:
> Please don't use get_unaligned() or whatever to fix this, it's
> going to generate the byte-at-a-time accesses on sparc64
> which doesn't need it since the redzone will be aligned. 

Yes, get_unaligned() would suck. But 'u64 __aligned__((BYTES_PER_WORD))'
as I suggested should result in a single 64-bit load on 64-bit
architectures, and two 32-bit loads on 32-bit architectures.

But I think the patch I just sent is a better option than that anyway.

-- 
dwmw2

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