Re: 4.0.0-rc4: panic in free_block

Re: 4.0.0-rc4: panic in free_block

[Linus Torvalds]

[ Added Davem and the sparc mailing list, since it happens on sparc
and that just makes me suspicious ]

On Fri, Mar 20, 2015 at 8:07 AM, David Ahern <david.ahern@oracle.com> wrote:
> I can easily reproduce the panic below doing a kernel build with make -j N,
> N\x128, 256, etc. This is a 1024 cpu system running 4.0.0-rc4.

3.19 is fine? Because I dont' think I've seen any reports like this
for others, and what stands out is sparc (and to a lesser degree "1024
cpus", which obviously gets a lot less testing)

> The top 3 frames are consistently:
>     free_block+0x60
>     cache_flusharray+0xac
>     kmem_cache_free+0xfc
>
> After that one path has been from __mmdrop and the others are like below,
> from remove_vma.
>
> Unable to handle kernel paging request at virtual address 0006100000000000

One thing you *might* check is if the problem goes away if you select
CONFIG_SLUB instead of CONFIG_SLAB. I'd really like to just get rid of
SLAB. The whole "we have multiple different allocators" is a mess and
causes test coverage issues.

Apart from testing with CONFIG_SLUB, if 3.19 is ok and you seem to be
able to "easily reproduce" this, the obvious thing to do is to try to
bisect it.

                         Linus

Re: 4.0.0-rc4: panic in free_block

[David Ahern]

On 3/20/15 10:48 AM, Linus Torvalds wrote:
> [ Added Davem and the sparc mailing list, since it happens on sparc
> and that just makes me suspicious ]
>
> On Fri, Mar 20, 2015 at 8:07 AM, David Ahern <david.ahern@oracle.com> wrote:
>> I can easily reproduce the panic below doing a kernel build with make -j N,
>> N\x128, 256, etc. This is a 1024 cpu system running 4.0.0-rc4.
>
> 3.19 is fine? Because I dont' think I've seen any reports like this
> for others, and what stands out is sparc (and to a lesser degree "1024
> cpus", which obviously gets a lot less testing)

I haven't tried 3.19 yet. Just backed up to 3.18 and it shows the same 
problem. And I can reproduce the 4.0 crash in a 128 cpu ldom (VM).

>
>> The top 3 frames are consistently:
>>      free_block+0x60
>>      cache_flusharray+0xac
>>      kmem_cache_free+0xfc
>>
>> After that one path has been from __mmdrop and the others are like below,
>> from remove_vma.
>>
>> Unable to handle kernel paging request at virtual address 0006100000000000
>
> One thing you *might* check is if the problem goes away if you select
> CONFIG_SLUB instead of CONFIG_SLAB. I'd really like to just get rid of
> SLAB. The whole "we have multiple different allocators" is a mess and
> causes test coverage issues.
>
> Apart from testing with CONFIG_SLUB, if 3.19 is ok and you seem to be
> able to "easily reproduce" this, the obvious thing to do is to try to
> bisect it.

I'll try SLUB. The ldom reboots 1000 times faster then resetting the h/w 
so a better chance of bisecting - if I can find a known good release.

David

Re: 4.0.0-rc4: panic in free_block

[Linus Torvalds]

On Fri, Mar 20, 2015 at 9:53 AM, David Ahern <david.ahern@oracle.com> wrote:
>
> I haven't tried 3.19 yet. Just backed up to 3.18 and it shows the same
> problem. And I can reproduce the 4.0 crash in a 128 cpu ldom (VM).

Ok, so if 3.18 also has it, then trying 3.19 is pointless, this is
obviously an old problem. Which makes it even more likely that it's
sparc-related.

128 cpu's is still "unusual", of course, but by no means unheard of,
and I'f have expected others to report it too if it was wasy to
trigger on x86-64.

That said, SLAB is probably also almost unheard of in high-CPU
configurations, since slub has all the magical unlocked lists etc for
scalability. So maybe it's a generic SLAB bug, and nobody with lots of
CPU's is testing SLAB.

                      Linus

Re: 4.0.0-rc4: panic in free_block

[David Ahern]

On 3/20/15 10:58 AM, Linus Torvalds wrote:
> That said, SLAB is probably also almost unheard of in high-CPU
> configurations, since slub has all the magical unlocked lists etc for
> scalability. So maybe it's a generic SLAB bug, and nobody with lots of
> CPU's is testing SLAB.
>

Evidently, it is a well known problem internally that goes back to at 
least 2.6.39.

To this point I have not paid attention to the allocators. At what point 
is SLUB considered stable for large systems? Is 2.6.39 stable?

As for SLAB it is not clear if this is a sparc only problem. Perhaps the 
config should have a warning? It looks like SLAB is still the default 
for most arch.

DaveM: do you mind if I submit a patch to change the default for sparc 
to SLUB?

Now that the monster is unleashed, off to other problems...

Thanks,
David

Re: 4.0.0-rc4: panic in free_block

[Linus Torvalds]

On Fri, Mar 20, 2015 at 11:05 AM, David Ahern <david.ahern@oracle.com> wrote:
>
> Evidently, it is a well known problem internally that goes back to at least
> 2.6.39.
>
> To this point I have not paid attention to the allocators. At what point is
> SLUB considered stable for large systems? Is 2.6.39 stable?

SLUB should definitely be considered a stable allocator.  It's the
default allocator for at least Fedora, and that presumably means all
of Redhat.

SuSE seems to use SLAB still, though, so it must be getting lots of
testing on x86 too.

Did you test with SLUB? Does it work there?

> As for SLAB it is not clear if this is a sparc only problem. Perhaps the
> config should have a warning? It looks like SLAB is still the default for
> most arch.

I definitely think SLAB should work (although I *would* like to get
rid of the duplicate allocators), and I still do think it's likely a
sparc issue. Especially as it apparently goes way back. x86 gets a
*lot* more testing, and I don't really recall anything like this.

I'm not comfy enough reading sparc asm code to really pinpoint exactly
what looks to go wrong, so I have very little input on what the
problem could be.

                         Linus

Re: 4.0.0-rc4: panic in free_block

[David Ahern]

On 3/20/15 12:53 PM, Linus Torvalds wrote:
> SLUB should definitely be considered a stable allocator.  It's the
> default allocator for at least Fedora, and that presumably means all
> of Redhat.
>
> SuSE seems to use SLAB still, though, so it must be getting lots of
> testing on x86 too.
>
> Did you test with SLUB? Does it work there?

sorry, forgot to add that detail in the last response: it works fine. No 
panics at all with SLUB.

David

Re: 4.0.0-rc4: panic in free_block

[David Miller]

From: David Ahern <david.ahern@oracle.com>
Date: Fri, 20 Mar 2015 12:05:05 -0600

> DaveM: do you mind if I submit a patch to change the default for sparc
> to SLUB?

I think we're jumping the gun about all of this, and doing anything
with default Kconfig settings would be entirely premature until we
know what the real bug is.

On my T4-2 I've used nothing but SLAB and haven't hit any of these
problems.  I can't even remember the last time I turned SLUB on,
and it's just because I'm lazy.

Re: 4.0.0-rc4: panic in free_block

[David Ahern]

On 3/20/15 1:47 PM, David Miller wrote:
> From: David Ahern <david.ahern@oracle.com>
> Date: Fri, 20 Mar 2015 12:05:05 -0600
>
>> DaveM: do you mind if I submit a patch to change the default for sparc
>> to SLUB?
>
> I think we're jumping the gun about all of this, and doing anything
> with default Kconfig settings would be entirely premature until we
> know what the real bug is.

The suggestion to change to SLUB as the default was based on Linus' 
comment "SLAB is probably also almost unheard of in high-CPU
configurations, since slub has all the magical unlocked lists etc for
scalability."

>
> On my T4-2 I've used nothing but SLAB and haven't hit any of these
> problems.  I can't even remember the last time I turned SLUB on,
> and it's just because I'm lazy.
>

Interesting. With -j <64 and talking softly it completes. But -j 128 and 
higher always ends in a panic.

Re: 4.0.0-rc4: panic in free_block

[David Miller]

From: David Ahern <david.ahern@oracle.com>
Date: Fri, 20 Mar 2015 13:54:09 -0600

> Interesting. With -j <64 and talking softly it completes. But -j 128
> and higher always ends in a panic.

Please share more details of your configuration.

Re: 4.0.0-rc4: panic in free_block

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Fri, 20 Mar 2015 09:58:25 -0700

> 128 cpu's is still "unusual"

As unusual as the system I do all of my kernel builds on :-)

Re: 4.0.0-rc4: panic in free_block

[Dave Hansen]

On 03/20/2015 09:58 AM, Linus Torvalds wrote:
> 128 cpu's is still "unusual", of course, but by no means unheard of,
> and I'f have expected others to report it too if it was wasy to
> trigger on x86-64.

FWIW, I configured a kernel with SLAB and kicked off a bunch of compiles
on a 160-thread x86_64 system.  It definitely doesn't die _quickly_.
It's been running for an hour or two.

Re: 4.0.0-rc4: panic in free_block

[Linus Torvalds]

On Sat, Mar 21, 2015 at 10:45 AM, David Ahern <david.ahern@oracle.com> wrote:
>
> You raise a lot of valid questions and something to look into. But if the
> root cause were such a fundamental issue (CPU memory ordering, compiler bug,
> etc) why would it only occur on this one code path -- free with SLAB and
> NUMA -- and so consistently?

So the consistency could easily come from a compiler bug (or a missing
barrier in the kernel code) that just happens to trigger in a single
place (or in a few places, but then that's the only place that gets
exercised heavily enough to show it).

I agree that an actual hardware bug is unlikely, although that too is
possible: I can pretty much guarantee that if it were a CPU bug, it
wouldn't be some "memory ordering is entirely broken" bug in general,
it would be some very specific case that only happens with just the
right instruction timing and mix.

That said, while I bring up a CPU bug as a possibility, I really do
agree that it is *very* unlikely. Memory ordering is hard, and yes,
you can get it wrong, but at the same time CPU designers very much
know about it and tend to be pretty damn good about it. And as you
say, it generally wouldn't be *that* consistent. It might be
consistent for one particular kernel build (due to very particular
instruction mix and timings), but over lots of versions of the code
and many different debug options? Very very very unlikely.

> Continuing to poke around, but open to any suggestions. I have enabled every
> DEBUG I can find in the memory code and nothing is popping out. In terms of
> races wouldn't all the DEBUG checks affect timing? Yet, I am still seeing
> the same stack traces due to the same root cause.

Yes, generally debug options would change timings sufficiently that
any particular low-level race would certainly go away or at least
become much harder to hit. So if you have enabled spinlock debugging
etc, I don't really believe in a hw bug. It's  more likely that there
is some kernel architecture-specific code that triggers it. Or even
generic code that just happens to work on other cases due to random
issues (ie memory alignment etc).

I *would* suggest looking at that "memmove()" code. It really looks
like crap. It seems to do things byte-at-a-time for the overlapping
case, and the code seems to depend on memcpy always doing things
low-to-high, but there are multiple different memcpy implementations
so I don't know that that is always true. If one of the memcpy
functions sometimes copies the other way depending on size etc, it
could screw up.

Basically, that sparc64 memmove() implementation looks like it was
written by a dyslexic 5-year-old as a throw-away hack, and then never
got fixed.

Davem? I don't read sparc assembly, so I'm *really* not going to try
to verify that (a) all the memcpy implementations always copy
low-to-high and (b) that I even read the address comparisons in
memmove.S right.

I mention memmove just because it's actually fairly unusual for the
kernel. At the same time, if it really is broken for overlapping
regions, I'd expect *some* other places to show breakage too. So it's
probably fine, even if it does look very very bad to do things one
byte at a time backwards as a fallback.

                             Linus

Re: 4.0.0-rc4: panic in free_block

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sat, 21 Mar 2015 11:49:12 -0700

> Davem? I don't read sparc assembly, so I'm *really* not going to try
> to verify that (a) all the memcpy implementations always copy
> low-to-high and (b) that I even read the address comparisons in
> memmove.S right.

All of the sparc memcpy implementations copy from low to high.
I'll eat my hat if they don't. :-)

The guard tests at the beginning of memmove() are saying:

	if (dst <= src)
		memcpy(...);
	if (src + len <= dst)
		memcpy(...);

And then the reverse copy loop (and we do have to copy in reverse for
correctness) is basically:

	src = (src + len - 1);
	dst = (dst + len - 1);

1:	tmp = *(u8 *)src;
	len -= 1;
	src -= 1;
	*(u8 *)dst = tmp;
	dst -= 1;
	if (len != 0)
		goto 1b;

And then we return the original 'dst' pointer.

So at first glance it looks at least correct.

memmove() is a good idea to look into though, as SLAB and SLUB are the
only really heavy users of it, and they do so with overlapping
contents.

And they end up using that byte-at-a-time code, since SLAB and SLUB
do mmemove() calls of the form:

	memmove(X + N, X, LEN);

In which case neither of the memcpy() guard tests will pass.

Maybe there is some subtle bug in there I just don't see right now.

Re: 4.0.0-rc4: panic in free_block

[Bob Picco]

David Miller wrote:	[Sun Mar 22 2015, 01:36:03PM EDT]
> From: Linus Torvalds <torvalds@linux-foundation.org>
> Date: Sat, 21 Mar 2015 11:49:12 -0700
> 
> > Davem? I don't read sparc assembly, so I'm *really* not going to try
> > to verify that (a) all the memcpy implementations always copy
> > low-to-high and (b) that I even read the address comparisons in
> > memmove.S right.
> 
> All of the sparc memcpy implementations copy from low to high.
> I'll eat my hat if they don't. :-)
> 
> The guard tests at the beginning of memmove() are saying:
> 
> 	if (dst <= src)
> 		memcpy(...);
> 	if (src + len <= dst)
> 		memcpy(...);
> 
> And then the reverse copy loop (and we do have to copy in reverse for
> correctness) is basically:
> 
> 	src = (src + len - 1);
> 	dst = (dst + len - 1);
> 
> 1:	tmp = *(u8 *)src;
> 	len -= 1;
> 	src -= 1;
> 	*(u8 *)dst = tmp;
> 	dst -= 1;
> 	if (len != 0)
> 		goto 1b;
> 
> And then we return the original 'dst' pointer.
> 
> So at first glance it looks at least correct.
> 
> memmove() is a good idea to look into though, as SLAB and SLUB are the
> only really heavy users of it, and they do so with overlapping
> contents.
> 
> And they end up using that byte-at-a-time code, since SLAB and SLUB
> do mmemove() calls of the form:
> 
> 	memmove(X + N, X, LEN);
> 
> In which case neither of the memcpy() guard tests will pass.
> 
> Maybe there is some subtle bug in there I just don't see right now.
My original pursuit of this issue focused on transfers to and from the shared
array. Basically substituting memcpy-s with a primitive unsigned long memory
mover. This might have been incorrect.

There were substantial doubts because of large modifications to 2.6.39 too.
Unstabile hardware cause(d|s) issue too.

Eliminating the shared array functions correctly. Though this removal changes
performance and timing dramatically.

This afternoon I included modification of two memmove-s and no issue thus far.
The issue APPEARS to come from memmove-s within cache_flusharray() and/or
drain_array(). Now we are covering moves within an array_cache.

The above was done on 2.6.39.

Re: 4.0.0-rc4: panic in free_block

[Linus Torvalds]

On Sun, Mar 22, 2015 at 10:36 AM, David Miller <davem@davemloft.net> wrote:
>
> And they end up using that byte-at-a-time code, since SLAB and SLUB
> do mmemove() calls of the form:
>
>         memmove(X + N, X, LEN);

Actually, the common case in slab is overlapping but of the form

     memmove(p, p+x, len);

which goes to memcpy. It's basically re-compacting the array at the beginning.

Which was why I was asking how sure you are that memcpy *always*
copies from low to high.

I don't even know which version of memcpy ends up being used on M7.
Some of them do things like use VIS. I can follow some regular sparc
asm, there's no way I'm even *looking* at that. Is it really ok to use
VIS registers in random contexts?

                           Linus

Re: 4.0.0-rc4: panic in free_block

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sun, 22 Mar 2015 12:47:08 -0700

> Which was why I was asking how sure you are that memcpy *always*
> copies from low to high.

Yeah I'm pretty sure.

> I don't even know which version of memcpy ends up being used on M7.
> Some of them do things like use VIS. I can follow some regular sparc
> asm, there's no way I'm even *looking* at that. Is it really ok to use
> VIS registers in random contexts?

Yes, using VIS how we do is alright, and in fact I did an audit of
this about 1 year ago.  This is another one of those "if this is
wrong, so much stuff would break"

The only thing funny some of these routines do is fetch 2 64-byte
blocks of data ahead in the inner loops, but that should be fine
right?

On the M7 we'll use the Niagara-4 memcpy.

Hmmm... I'll run this silly sparc kernel memmove through the glibc
testsuite and see if it barfs.

Re: 4.0.0-rc4: panic in free_block

[David Ahern]

On 3/22/15 4:23 PM, David Miller wrote:
>> I don't even know which version of memcpy ends up being used on M7.
>> Some of them do things like use VIS. I can follow some regular sparc
>> asm, there's no way I'm even *looking* at that. Is it really ok to use
>> VIS registers in random contexts?
>
> Yes, using VIS how we do is alright, and in fact I did an audit of
> this about 1 year ago.  This is another one of those "if this is
> wrong, so much stuff would break"
>
> The only thing funny some of these routines do is fetch 2 64-byte
> blocks of data ahead in the inner loops, but that should be fine
> right?
>
> On the M7 we'll use the Niagara-4 memcpy.
>
> Hmmm... I'll run this silly sparc kernel memmove through the glibc
> testsuite and see if it barfs.
>

I don't know if you caught Bob's message; he has a hack to bypass memcpy 
and memmove in mm/slab.c use a for loop to move entries. With the hack 
he is not seeing the problem.

This is the hack:

+static void move_entries(void *dest, void *src, int nr)
+{
+       unsigned long *dp = dest;
+       unsigned long *sp = src;
+
+       for (; nr; nr--, dp++, sp++)
+               *dp = *sp;
+}
+

and then replace the mempy and memmove calls in transfer_objects, 
cache_flusharray and drain_array to use move_entries.

I just put it on 4.0.0-rc4 and ditto -- problem goes away, so it clearly 
suggests the memcpy or memmove are the root cause.

David

Re: 4.0.0-rc4: panic in free_block

[David Miller]

From: David Ahern <david.ahern@oracle.com>
Date: Sun, 22 Mar 2015 17:35:49 -0600

> I don't know if you caught Bob's message; he has a hack to bypass
> memcpy and memmove in mm/slab.c use a for loop to move entries. With
> the hack he is not seeing the problem.
> 
> This is the hack:
> 
> +static void move_entries(void *dest, void *src, int nr)
> +{
> +       unsigned long *dp = dest;
> +       unsigned long *sp = src;
> +
> +       for (; nr; nr--, dp++, sp++)
> +               *dp = *sp;
> +}
> +
> 
> and then replace the mempy and memmove calls in transfer_objects,
> cache_flusharray and drain_array to use move_entries.
> 
> I just put it on 4.0.0-rc4 and ditto -- problem goes away, so it
> clearly suggests the memcpy or memmove are the root cause.

Thanks, didn't notice that.

So, something is amuck.

Re: 4.0.0-rc4: panic in free_block

[David Ahern]

On 3/22/15 5:54 PM, David Miller wrote:
>> I just put it on 4.0.0-rc4 and ditto -- problem goes away, so it
>> clearly suggests the memcpy or memmove are the root cause.
>
> Thanks, didn't notice that.
>
> So, something is amuck.

to continue to refine the problem ... I modified only the memmove lines 
(not the memcpy) and it works fine. So its the memmove.

I'm sure this will get whitespaced damaged on the copy and paste but to 
be clear this is the patch I am currently running and system is stable. 
On Friday it failed on every single; with this patch I have allyesconfig 
builds with -j 128 in a loop (clean in between) and nothing -- no panics.

diff --git a/mm/slab.c b/mm/slab.c
index c4b89ea..f5e9716 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -802,6 +802,16 @@ static inline void ac_put_obj(struct kmem_cache 
*cachep, struct array_cache *ac,
         ac->entry[ac->avail++] = objp;
  }

+static void move_entries(void *dest, void *src, int nr)
+{
+       unsigned long *dp = dest;
+       unsigned long *sp = src;
+
+       for (; nr; nr--, dp++, sp++)
+               *dp = *sp;
+}
+
+
  /*
   * Transfer objects in one arraycache to another.
   * Locking must be handled by the caller.
@@ -3344,7 +3354,7 @@ free_done:
         spin_unlock(&n->list_lock);
         slabs_destroy(cachep, &list);
         ac->avail -= batchcount;
-       memmove(ac->entry, &(ac->entry[batchcount]), sizeof(void 
*)*ac->avail);
+       move_entries(ac->entry, &(ac->entry[batchcount]), ac->avail);
  }

  /*
@@ -3817,8 +3827,7 @@ static void drain_array(struct kmem_cache *cachep, 
struct kmem_cache_node *n,
                                 tofree = (ac->avail + 1) / 2;
                         free_block(cachep, ac->entry, tofree, node, &list);
                         ac->avail -= tofree;
-                       memmove(ac->entry, &(ac->entry[tofree]),
-                               sizeof(void *) * ac->avail);
+                       move_entries(ac->entry, &(ac->entry[tofree]), 
ac->avail);
                 }
                 spin_unlock_irq(&n->list_lock);
                 slabs_destroy(cachep, &list);

Re: 4.0.0-rc4: panic in free_block

[David Miller]

From: David Ahern <david.ahern@oracle.com>
Date: Sun, 22 Mar 2015 18:03:30 -0600

> On 3/22/15 5:54 PM, David Miller wrote:
>>> I just put it on 4.0.0-rc4 and ditto -- problem goes away, so it
>>> clearly suggests the memcpy or memmove are the root cause.
>>
>> Thanks, didn't notice that.
>>
>> So, something is amuck.
> 
> to continue to refine the problem ... I modified only the memmove
> lines (not the memcpy) and it works fine. So its the memmove.
> 
> I'm sure this will get whitespaced damaged on the copy and paste but
> to be clear this is the patch I am currently running and system is
> stable. On Friday it failed on every single; with this patch I have
> allyesconfig builds with -j 128 in a loop (clean in between) and
> nothing -- no panics.

Can you just try calling memcpy(), that should work because
I think we agree that if the memcpy() implementation copies
from low to high it should work.

I wonder if the triggering factor is configuring for a high
number of cpus.  I always have NR_CPUS\x128 since that's the
largest machine I have.  I'll give NR_CPUS\x1024 a spin.

Re: 4.0.0-rc4: panic in free_block

[David Miller]

Nevermind I think I figured out the problem.

It's the cache initializing stores, we can't do overlapping
copies where dst <= src in all cases because of them.

A store to a address modulo the cache line size (which for
these instructions is 64 bytes), clears that whole line.

But when we're doing these memmove() calls in SLAB/SLUB, we
can clear some bytes at the end of the line before they've
been read in.

And reading over NG4memcpy, this _can_ happen, the main unrolled
loop begins like this:

	load	src + 0x00
	load	src + 0x08
	load	src + 0x10
	load	src + 0x18
	load	src + 0x20
	store	dst + 0x00

Assume dst is 64 byte aligned and let's say that dst is src - 8 for
this memcpy() call, right?  That store at the end there is the one to
the first line in the cache line, thus clearing the whole line, which
thus clobbers "src + 0x28" before it even gets loaded.

I'm pretty sure this is what's happening.

And it's only going to trigger if the memcpy() is 128 bytes or larger.

I'll work on a fix.

Re: 4.0.0-rc4: panic in free_block

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Sun, 22 Mar 2015 22:19:06 -0400 (EDT)

> I'll work on a fix.

Ok, here is what I committed.   David et al., let me know if you still
see the crashes with this applied.

Of course, I'll queue this up for -stable as well.

Thanks!

==========
[PATCH] sparc64: Fix several bugs in memmove().

Firstly, handle zero length calls properly.  Believe it or not there
are a few of these happening during early boot.

Next, we can't just drop to a memcpy() call in the forward copy case
where dst <= src.  The reason is that the cache initializing stores
used in the Niagara memcpy() implementations can end up clearing out
cache lines before we've sourced their original contents completely.

For example, considering NG4memcpy, the main unrolled loop begins like
this:

     load   src + 0x00
     load   src + 0x08
     load   src + 0x10
     load   src + 0x18
     load   src + 0x20
     store  dst + 0x00

Assume dst is 64 byte aligned and let's say that dst is src - 8 for
this memcpy() call.  That store at the end there is the one to the
first line in the cache line, thus clearing the whole line, which thus
clobbers "src + 0x28" before it even gets loaded.

To avoid this, just fall through to a simple copy only mildly
optimized for the case where src and dst are 8 byte aligned and the
length is a multiple of 8 as well.  We could get fancy and call
GENmemcpy() but this is good enough for how this thing is actually
used.

Reported-by: David Ahern <david.ahern@oracle.com>
Reported-by: Bob Picco <bpicco@meloft.net>
Signed-off-by: David S. Miller <davem@davemloft.net>
---
 arch/sparc/lib/memmove.S | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/arch/sparc/lib/memmove.S b/arch/sparc/lib/memmove.S
index b7f6334..857ad4f 100644
--- a/arch/sparc/lib/memmove.S
+++ b/arch/sparc/lib/memmove.S
@@ -8,9 +8,11 @@
 
 	.text
 ENTRY(memmove) /* o0=dst o1=src o2=len */
-	mov		%o0, %g1
+	brz,pn		%o2, 99f
+	 mov		%o0, %g1
+
 	cmp		%o0, %o1
-	bleu,pt		%xcc, memcpy
+	bleu,pt		%xcc, 2f
 	 add		%o1, %o2, %g7
 	cmp		%g7, %o0
 	bleu,pt		%xcc, memcpy
@@ -24,7 +26,34 @@ ENTRY(memmove) /* o0=dst o1=src o2=len */
 	stb		%g7, [%o0]
 	bne,pt		%icc, 1b
 	 sub		%o0, 1, %o0
-
+99:
 	retl
 	 mov		%g1, %o0
+
+	/* We can't just call memcpy for these memmove cases.  On some
+	 * chips the memcpy uses cache initializing stores and when dst
+	 * and src are close enough, those can clobber the source data
+	 * before we've loaded it in.
+	 */
+2:	or		%o0, %o1, %g7
+	or		%o2, %g7, %g7
+	andcc		%g7, 0x7, %g0
+	bne,pn		%xcc, 4f
+	 nop
+
+3:	ldx		[%o1], %g7
+	add		%o1, 8, %o1
+	subcc		%o2, 8, %o2
+	add		%o0, 8, %o0
+	bne,pt		%icc, 3b
+	 stx		%g7, [%o0 - 0x8]
+	ba,a,pt		%xcc, 99b
+
+4:	ldub		[%o1], %g7
+	add		%o1, 1, %o1
+	subcc		%o2, 1, %o2
+	add		%o0, 1, %o0
+	bne,pt		%icc, 4b
+	 stb		%g7, [%o0 - 0x1]
+	ba,a,pt		%xcc, 99b
 ENDPROC(memmove)
-- 
1.7.11.7

Re: 4.0.0-rc4: panic in free_block

[John Stoffel]

David> ==========
David> [PATCH] sparc64: Fix several bugs in memmove().

David> Firstly, handle zero length calls properly.  Believe it or not there
David> are a few of these happening during early boot.

David> Next, we can't just drop to a memcpy() call in the forward copy case
David> where dst <= src.  The reason is that the cache initializing stores
David> used in the Niagara memcpy() implementations can end up clearing out
David> cache lines before we've sourced their original contents completely.

David> For example, considering NG4memcpy, the main unrolled loop begins like
David> this:

David>      load   src + 0x00
David>      load   src + 0x08
David>      load   src + 0x10
David>      load   src + 0x18
David>      load   src + 0x20
David>      store  dst + 0x00

David> Assume dst is 64 byte aligned and let's say that dst is src - 8 for
David> this memcpy() call.  That store at the end there is the one to the
David> first line in the cache line, thus clearing the whole line, which thus
David> clobbers "src + 0x28" before it even gets loaded.

David> To avoid this, just fall through to a simple copy only mildly
David> optimized for the case where src and dst are 8 byte aligned and the
David> length is a multiple of 8 as well.  We could get fancy and call
David> GENmemcpy() but this is good enough for how this thing is actually
David> used.

Would it make sense to have some memmove()/memcopy() tests on bootup
to catch problems like this?  I know this is a strange case, and
probably not too common, but how hard would it be to wire up tests
that go through 1 to 128 byte memmove() on bootup to make sure things
work properly?

This seems like one of those critical, but subtle things to be
checked.  And doing it only on bootup wouldn't slow anything down and
would (ideally) automatically get us coverage when people add new
archs or update the code.

John

Re: 4.0.0-rc4: panic in free_block

[David Miller]

From: "John Stoffel" <john@stoffel.org>
Date: Mon, 23 Mar 2015 12:51:03 -0400

> Would it make sense to have some memmove()/memcopy() tests on bootup
> to catch problems like this?  I know this is a strange case, and
> probably not too common, but how hard would it be to wire up tests
> that go through 1 to 128 byte memmove() on bootup to make sure things
> work properly?
> 
> This seems like one of those critical, but subtle things to be
> checked.  And doing it only on bootup wouldn't slow anything down and
> would (ideally) automatically get us coverage when people add new
> archs or update the code.

One of two things is already happening.

There have been assembler memcpy/memset development test harnesses
around that most arch developers are using, and those test things
rather extensively.

Also, the memcpy/memset routines on sparc in particular are completely
shared with glibc, we use the same exact code in both trees.  So it's
getting tested there too.

memmove() is just not handled this way.

Re: 4.0.0-rc4: panic in free_block

[John Stoffel]

>>>>> "David" = David Miller <davem@davemloft.net> writes:

David> From: "John Stoffel" <john@stoffel.org>
David> Date: Mon, 23 Mar 2015 12:51:03 -0400

>> Would it make sense to have some memmove()/memcopy() tests on bootup
>> to catch problems like this?  I know this is a strange case, and
>> probably not too common, but how hard would it be to wire up tests
>> that go through 1 to 128 byte memmove() on bootup to make sure things
>> work properly?
>> 
>> This seems like one of those critical, but subtle things to be
>> checked.  And doing it only on bootup wouldn't slow anything down and
>> would (ideally) automatically get us coverage when people add new
>> archs or update the code.

David> One of two things is already happening.

David> There have been assembler memcpy/memset development test harnesses
David> around that most arch developers are using, and those test things
David> rather extensively.

David> Also, the memcpy/memset routines on sparc in particular are completely
David> shared with glibc, we use the same exact code in both trees.  So it's
David> getting tested there too.

Thats' good to know.   I wasn't sure.

David> memmove() is just not handled this way.

Bummers.  So why isn't this covered by the glibc tests too?  Not
accusing, not at all!  Just wondering.  

Thanks for all your work David, I've been amazed at your energy here!

Re: 4.0.0-rc4: panic in free_block

[David Miller]

From: "John Stoffel" <john@stoffel.org>
Date: Mon, 23 Mar 2015 15:56:02 -0400

>>>>>> "David" = David Miller <davem@davemloft.net> writes:
> 
> David> From: "John Stoffel" <john@stoffel.org>
> David> Date: Mon, 23 Mar 2015 12:51:03 -0400
> 
>>> Would it make sense to have some memmove()/memcopy() tests on bootup
>>> to catch problems like this?  I know this is a strange case, and
>>> probably not too common, but how hard would it be to wire up tests
>>> that go through 1 to 128 byte memmove() on bootup to make sure things
>>> work properly?
>>> 
>>> This seems like one of those critical, but subtle things to be
>>> checked.  And doing it only on bootup wouldn't slow anything down and
>>> would (ideally) automatically get us coverage when people add new
>>> archs or update the code.
> 
> David> One of two things is already happening.
> 
> David> There have been assembler memcpy/memset development test harnesses
> David> around that most arch developers are using, and those test things
> David> rather extensively.
> 
> David> Also, the memcpy/memset routines on sparc in particular are completely
> David> shared with glibc, we use the same exact code in both trees.  So it's
> David> getting tested there too.
> 
> Thats' good to know.   I wasn't sure.
> 
> David> memmove() is just not handled this way.
> 
> Bummers.  So why isn't this covered by the glibc tests too?

Because the kernel's memmove() is different from the one we use in glibc
on sparc.  In fact, we use the generic C version in glibc which expands
to forward and backward word copies.

Re: 4.0.0-rc4: panic in free_block

[Linus Torvalds]

On Mon, Mar 23, 2015 at 9:25 AM, David Miller <davem@davemloft.net> wrote:
>
> Ok, here is what I committed.

So I wonder - looking at that assembly, I get the feeling that it
isn't any better code than gcc could generate from simple C code.

Would it perhaps be better to turn memmove() into C?

That's particularly true because if I read this code right, it now
seems to seriously pessimise non-overlapping memmove's, in that it now
*always* uses that slow downward copy if the destination is below the
source.

Now, admittedly, the kernel doesn't use a lot of memmov's, but this
still falls back on the "byte at a time" model for a lot of cases (all
non-64-bit-aligned ones). I could imagine those existing. And some
people (reasonably) hate memcpy because they've been burnt by the
overlapping case and end up using memmove as a "safe alternative", so
it's not necessarily just the overlapping case that might trigger
this.

Maybe the code could be something like

    void *memmove(void *dst, const void *src, size_t n);
    {
        // non-overlapping cases
        if (src + n <= dst)
            return memcpy(dst, src, n);
        if (dst + n <= src)
            return memcpy(dst, src, n);

        // overlapping, but we know we
        //  (a) copy upwards
        //  (b) initialize the result in at most chunks of 64
        if (dst+64 <= src)
            return memcpy(dst, src, n);

        .. do the backwards thing ..
    }

(ok, maybe I got it wrong, but you get the idea).

I *think* gcc should do ok on the above kind of code, and not generate
wildly different code from your handcoded version.

                            Linus

Re: 4.0.0-rc4: panic in free_block

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Mon, 23 Mar 2015 10:00:02 -0700

> Maybe the code could be something like
> 
>     void *memmove(void *dst, const void *src, size_t n);
>     {
>         // non-overlapping cases
>         if (src + n <= dst)
>             return memcpy(dst, src, n);
>         if (dst + n <= src)
>             return memcpy(dst, src, n);
> 
>         // overlapping, but we know we
>         //  (a) copy upwards
>         //  (b) initialize the result in at most chunks of 64
>         if (dst+64 <= src)
>             return memcpy(dst, src, n);
> 
>         .. do the backwards thing ..
>     }
> 
> (ok, maybe I got it wrong, but you get the idea).
> 
> I *think* gcc should do ok on the above kind of code, and not generate
> wildly different code from your handcoded version.

Sure you could do that in C, but I really want to avoid using memcpy()
if dst and src overlap in any way at all.

Said another way, I don't want to codify that "64" thing.  The next
chip could do 128 byte initializing stores.

Re: 4.0.0-rc4: panic in free_block

[Linus Torvalds]

On Mon, Mar 23, 2015 at 12:08 PM, David Miller <davem@davemloft.net> wrote:
>
> Sure you could do that in C, but I really want to avoid using memcpy()
> if dst and src overlap in any way at all.
>
> Said another way, I don't want to codify that "64" thing.  The next
> chip could do 128 byte initializing stores.

But David, THAT IS NOT WHAT YOUR BROKEN ASM DOES ANYWAY!

Read it again. Your asm code does not check for overlap. Look at this:

        cmp             %o0, %o1
        bleu,pt         %xcc, 2f

and ponder. It's wrong.

So even if you don't want to take that "allow overlap more than 64
bytes apart" thing, my C version actually is *better* than the broken
asm version you have.

The new asm version is better than the old one, because the new
breakage is about really bad performance rather than actively
breaking, but still..

                         Linus

Re: 4.0.0-rc4: panic in free_block

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Mon, 23 Mar 2015 12:47:49 -0700

> On Mon, Mar 23, 2015 at 12:08 PM, David Miller <davem@davemloft.net> wrote:
>>
>> Sure you could do that in C, but I really want to avoid using memcpy()
>> if dst and src overlap in any way at all.
>>
>> Said another way, I don't want to codify that "64" thing.  The next
>> chip could do 128 byte initializing stores.
> 
> But David, THAT IS NOT WHAT YOUR BROKEN ASM DOES ANYWAY!
> 
> Read it again. Your asm code does not check for overlap. Look at this:
> 
>         cmp             %o0, %o1
>         bleu,pt         %xcc, 2f
> 
> and ponder. It's wrong.

Right, it's not checking for overlap.  It's checking for "does a
forward copy work?"

That's the standard test for this, and it's what glibc uses in it's
generic memmove() implementation FWIW.  (granted, I know glibc is not
generally a good source for "right way to do things :-)

> The new asm version is better than the old one, because the new
> breakage is about really bad performance rather than actively
> breaking, but still..

I accept that it's suboptimal.

Re: 4.0.0-rc4: panic in free_block

[David Ahern]

On 3/23/15 10:25 AM, David Miller wrote:
> [PATCH] sparc64: Fix several bugs in memmove().
>
> Firstly, handle zero length calls properly.  Believe it or not there
> are a few of these happening during early boot.
>
> Next, we can't just drop to a memcpy() call in the forward copy case
> where dst <= src.  The reason is that the cache initializing stores
> used in the Niagara memcpy() implementations can end up clearing out
> cache lines before we've sourced their original contents completely.
>
> For example, considering NG4memcpy, the main unrolled loop begins like
> this:
>
>       load   src + 0x00
>       load   src + 0x08
>       load   src + 0x10
>       load   src + 0x18
>       load   src + 0x20
>       store  dst + 0x00
>
> Assume dst is 64 byte aligned and let's say that dst is src - 8 for
> this memcpy() call.  That store at the end there is the one to the
> first line in the cache line, thus clearing the whole line, which thus
> clobbers "src + 0x28" before it even gets loaded.
>
> To avoid this, just fall through to a simple copy only mildly
> optimized for the case where src and dst are 8 byte aligned and the
> length is a multiple of 8 as well.  We could get fancy and call
> GENmemcpy() but this is good enough for how this thing is actually
> used.
>
> Reported-by: David Ahern <david.ahern@oracle.com>
> Reported-by: Bob Picco <bpicco@meloft.net>
> Signed-off-by: David S. Miller <davem@davemloft.net>

seems like a formality at this point, but this resolves the panic on the 
M7-based ldom and baremetal. The T5-8 failed to boot, but it could be a 
different problem.

Thanks for the fast turnaround,
David

Re: 4.0.0-rc4: panic in free_block

[David Miller]

From: David Ahern <david.ahern@oracle.com>
Date: Mon, 23 Mar 2015 11:34:34 -0600

> seems like a formality at this point, but this resolves the panic on
> the M7-based ldom and baremetal. The T5-8 failed to boot, but it could
> be a different problem.

Specifically, does the T5-8 boot without my patch applied?

Re: 4.0.0-rc4: panic in free_block

[David Ahern]

On 3/23/15 1:35 PM, David Miller wrote:
> From: David Ahern <david.ahern@oracle.com>
> Date: Mon, 23 Mar 2015 11:34:34 -0600
>
>> seems like a formality at this point, but this resolves the panic on
>> the M7-based ldom and baremetal. The T5-8 failed to boot, but it could
>> be a different problem.
>
> Specifically, does the T5-8 boot without my patch applied?
>

I am running around in circles with it... it takes 15 minutes after a 
hard reset to get logged in, and I forgot that the 2.6.39 can't handle 
-j 1024 either (task scheduler problem), and then I wasted time waiting 
for sandwich shop to learn how to use mobile app ordering, ...

I'll respond as soon as I can.

Re: 4.0.0-rc4: panic in free_block

[David Ahern]

On 3/23/15 1:35 PM, David Miller wrote:
> From: David Ahern <david.ahern@oracle.com>
> Date: Mon, 23 Mar 2015 11:34:34 -0600
>
>> seems like a formality at this point, but this resolves the panic on
>> the M7-based ldom and baremetal. The T5-8 failed to boot, but it could
>> be a different problem.
>
> Specifically, does the T5-8 boot without my patch applied?
>

The T5-8 is having problems; has to be unrelated to this commit. T5-2 
(256 cpus) boots fine, and make -j 256 on an allyesconfig builds fine.

Re: 4.0.0-rc4: panic in free_block

[Bob Picco]

David Miller wrote:	[Mon Mar 23 2015, 12:25:30PM EDT]
> From: David Miller <davem@davemloft.net>
> Date: Sun, 22 Mar 2015 22:19:06 -0400 (EDT)
> 
> > I'll work on a fix.
> 
> Ok, here is what I committed.   David et al., let me know if you still
> see the crashes with this applied.
> 
> Of course, I'll queue this up for -stable as well.
> 
> Thanks!
> 
> ==========
> [PATCH] sparc64: Fix several bugs in memmove().
> 
> Firstly, handle zero length calls properly.  Believe it or not there
> are a few of these happening during early boot.
> 
> Next, we can't just drop to a memcpy() call in the forward copy case
> where dst <= src.  The reason is that the cache initializing stores
> used in the Niagara memcpy() implementations can end up clearing out
> cache lines before we've sourced their original contents completely.
> 
> For example, considering NG4memcpy, the main unrolled loop begins like
> this:
> 
>      load   src + 0x00
>      load   src + 0x08
>      load   src + 0x10
>      load   src + 0x18
>      load   src + 0x20
>      store  dst + 0x00
> 
> Assume dst is 64 byte aligned and let's say that dst is src - 8 for
> this memcpy() call.  That store at the end there is the one to the
> first line in the cache line, thus clearing the whole line, which thus
> clobbers "src + 0x28" before it even gets loaded.
> 
> To avoid this, just fall through to a simple copy only mildly
> optimized for the case where src and dst are 8 byte aligned and the
> length is a multiple of 8 as well.  We could get fancy and call
> GENmemcpy() but this is good enough for how this thing is actually
> used.
> 
> Reported-by: David Ahern <david.ahern@oracle.com>
> Reported-by: Bob Picco <bpicco@meloft.net>
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
Seems solid with 2.6.39 on M7-4. Jalap?no is happy with current sparc.git.

Re: 4.0.0-rc4: panic in free_block

[David Miller]

From: Bob Picco <bpicco@meloft.net>
Date: Tue, 24 Mar 2015 10:57:53 -0400

> Seems solid with 2.6.39 on M7-4. Jalap?no is happy with current sparc.git.

Thanks for all the testing, it's been integrated into the -stable
queues as well.

Re: 4.0.0-rc4: panic in free_block

[Linus Torvalds]

On Sun, Mar 22, 2015 at 3:23 PM, David Miller <davem@davemloft.net> wrote:
>
> Yes, using VIS how we do is alright, and in fact I did an audit of
> this about 1 year ago.  This is another one of those "if this is
> wrong, so much stuff would break"

Maybe. But it does seem like Bob Picco has narrowed it down to memmove().

It also bothers me enormously - and perhaps unreasonably - how that
memcpy code has memory barriers in it. I can see _zero_ reason for a
memory barrier inside a memcpy, unless the memcpy does something that
isn't valid to begin with. Are the VIS operatiosn perhaps using some
kind of non-temporal form that doesn't follow the TSO rules? Kind of
like the "movnt" that Intel has?

That kind of stuff makes me worry. For example, the only reason I see for

        membar          #StoreLoad | #StoreStore

after that VIS loop is that the stxa doesn't honor normal memory store
ordering rules, but if that's true, then shouldn't we have a membar
*before* the loop too? How about "ldx"? Does that also do some
unordered loads?

So the memory barriers in there just make me nervous, because they are
either entirely bogus or superfluous, or they are not - and if they
aren't, then that implies that some of the code does something really
odd with memory ordering.

I dunno. I really can't read that code at all, so I'm going entirely
by gut instinct here.

                          Linus

Re: 4.0.0-rc4: panic in free_block

[David Miller]

From: Linus Torvalds <torvalds@linux-foundation.org>
Date: Sun, 22 Mar 2015 16:49:51 -0700

> On Sun, Mar 22, 2015 at 3:23 PM, David Miller <davem@davemloft.net> wrote:
>>
>> Yes, using VIS how we do is alright, and in fact I did an audit of
>> this about 1 year ago.  This is another one of those "if this is
>> wrong, so much stuff would break"
> 
> Maybe. But it does seem like Bob Picco has narrowed it down to memmove().
> 
> It also bothers me enormously - and perhaps unreasonably - how that
> memcpy code has memory barriers in it. I can see _zero_ reason for a
> memory barrier inside a memcpy, unless the memcpy does something that
> isn't valid to begin with. Are the VIS operatiosn perhaps using some
> kind of non-temporal form that doesn't follow the TSO rules? Kind of
> like the "movnt" that Intel has?

The special cache line clearing stores (the ASI_BLK_INIT_QUAD_LDD_P
stuff) do not adhere to the memory model.

I had elided the memory barriers originally, but it caused memory
corruption.

So yes, this is similar to the 'movnt' situation.