LMB regression...

LMB regression...

[David Miller]

Paul, I finally got around to testing your changeset on sparc64, it
breaks things:

commit d9024df02ffe74d723d97d552f86de3b34beb8cc
Author: Paul Mackerras <paulus@samba.org>
Date:   Sat Apr 12 15:20:59 2008 +1000

    [LMB] Restructure allocation loops to avoid unsigned underflow
 ...    

Specifically, you removed the aligning of the size argument given to
lmb_add_region() in the lmb allocators, and that is critical when
allocating many small chunks, we run out of LMB slots otherwise
and allocations start failing.

I added the alignment there as a bug fix earlier:

commit eea89e13a9c61d3928223d2f9bf2295e22e0efb6
Author: David S. Miller <davem@davemloft.net>
Date:   Wed Feb 13 16:57:09 2008 -0800

    [LMB]: Fix bug in __lmb_alloc_base().
    
    We need to check lmb_add_region() for errors, it can run out
    of regions etc.
    
    Also, the size needs to be padded to the given alignment
    or else the lmb.reserved regions don't get expanded and
    instead we get tons of holes and eventually run out of
    regions prematurely.
    
    Signed-off-by: David S. Miller <davem@davemloft.net>

Please be more careful in the future :-(

I find it quite ironic that you spent so much time and effort fixing
an absolutely totally theoretical bug that nobody was triggering, and
in the process reintroduced a real one that triggers immediately on
real systems, and that had even been explicitly fixed previously.

Re: LMB regression...

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1900 bytes --]

On Wed, 2008-04-23 at 23:24 -0700, David Miller wrote:
> Paul, I finally got around to testing your changeset on sparc64, it
> breaks things:
> 
> commit d9024df02ffe74d723d97d552f86de3b34beb8cc
> Author: Paul Mackerras <paulus@samba.org>
> Date:   Sat Apr 12 15:20:59 2008 +1000
> 
>     [LMB] Restructure allocation loops to avoid unsigned underflow
>  ...    
> 
> Specifically, you removed the aligning of the size argument given to
> lmb_add_region() in the lmb allocators, and that is critical when
> allocating many small chunks, we run out of LMB slots otherwise
> and allocations start failing.
> 
> I added the alignment there as a bug fix earlier:
> 
> commit eea89e13a9c61d3928223d2f9bf2295e22e0efb6
> Author: David S. Miller <davem@davemloft.net>
> Date:   Wed Feb 13 16:57:09 2008 -0800
> 
>     [LMB]: Fix bug in __lmb_alloc_base().
>     
>     We need to check lmb_add_region() for errors, it can run out
>     of regions etc.
>     
>     Also, the size needs to be padded to the given alignment
>     or else the lmb.reserved regions don't get expanded and
>     instead we get tons of holes and eventually run out of
>     regions prematurely.
>     
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> Please be more careful in the future :-(
> 
> I find it quite ironic that you spent so much time and effort fixing
> an absolutely totally theoretical bug that nobody was triggering, and
> in the process reintroduced a real one that triggers immediately on
> real systems, and that had even been explicitly fixed previously.

Sounds like we need a test suite :)

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: LMB regression...

[David Miller]

From: Michael Ellerman <michael@ellerman.id.au>
Date: Thu, 24 Apr 2008 16:35:33 +1000

> Sounds like we need a test suite :)

Maybe :-)  Anyways, here is the bug fix I plan to push to
Linus with my sparc64 NUMA changes, unless someone has an
objection:

[LMB]: Fix lmb allocation regression.

Changeset d9024df02ffe74d723d97d552f86de3b34beb8cc ("[LMB] Restructure
allocation loops to avoid unsigned underflow") removed the alignment
of the 'size' argument to call lmb_add_region() done by __lmb_alloc_base().

In doing so it reintroduced the bug fixed by changeset
eea89e13a9c61d3928223d2f9bf2295e22e0efb6 ("[LMB]: Fix bug in
__lmb_alloc_base().").

This puts it back.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 lib/lmb.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/lib/lmb.c b/lib/lmb.c
index 896e283..207147a 100644
--- a/lib/lmb.c
+++ b/lib/lmb.c
@@ -346,7 +346,7 @@ u64 __init __lmb_alloc_base(u64 size, u64 align, u64 max_addr)
 			if (j < 0) {
 				/* this area isn't reserved, take it */
 				if (lmb_add_region(&lmb.reserved, base,
-						   size) < 0)
+						   lmb_align_up(size, align)) < 0)
 					return 0;
 				return base;
 			}
-- 
1.5.5.1.57.g5909c

Re: LMB regression...

[Paul Mackerras]

David Miller writes:

> Specifically, you removed the aligning of the size argument given to
> lmb_add_region() in the lmb allocators, and that is critical when
> allocating many small chunks, we run out of LMB slots otherwise
> and allocations start failing.

Sorry.  It's still there in lmb_alloc_nid_unreserved but I lost the
one in __lmb_alloc_base.  Are you going to send a fix to Linus, or
will I include it in the pull request I'm going to send tomorrow?

> I find it quite ironic that you spent so much time and effort fixing
> an absolutely totally theoretical bug that nobody was triggering, and
> in the process reintroduced a real one that triggers immediately on
> real systems, and that had even been explicitly fixed previously.

In fact the bug should be relatively easily triggerable in
lmb_alloc_nid as far as I can see, since it scans upwards
from zero.  It depends a bit on how memory is distributed across
nodes, but it you are allocating on the node that has the memory
starting at physical address 0, and all of the first contiguous chunk
on that node is reserved, then I think you'll hit the bug.  The only
saving grace is that lmb_alloc_nid is presently unused.

Paul.

Re: LMB regression...

[David Miller]

From: Paul Mackerras <paulus@samba.org>
Date: Thu, 24 Apr 2008 16:56:26 +1000

> David Miller writes:
> 
> > Specifically, you removed the aligning of the size argument given to
> > lmb_add_region() in the lmb allocators, and that is critical when
> > allocating many small chunks, we run out of LMB slots otherwise
> > and allocations start failing.
> 
> Sorry.  It's still there in lmb_alloc_nid_unreserved but I lost the
> one in __lmb_alloc_base.  Are you going to send a fix to Linus, or
> will I include it in the pull request I'm going to send tomorrow?

I've got it, see my other email.