[PATCH] nommu: fix kobjsize() for SLOB and SLUB

[PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Pekka J Enberg]

From: Christoph Lameter <clameter@sgi.com>

As reported by Paul Mundt, kobjsize() does not work properly with SLOB and SLUB
that re-use parts of struct page for their own purposes. Fix it up by using
compound_order() for compound pages that don't have PageSlab set.

Reported-by: Paul Mundt <lethal@linux-sh.org>
Cc: Christoph Lameter <clameter@sgi.com>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---
I kept the original object size calculation for non-compound pages in this 
version. It looks like the nommu code uses kobjsize() for all sorts of 
interesting things.

 mm/nommu.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

Index: slab-2.6/mm/nommu.c
===================================================================
--- slab-2.6.orig/mm/nommu.c	2008-05-22 18:59:01.000000000 +0300
+++ slab-2.6/mm/nommu.c	2008-05-22 19:00:36.000000000 +0300
@@ -109,12 +109,22 @@
 	 * If the object we have should not have ksize performed on it,
 	 * return size of 0
 	 */
-	if (!objp || (unsigned long)objp >= memory_end || !((page = virt_to_page(objp))))
+	if (!objp)
+		return 0;
+
+	if ((unsigned long) objp >= memory_end)
+		return 0;
+
+	page = virt_to_head_page(objp);
+	if (!page)
 		return 0;
 
 	if (PageSlab(page))
 		return ksize(objp);
 
+	if (PageCompound(page))
+		return PAGE_SIZE << compound_order(page);
+
 	BUG_ON(page->index < 0);
 	BUG_ON(page->index >= MAX_ORDER);
 

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Christoph Lameter]

On Thu, 22 May 2008, Pekka J Enberg wrote:

>  	BUG_ON(page->index < 0);
>  	BUG_ON(page->index >= MAX_ORDER);

Would someone explain to me what this is supposed to be doing?

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Paul Mundt]

On Thu, May 22, 2008 at 07:09:04PM +0300, Pekka J Enberg wrote:
> Index: slab-2.6/mm/nommu.c
> ===================================================================
> --- slab-2.6.orig/mm/nommu.c	2008-05-22 18:59:01.000000000 +0300
> +++ slab-2.6/mm/nommu.c	2008-05-22 19:00:36.000000000 +0300
> @@ -109,12 +109,22 @@
>  	 * If the object we have should not have ksize performed on it,
>  	 * return size of 0
>  	 */
> -	if (!objp || (unsigned long)objp >= memory_end || !((page = virt_to_page(objp))))
> +	if (!objp)
> +		return 0;
> +
> +	if ((unsigned long) objp >= memory_end)
> +		return 0;
> +
> +	page = virt_to_head_page(objp);
> +	if (!page)
>  		return 0;
>  
>  	if (PageSlab(page))
>  		return ksize(objp);
>  
Is ksize() happy with taking the head page instead of virt_to_page(objp)?

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Christoph Lameter]

On Fri, 23 May 2008, Paul Mundt wrote:

> Is ksize() happy with taking the head page instead of virt_to_page(objp)?

ksize() takes a pointer to an object. ksize() cannot take a the pointer to 
a page struct.

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Paul Mundt]

On Thu, May 22, 2008 at 04:45:55PM -0700, Christoph Lameter wrote:
> On Fri, 23 May 2008, Paul Mundt wrote:
> > Is ksize() happy with taking the head page instead of virt_to_page(objp)?
> 
> ksize() takes a pointer to an object. ksize() cannot take a the pointer to 
> a page struct.
> 
Ah, so it does. In that case, we still need to kill off the page->index
BUG_ON() bits, while PageCompound() will take care of the compound order
bits that the page->index shifting seems to have been trying to
implement, the BUG_ON() will still trigger for PAGE_SIZE page cache
pages.

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Christoph Lameter]

On Fri, 23 May 2008, Paul Mundt wrote:

> Ah, so it does. In that case, we still need to kill off the page->index
> BUG_ON() bits, while PageCompound() will take care of the compound order
> bits that the page->index shifting seems to have been trying to
> implement, the BUG_ON() will still trigger for PAGE_SIZE page cache
> pages.

Yes. I have no idea what the page->index check does there. The compound 
page order is not stored in the index field.
 

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[David Howells]

Pekka J Enberg <penberg@cs.helsinki.fi> wrote:

> From: Christoph Lameter <clameter@sgi.com>
> 
> As reported by Paul Mundt, kobjsize() does not work properly with SLOB and SLUB
> that re-use parts of struct page for their own purposes. Fix it up by using
> compound_order() for compound pages that don't have PageSlab set.
> 
> Reported-by: Paul Mundt <lethal@linux-sh.org>
> Cc: Christoph Lameter <clameter@sgi.com>
> Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
> ---
> I kept the original object size calculation for non-compound pages in this 
> version. It looks like the nommu code uses kobjsize() for all sorts of 
> interesting things.

Works for SLAB and SLUB.  SLOB breaks well before getting to anything these
patches affect.

Acked-by: David Howells <dhowells@redhat.com>

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Pekka J Enberg]

On Wed, 28 May 2008, David Howells wrote:
> Works for SLAB and SLUB.  SLOB breaks well before getting to anything these
> patches affect.
> 
> Acked-by: David Howells <dhowells@redhat.com>

Thanks for testing! Where do I send these patches though? There's no entry 
for no-MMU in MAINTAINERS. I guess I could just send them to Andrew...

		Pekka

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[David Howells]

Pekka J Enberg <penberg@cs.helsinki.fi> wrote:

> Thanks for testing! Where do I send these patches though? There's no entry 
> for no-MMU in MAINTAINERS. I guess I could just send them to Andrew...

Andrew and/or Linus.

David

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> Works for SLAB and SLUB.  SLOB breaks well before getting to anything these
> patches affect.

Okay...  The problem with SLOB is that it can return large objects (8Kb in this
case) that aren't 8-byte aligned.  This screws up FRV as it expects to be able
to use load and store-double instructions, which fault if they don't get
8-byte (double register) alignment.

David

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Christoph Lameter]

On Wed, 28 May 2008, David Howells wrote:

> David Howells <dhowells@redhat.com> wrote:
> 
> > Works for SLAB and SLUB.  SLOB breaks well before getting to anything these
> > patches affect.
> 
> Okay...  The problem with SLOB is that it can return large objects (8Kb in this
> case) that aren't 8-byte aligned.  This screws up FRV as it expects to be able
> to use load and store-double instructions, which fault if they don't get
> 8-byte (double register) alignment.

Right. SLAB/SLUB align to long long. SLOB needs to do the same.

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[David Howells]

Christoph Lameter <clameter@sgi.com> wrote:

> Right. SLAB/SLUB align to long long. SLOB needs to do the same.

I fixed it by setting ARCH_KMALLOC_MINALIGN and ARCH_SLAB_MINALIGN.

David

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Mike Frysinger]

On Wed, May 28, 2008 at 1:38 PM, David Howells <dhowells@redhat.com> wrote:
> Christoph Lameter <clameter@sgi.com> wrote:
>> Right. SLAB/SLUB align to long long. SLOB needs to do the same.
>
> I fixed it by setting ARCH_KMALLOC_MINALIGN and ARCH_SLAB_MINALIGN.

what was the change exactly ?  we were seeing alignment problems on
Blackfin and we fixed it with:
http://blackfin.uclinux.org/git/?p=readonly-mirrors/linux-kernel.git;a=commitdiff;h=506175f9d4c8ea43ce5687edb98feb3475c45ea7
-mike

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[David Howells]

Mike Frysinger <vapier.adi@gmail.com> wrote:

> > I fixed it by setting ARCH_KMALLOC_MINALIGN and ARCH_SLAB_MINALIGN.
> 
> what was the change exactly ?

The attached patch.

David
---
[PATCH] FRV: Specify the minimum slab/kmalloc alignment

From: David Howells <dhowells@redhat.com>

Specify the minimum slab/kmalloc alignment to be 8 bytes.  This fixes a crash
when SLOB is selected as the memory allocator.  The FRV arch needs this so
that it can use the load- and store-double instructions without faulting.  By
default SLOB sets the minimum to be 4 bytes.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 include/asm-frv/mem-layout.h |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)


diff --git a/include/asm-frv/mem-layout.h b/include/asm-frv/mem-layout.h
index 734a1d0..2947764 100644
--- a/include/asm-frv/mem-layout.h
+++ b/include/asm-frv/mem-layout.h
@@ -31,6 +31,13 @@
 
 #define PAGE_MASK			(~(PAGE_SIZE-1))
 
+/*
+ * the slab must be aligned such that load- and store-double instructions don't
+ * fault if used
+ */
+#define	ARCH_KMALLOC_MINALIGN		8
+#define	ARCH_SLAB_MINALIGN		8
+
 /*****************************************************************************/
 /*
  * virtual memory layout from kernel's point of view

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Mike Frysinger]

On Thu, May 29, 2008 at 9:03 AM, David Howells wrote:
> Mike Frysinger <vapier.adi@gmail.com> wrote:
>> > I fixed it by setting ARCH_KMALLOC_MINALIGN and ARCH_SLAB_MINALIGN.
>>
>> what was the change exactly ?
>
> The attached patch.
>
> diff --git a/include/asm-frv/mem-layout.h b/include/asm-frv/mem-layout.h
> +#define        ARCH_KMALLOC_MINALIGN           8
> +#define        ARCH_SLAB_MINALIGN              8

yeah, we were already doing that ... and the problem we had that i
referred to seems to be merged already into mainline, so you shouldnt
run into it unless you're using older kernel versions
-mike

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Christoph Lameter]

On Thu, 29 May 2008, Mike Frysinger wrote:

> On Thu, May 29, 2008 at 9:03 AM, David Howells wrote:
> > Mike Frysinger <vapier.adi@gmail.com> wrote:
> >> > I fixed it by setting ARCH_KMALLOC_MINALIGN and ARCH_SLAB_MINALIGN.
> >>
> >> what was the change exactly ?
> >
> > The attached patch.
> >
> > diff --git a/include/asm-frv/mem-layout.h b/include/asm-frv/mem-layout.h
> > +#define        ARCH_KMALLOC_MINALIGN           8
> > +#define        ARCH_SLAB_MINALIGN              8
> 
> yeah, we were already doing that ... and the problem we had that i
> referred to seems to be merged already into mainline, so you shouldnt
> run into it unless you're using older kernel versions

You should not have to do this. This is the default alignment that also 
SLOB needs to follow. We need to align structures correctly for access to 
long long's.

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Mike Frysinger]

On Thu, May 29, 2008 at 4:30 PM, Christoph Lameter wrote:
> On Thu, 29 May 2008, Mike Frysinger wrote:
>> On Thu, May 29, 2008 at 9:03 AM, David Howells wrote:
>> > Mike Frysinger <vapier.adi@gmail.com> wrote:
>> >> > I fixed it by setting ARCH_KMALLOC_MINALIGN and ARCH_SLAB_MINALIGN.
>> >>
>> >> what was the change exactly ?
>> >
>> > The attached patch.
>> >
>> > diff --git a/include/asm-frv/mem-layout.h b/include/asm-frv/mem-layout.h
>> > +#define        ARCH_KMALLOC_MINALIGN           8
>> > +#define        ARCH_SLAB_MINALIGN              8
>>
>> yeah, we were already doing that ... and the problem we had that i
>> referred to seems to be merged already into mainline, so you shouldnt
>> run into it unless you're using older kernel versions
>
> You should not have to do this. This is the default alignment that also
> SLOB needs to follow. We need to align structures correctly for access to
> long long's.

are you saying that slob is broken ?  i see in mm/slob.c:
#ifndef ARCH_KMALLOC_MINALIGN
#define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long)
#endif
-mike

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Christoph Lameter]

On Thu, 29 May 2008, Mike Frysinger wrote:

> > You should not have to do this. This is the default alignment that also
> > SLOB needs to follow. We need to align structures correctly for access to
> > long long's.
> 
> are you saying that slob is broken ?  i see in mm/slob.c:
> #ifndef ARCH_KMALLOC_MINALIGN
> #define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long)
> #endif
> -mike

It should be long long instead.

SLUB:

#ifndef ARCH_KMALLOC_MINALIGN
#define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long long)
#endif

SLAB:

#define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long long)
 

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Mike Frysinger]

On Thu, May 29, 2008 at 5:29 PM, Christoph Lameter wrote:
> On Thu, 29 May 2008, Mike Frysinger wrote:
>> > You should not have to do this. This is the default alignment that also
>> > SLOB needs to follow. We need to align structures correctly for access to
>> > long long's.
>>
>> are you saying that slob is broken ?  i see in mm/slob.c:
>> #ifndef ARCH_KMALLOC_MINALIGN
>> #define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long)
>> #endif
>
> It should be long long instead.
>
> SLUB:
>
> #ifndef ARCH_KMALLOC_MINALIGN
> #define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long long)
> #endif
>
> SLAB:
>
> #define ARCH_KMALLOC_MINALIGN __alignof__(unsigned long long)

will you post a patch to do that (and ideally delete all of the
occurrences in asm-*/ where the minalign is defined to 8) ?
-mike

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> Works for SLAB and SLUB.  SLOB breaks well before getting to anything these
> patches affect.

Okay, I've fixed things so that it gets things aligned properly with SLOB.
This patch doesn't work for SLOB because SLOB doesn't set PG_slab on its
pages.  This causes:

	kernel BUG at mm/nommu.c:129!

Or, as gdb sees it:

Break 00000002

Program received signal SIGABRT, Aborted.
0xc0055498 in kobjsize (objp=<value optimized out>) at mm/nommu.c:129
(gdb) list
124	
125		if (PageCompound(page))
126			return PAGE_SIZE << compound_order(page);
127	
128		BUG_ON(page->index < 0);
129		BUG_ON(page->index >= MAX_ORDER);
130	
131		return (PAGE_SIZE << page->index);
132	}
133	
(gdb) bt
#0  0xc0055498 in kobjsize (objp=<value optimized out>) at mm/nommu.c:129
#1  0xc0056408 in do_mmap_pgoff (file=0xc08f7d00, addr=3231711232, len=536128, prot=5, 
    flags=3758096383, pgoff=<value optimized out>) at mm/nommu.c:1010
#2  0xc008b988 in elf_fdpic_map_file (params=0xc0849eac, file=0xc08f7d00, mm=0xc0989ee0, 
    what=0xc01cc00c "executable") at mm.h:1105
#3  0xc008cb98 in load_elf_fdpic_binary (bprm=0xc0988e68, regs=<value optimized out>)
    at fs/binfmt_elf_fdpic.c:345
#4  0xc0060d90 in search_binary_handler (bprm=0xc0988e68, regs=0xc0849f90) at fs/exec.c:1215
#5  0xc0061d24 in do_execve (filename=<value optimized out>, argv=0xc01e1c38, envp=0xc01e1bb0, 
    regs=0xc0849f90) at fs/exec.c:1327
#6  0xc000aab0 in sys_execve (name=<value optimized out>, argv=0xc01e1c38, envp=0xc01e1bb0)
    at arch/frv/kernel/process.c:267
#7  0xc00095e0 in __syscall_call () at arch/frv/kernel/entry.S:898
#8  0xc00095e0 in __syscall_call () at arch/frv/kernel/entry.S:898
(gdb) p/x *page
$3 = {flags = 0x40000840, _count = {counter = 0x1}, {_mapcount = {counter = 0x1cfbffff}, {
      inuse = 0x1cfb, objects = 0xffff}}, {{private = 0x0, mapping = 0x0}, slab = 0x0, 
    first_page = 0x0}, {index = 0xc0980056, freelist = 0xc0980056}, lru = {next = 0xc08107b8, 
    prev = 0xc01e51f0}}
(gdb) up
#1  0xc0056408 in do_mmap_pgoff (file=0xc08f7d00, addr=3231711232, len=536128, prot=5, 
    flags=3758096383, pgoff=<value optimized out>) at mm/nommu.c:1010
(gdb) list
1005		if (vma->vm_flags & VM_MAPPED_COPY) {
1006			realalloc += kobjsize(result);
1007			askedalloc += len;
1008		}
1009	
1010		realalloc += kobjsize(vma);
1011		askedalloc += sizeof(*vma);
1012	
1013		current->mm->total_vm += len >> PAGE_SHIFT;
1014	


PG_active is set (0x00000040), but not PG_slab (0x00000080).  The VMA object
in question is allocated using kzalloc().

David

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Pekka Enberg]

(cc'ing linux-kernel)

Paul Mundt wrote:
> I have no idea why you are maintaining the page->index case here, it's
> completely broken for page cache pages. Unless someone more familiar with
> the page->index BUG_ON() tests objects, the BUG_ON() there should be
> removed completely. Neither Christoph nor I were able to work out what the
> point of the page->index tests were, and no one else has spoken up about
> them, so it's likely safe to treat them as bogus.

We use kobjsize() for pointers returned from do_mmap() which is why I 
kept the page->index case. Are we using PageCompound for those as well?

Paul Mundt wrote:
> SLOB on nommu will oops on any page cache page in the current git kernels
> until those BUG_ON()'s are killed. The more distressing thing is the fact
> that the blackfin people have been patching SLOB when it's obvious that
> they haven't been testing (ie, even booting!) it.

Hmm...

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Christoph Lameter]

On Wed, 28 May 2008, Pekka Enberg wrote:

> (cc'ing linux-kernel)
> 
> Paul Mundt wrote:
> > I have no idea why you are maintaining the page->index case here, it's
> > completely broken for page cache pages. Unless someone more familiar with
> > the page->index BUG_ON() tests objects, the BUG_ON() there should be
> > removed completely. Neither Christoph nor I were able to work out what the
> > point of the page->index tests were, and no one else has spoken up about
> > them, so it's likely safe to treat them as bogus.
> 
> We use kobjsize() for pointers returned from do_mmap() which is why I kept the
> page->index case. Are we using PageCompound for those as well?

What does do_mmap have to do with page->index?

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Pekka Enberg]

Christoph Lameter wrote:
>> We use kobjsize() for pointers returned from do_mmap() which is why I kept the
>> page->index case. Are we using PageCompound for those as well?
> 
> What does do_mmap have to do with page->index?

I don't know how the nommu code is supposed to work. I simply assumed it 
has a reason for being there and that compound_order() is not enough to 
handle that.

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Christoph Lameter]

On Wed, 28 May 2008, Pekka Enberg wrote:

> Christoph Lameter wrote:
> > > We use kobjsize() for pointers returned from do_mmap() which is why I kept
> > > the
> > > page->index case. Are we using PageCompound for those as well?
> > 
> > What does do_mmap have to do with page->index?
> 
> I don't know how the nommu code is supposed to work. I simply assumed it has a
> reason for being there and that compound_order() is not enough to handle that.

Could the nommu people either tell us what this is all about? Or junk the 
code. This looks very wrong.

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[David Howells]

Christoph Lameter <clameter@sgi.com> wrote:

> Could the nommu people either tell us what this is all about? Or junk the 
> code. This looks very wrong.

ELF-FDPIC is currently using kobjsize() so that it can expand the heap/stack
segment to fill up the entirety of its allocation.  It's probably worth
dropping that, though.

NOMMU mmap() is using kobjsize()/ksize() to keep track of the number of bytes
allocated and the amount of dead space.  We can probably ditch that too.

However, fs/proc/task_nommu.c uses kobjsize() quite a bit to determine how
much metadata space a process is carrying around.  We could just use sizeof(),
I suppose, and not bother calculating the slack.

David

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Pekka J Enberg]

On Thu, 29 May 2008, David Howells wrote:
> ELF-FDPIC is currently using kobjsize() so that it can expand the heap/stack
> segment to fill up the entirety of its allocation.  It's probably worth
> dropping that, though.
> 
> NOMMU mmap() is using kobjsize()/ksize() to keep track of the number of bytes
> allocated and the amount of dead space.  We can probably ditch that too.
> 
> However, fs/proc/task_nommu.c uses kobjsize() quite a bit to determine how
> much metadata space a process is carrying around.  We could just use sizeof(),
> I suppose, and not bother calculating the slack.

For short term, we should merge my two nommu patches as-is, no?

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Paul Mundt]

On Thu, May 29, 2008 at 04:21:05PM +0300, Pekka J Enberg wrote:
> On Thu, 29 May 2008, David Howells wrote:
> > ELF-FDPIC is currently using kobjsize() so that it can expand the heap/stack
> > segment to fill up the entirety of its allocation.  It's probably worth
> > dropping that, though.
> > 
> > NOMMU mmap() is using kobjsize()/ksize() to keep track of the number of bytes
> > allocated and the amount of dead space.  We can probably ditch that too.
> > 
> > However, fs/proc/task_nommu.c uses kobjsize() quite a bit to determine how
> > much metadata space a process is carrying around.  We could just use sizeof(),
> > I suppose, and not bother calculating the slack.
> 
> For short term, we should merge my two nommu patches as-is, no?

Not until the page->index bits are killed, otherwise you aren't fixing
anything. SLOB on nommu with those page->index tests will automatically
oops today, before or after your patches. Until that's resolved, there's
no point in pretending like kobjsize() has been "fixed". As no one has
come up with a valid reason for those tests existing in the first place,
simply having your patches and killing the BUG_ON()'s seems ok.

If we're not going to kill the BUG_ON()'s, then your patches are purely
cosmetic fixups with no behavioural change -- (ie, nommu is still hosed
on SLOB with current git).

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Pekka Enberg]

Hi Paul,

Paul Mundt wrote:
> Not until the page->index bits are killed, otherwise you aren't fixing
> anything. SLOB on nommu with those page->index tests will automatically
> oops today, before or after your patches. Until that's resolved, there's
> no point in pretending like kobjsize() has been "fixed". As no one has
> come up with a valid reason for those tests existing in the first place,
> simply having your patches and killing the BUG_ON()'s seems ok.

Sorry if I'm starting to sound like a broken record, but can you explain 
why removing the ->index bits are safe? I mean, if removing them is 
really okay, that means we don't hit that code path with SLAB at all?

Paul Mundt wrote:
> If we're not going to kill the BUG_ON()'s, then your patches are purely
> cosmetic fixups with no behavioural change -- (ie, nommu is still hosed
> on SLOB with current git).

It fixes nommu with SLUB, doesn't it?

		Pekka

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Pekka J Enberg]

On Sun, 1 Jun 2008, Pekka Enberg wrote:
> > Not until the page->index bits are killed, otherwise you aren't fixing
> > anything. SLOB on nommu with those page->index tests will automatically
> > oops today, before or after your patches. Until that's resolved, there's
> > no point in pretending like kobjsize() has been "fixed". As no one has
> > come up with a valid reason for those tests existing in the first place,
> > simply having your patches and killing the BUG_ON()'s seems ok.
> 
> Sorry if I'm starting to sound like a broken record, but can you explain why
> removing the ->index bits are safe? I mean, if removing them is really okay,
> that means we don't hit that code path with SLAB at all?

Paul, so with something like this, the WARN_ON never triggers?

diff --git a/mm/nommu.c b/mm/nommu.c
index dca93fc..38eec2e 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -109,16 +109,23 @@ unsigned int kobjsize(const void *objp)
 	 * If the object we have should not have ksize performed on it,
 	 * return size of 0
 	 */
-	if (!objp || (unsigned long)objp >= memory_end || !((page = virt_to_page(objp))))
+	if (!objp)
+		return 0;
+
+	if ((unsigned long)objp >= memory_end)
+		return 0;
+
+	page = virt_to_page(objp);
+	if (!page)
 		return 0;
 
 	if (PageSlab(page))
 		return ksize(objp);
 
-	BUG_ON(page->index < 0);
-	BUG_ON(page->index >= MAX_ORDER);
+	if (WARN_ON(!PageCompound(page)))
+		return 0;
 
-	return (PAGE_SIZE << page->index);
+	return PAGE_SIZE << compound_order(page);
 }
 
 /*

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Paul Mundt]

On Sun, Jun 01, 2008 at 11:22:04AM +0300, Pekka J Enberg wrote:
> On Sun, 1 Jun 2008, Pekka Enberg wrote:
> > > Not until the page->index bits are killed, otherwise you aren't fixing
> > > anything. SLOB on nommu with those page->index tests will automatically
> > > oops today, before or after your patches. Until that's resolved, there's
> > > no point in pretending like kobjsize() has been "fixed". As no one has
> > > come up with a valid reason for those tests existing in the first place,
> > > simply having your patches and killing the BUG_ON()'s seems ok.
> > 
> > Sorry if I'm starting to sound like a broken record, but can you explain why
> > removing the ->index bits are safe? I mean, if removing them is really okay,
> > that means we don't hit that code path with SLAB at all?
> 
> Paul, so with something like this, the WARN_ON never triggers?
> 
> diff --git a/mm/nommu.c b/mm/nommu.c
> index dca93fc..38eec2e 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -109,16 +109,23 @@ unsigned int kobjsize(const void *objp)
>  	 * If the object we have should not have ksize performed on it,
>  	 * return size of 0
>  	 */
> -	if (!objp || (unsigned long)objp >= memory_end || !((page = virt_to_page(objp))))
> +	if (!objp)
> +		return 0;
> +
> +	if ((unsigned long)objp >= memory_end)
> +		return 0;
> +
> +	page = virt_to_page(objp);
> +	if (!page)
>  		return 0;
>  
This still needs to be virt_to_head_page() I think.

I don't have my nommu boards at home, so I'll test at the office tomorow
morning and let you know.

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Pekka J Enberg]

On Sun, 1 Jun 2008, Paul Mundt wrote:
> This still needs to be virt_to_head_page() I think.
> 
> I don't have my nommu boards at home, so I'll test at the office tomorow
> morning and let you know.

Yes, I messed that up. Thanks a lot for your help, Paul!

		Pekka

[PATCH] nommu: kobjsize fix
From: Christoph Lameter <clameter@sgi.com>

The kobjsize() function is broken with SLOB and SLUB. As summarized by Paul
Mundt:

  The page->index bits look like they are being used for determining compound
  order, which is _completely_ bogus, and only happens to "work" in a few
  cases.  Christoph and I have repeatedly asked for someone to explain what the
  hell those tests are there for, as right now they not only look completely
  bogus, but they also stop us from booting on SLOB.  So far no one has
  provided any input on why those page->index BUG_ON()'s have any right to
  exist.

  So while having 2 out of 3 SLAB allocators in a bootable state might seem
  like progress, I'd rather see kobjsize() fixed correctly. Even my initial
  patches worked for all 3.

  If no one can speak up to defend those bits, they should be killed off before
  2.6.26. Whether this is done in combination with your patch or Christoph's
  patch or whatever else doesn't matter.

You can find the discussion here:

  http://lkml.org/lkml/2008/5/22/223

Reported-by: Paul Mundt <lethal@linux-sh.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Matt Mackall <mpm@selenic.com>
Signed-off-by: Christoph Lameter <clameter@sgi.com>
Signed-off-by: Pekka Enberg <penberg@cs.helsinki.fi>
---

diff --git a/mm/nommu.c b/mm/nommu.c
index dca93fc..935887b 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -109,16 +109,23 @@ unsigned int kobjsize(const void *objp)
 	 * If the object we have should not have ksize performed on it,
 	 * return size of 0
 	 */
-	if (!objp || (unsigned long)objp >= memory_end || !((page = virt_to_page(objp))))
+	if (!objp)
+		return 0;
+
+	if ((unsigned long)objp >= memory_end)
+		return 0;
+
+	page = virt_to_head_page(objp);
+	if (!page)
 		return 0;
 
 	if (PageSlab(page))
 		return ksize(objp);
 
-	BUG_ON(page->index < 0);
-	BUG_ON(page->index >= MAX_ORDER);
+	if (WARN_ON(!PageCompound(page)))
+		return 0;
 
-	return (PAGE_SIZE << page->index);
+	return PAGE_SIZE << compound_order(page);
 }
 
 /*

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Pekka J Enberg]

Hi Paul,

On Sun, 1 Jun 2008, Paul Mundt wrote:
> This still needs to be virt_to_head_page() I think.
> 
> I don't have my nommu boards at home, so I'll test at the office tomorow
> morning and let you know.

I was about to send a patch that fixes some of the kobjsize() abuses to 
use ksize() but then I realized that what we probably should do is 
something like this instead.

I mean, assuming the BUG_ON bits are bogus, then we should always pass the 
pointer to the allocator. I audited most of the callers and they all seem 
to be really just using kmalloc() for allocation anyway.

What do you think?

		Pekka

diff --git a/mm/nommu.c b/mm/nommu.c
index dca93fc..fc1ff52 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -103,22 +103,20 @@ EXPORT_SYMBOL(vmtruncate);
  */
 unsigned int kobjsize(const void *objp)
 {
-	struct page *page;
-
 	/*
 	 * If the object we have should not have ksize performed on it,
 	 * return size of 0
 	 */
-	if (!objp || (unsigned long)objp >= memory_end || !((page = virt_to_page(objp))))
+	if (!objp)
 		return 0;
 
-	if (PageSlab(page))
-		return ksize(objp);
+	if ((unsigned long)objp >= memory_end)
+		return 0;
 
-	BUG_ON(page->index < 0);
-	BUG_ON(page->index >= MAX_ORDER);
+	if (!virt_to_page(objp))
+		return 0;
 
-	return (PAGE_SIZE << page->index);
+	return ksize(objp);
 }
 
 /*

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Paul Mundt]

On Sun, Jun 01, 2008 at 12:13:02PM +0300, Pekka J Enberg wrote:
> Hi Paul,
> 
> On Sun, 1 Jun 2008, Paul Mundt wrote:
> > This still needs to be virt_to_head_page() I think.
> > 
> > I don't have my nommu boards at home, so I'll test at the office tomorow
> > morning and let you know.
> 
> I was about to send a patch that fixes some of the kobjsize() abuses to 
> use ksize() but then I realized that what we probably should do is 
> something like this instead.
> 
> I mean, assuming the BUG_ON bits are bogus, then we should always pass the 
> pointer to the allocator. I audited most of the callers and they all seem 
> to be really just using kmalloc() for allocation anyway.
> 
> What do you think?
> 
Isn't this what my original patch did? ;-)

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Pekka Enberg]

On Sun, Jun 01, 2008 at 12:13:02PM +0300, Pekka J Enberg wrote:
>> I mean, assuming the BUG_ON bits are bogus, then we should always pass the 
>> pointer to the allocator. I audited most of the callers and they all seem 
>> to be really just using kmalloc() for allocation anyway.
>>
>> What do you think?

Paul Mundt wrote:
> Isn't this what my original patch did? ;-)

Oh, almost, you had this bit in ksize() of SLAB:

+	page = virt_to_head_page(objp);
+	if (unlikely(!PageSlab(page)))
+		return PAGE_SIZE << compound_order(page);

Did you actually need it for something?

		Pekka

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Paul Mundt]

On Sun, Jun 01, 2008 at 01:29:39PM +0300, Pekka Enberg wrote:
> On Sun, Jun 01, 2008 at 12:13:02PM +0300, Pekka J Enberg wrote:
> >>I mean, assuming the BUG_ON bits are bogus, then we should always pass 
> >>the pointer to the allocator. I audited most of the callers and they all 
> >>seem to be really just using kmalloc() for allocation anyway.
> >>
> >>What do you think?
> 
> Paul Mundt wrote:
> >Isn't this what my original patch did? ;-)
> 
> Oh, almost, you had this bit in ksize() of SLAB:
> 
> +	page = virt_to_head_page(objp);
> +	if (unlikely(!PageSlab(page)))
> +		return PAGE_SIZE << compound_order(page);
> 
> Did you actually need it for something?
> 
Not that I recall, it was just for consistency with SLUB. I'll have to
re-test though, as I'm not sure if it was necessary or not.

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Pekka J Enberg]

On Sun, Jun 01, 2008 at 01:29:39PM +0300, Pekka Enberg wrote:
> > Oh, almost, you had this bit in ksize() of SLAB:
> > 
> > +	page = virt_to_head_page(objp);
> > +	if (unlikely(!PageSlab(page)))
> > +		return PAGE_SIZE << compound_order(page);
> > 
> > Did you actually need it for something?
 
On Sun, 1 Jun 2008, Paul Mundt wrote:
> Not that I recall, it was just for consistency with SLUB. I'll have to
> re-test though, as I'm not sure if it was necessary or not.

OK. If we do need it, then something like this should work. But I don't 
see how we could have these "arbitrary pointers" (meaning not returned 
by kmalloc()) to compound pages; otherwise the BUG_ON would trigger with 
SLAB as well. I also don't see any call-sites that do this (but I'm not an 
expert on nommu).

		Pekka

diff --git a/mm/nommu.c b/mm/nommu.c
index dca93fc..604e7de 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -109,16 +109,39 @@ unsigned int kobjsize(const void *objp)
 	 * If the object we have should not have ksize performed on it,
 	 * return size of 0
 	 */
-	if (!objp || (unsigned long)objp >= memory_end || !((page = virt_to_page(objp))))
+	if (!objp)
 		return 0;
 
+	if ((unsigned long)objp >= memory_end)
+		return 0;
+
+	page = virt_to_head_page(objp);
+	if (!page)
+		return 0;
+
+	/*
+	 * If the allocator sets PageSlab, we know the pointer came from
+	 * kmalloc(). The allocator, however, is not guaranteed to set PageSlab
+	 * so the pointer might still have come from kmalloc() (see the comments
+	 * below).
+	 */
 	if (PageSlab(page))
 		return ksize(objp);
 
-	BUG_ON(page->index < 0);
-	BUG_ON(page->index >= MAX_ORDER);
+	/*
+	 * The ksize() function is only guaranteed to work for pointers
+	 * returned by kmalloc(). So handle arbitrary pointers, that we expect
+	 * always to be compound pages, here. Note: we also handle pointers to
+	 * compound pages that came from kmalloc() here.
+	 */
+	if (PageCompound(page))
+		return PAGE_SIZE << compound_order(page);
 
-	return (PAGE_SIZE << page->index);
+	/*
+	 * Finally, handle pointers that came from kmalloc() that don't have
+	 * PageSlab set.
+	 */
+	return ksize(objp);
 }
 
 /*

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Paul Mundt]

On Sun, Jun 01, 2008 at 02:30:28PM +0300, Pekka J Enberg wrote:
> On Sun, Jun 01, 2008 at 01:29:39PM +0300, Pekka Enberg wrote:
> > > Oh, almost, you had this bit in ksize() of SLAB:
> > > 
> > > +	page = virt_to_head_page(objp);
> > > +	if (unlikely(!PageSlab(page)))
> > > +		return PAGE_SIZE << compound_order(page);
> > > 
> > > Did you actually need it for something?
>  
> On Sun, 1 Jun 2008, Paul Mundt wrote:
> > Not that I recall, it was just for consistency with SLUB. I'll have to
> > re-test though, as I'm not sure if it was necessary or not.
> 
> OK. If we do need it, then something like this should work. But I don't 
> see how we could have these "arbitrary pointers" (meaning not returned 
> by kmalloc()) to compound pages; otherwise the BUG_ON would trigger with 
> SLAB as well. I also don't see any call-sites that do this (but I'm not an 
> expert on nommu).
> 
In the kmem_cache_alloc() case calling ksize() there is bogus, the
previous semantics for kobjsize() just defaulted to returning PAGE_SIZE
for these, since page->index was typically 0. Presently if we ksize()
those objects, we get bogus size results that are smaller than the
minimum alignment size. So we still need a way to handle that, even if
it's not frightfully accurate.

If we go back and apply your PG_slab patch for SLUB + SLOB, then
kobjsize() can just become:

diff --git a/mm/nommu.c b/mm/nommu.c
index dca93fc..3abd084 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -104,21 +104,43 @@ EXPORT_SYMBOL(vmtruncate);
 unsigned int kobjsize(const void *objp)
 {
 	struct page *page;
+	int order = 0;
 
 	/*
 	 * If the object we have should not have ksize performed on it,
 	 * return size of 0
 	 */
-	if (!objp || (unsigned long)objp >= memory_end || !((page = virt_to_page(objp))))
+	if (!objp)
 		return 0;
 
+	if ((unsigned long)objp >= memory_end)
+		return 0;
+
+	page = virt_to_head_page(objp);
+	if (!page)
+		return 0;
+
+	/*
+	 * If the allocator sets PageSlab, we know the pointer came from
+	 * kmalloc().
+	 */
 	if (PageSlab(page))
 		return ksize(objp);
 
-	BUG_ON(page->index < 0);
-	BUG_ON(page->index >= MAX_ORDER);
+	/*
+	 * The ksize() function is only guaranteed to work for pointers
+	 * returned by kmalloc(). So handle arbitrary pointers, that we expect
+	 * always to be compound pages, here.
+	 */
+	if (PageCompound(page))
+		order = compound_order(page);
 
-	return (PAGE_SIZE << page->index);
+	/*
+	 * Finally, handle arbitrary pointers that don't set PageSlab.
+	 * Default to 0-order in the case when we're unable to ksize()
+	 * the object.
+	 */
+	return PAGE_SIZE << order;
 }
 
 /*

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Pekka Enberg]

Hi Paul,

On Mon, Jun 2, 2008 at 8:59 AM, Paul Mundt <lethal@linux-sh.org> wrote:
> In the kmem_cache_alloc() case calling ksize() there is bogus, the
> previous semantics for kobjsize() just defaulted to returning PAGE_SIZE
> for these, since page->index was typically 0. Presently if we ksize()
> those objects, we get bogus size results that are smaller than the
> minimum alignment size. So we still need a way to handle that, even if
> it's not frightfully accurate.
>
> If we go back and apply your PG_slab patch for SLUB + SLOB, then
> kobjsize() can just become:

What call-sites are using kmem_cache_alloc()? Can we convert them to
kmalloc() or page_alloc()? IIRC both Christoph and Matt opposed my
PG_slab patches.

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Paul Mundt]

On Mon, Jun 02, 2008 at 09:32:48AM +0300, Pekka Enberg wrote:
> On Mon, Jun 2, 2008 at 8:59 AM, Paul Mundt <lethal@linux-sh.org> wrote:
> > In the kmem_cache_alloc() case calling ksize() there is bogus, the
> > previous semantics for kobjsize() just defaulted to returning PAGE_SIZE
> > for these, since page->index was typically 0. Presently if we ksize()
> > those objects, we get bogus size results that are smaller than the
> > minimum alignment size. So we still need a way to handle that, even if
> > it's not frightfully accurate.
> >
> > If we go back and apply your PG_slab patch for SLUB + SLOB, then
> > kobjsize() can just become:
> 
> What call-sites are using kmem_cache_alloc()? Can we convert them to
> kmalloc() or page_alloc()? IIRC both Christoph and Matt opposed my
> PG_slab patches.

Well, with that modified version of your patch I posted, even if your
previous PG_slab patches aren't applied, kobjsize() doesn't behave any
worse than it presently does in terms of object size accuracy.

In short: PG_slab doesn't get set and ksize() is never called, so we get
the same degree of accuracy as the existing implementation, and the
oopses get fixed (and the comments are still accurate, too!). So I think
it's worth applying. Verified on all of SLUB/SLOB/SLAB.

As for the call-site question, perhaps I'm misunderstanding your
question. alloc_vfsmnt() is the first to call kmem_cache_zalloc() during
boot-up on my system, but I'm not sure what relevance this has to
anything? Accurately measuring kmem_cache_alloc() and static allocations
is going to need quite a bit more of a re-think, but that's out of scope
for 2.6.26. Presently I'd rather have my system booting first :-)

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Pekka Enberg]

Hi Paul,

On Mon, Jun 2, 2008 at 9:50 AM, Paul Mundt <lethal@linux-sh.org> wrote:
> Well, with that modified version of your patch I posted, even if your
> previous PG_slab patches aren't applied, kobjsize() doesn't behave any
> worse than it presently does in terms of object size accuracy.
>
> In short: PG_slab doesn't get set and ksize() is never called, so we get
> the same degree of accuracy as the existing implementation, and the
> oopses get fixed (and the comments are still accurate, too!). So I think
> it's worth applying. Verified on all of SLUB/SLOB/SLAB.

Agreed. Can you send this to Andrew?

Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>

On Mon, Jun 2, 2008 at 9:50 AM, Paul Mundt <lethal@linux-sh.org> wrote:
> As for the call-site question, perhaps I'm misunderstanding your
> question. alloc_vfsmnt() is the first to call kmem_cache_zalloc() during
> boot-up on my system, but I'm not sure what relevance this has to
> anything? Accurately measuring kmem_cache_alloc() and static allocations
> is going to need quite a bit more of a re-think, but that's out of scope
> for 2.6.26. Presently I'd rather have my system booting first :-)

David already mentioned some (most?) of the kobjsize() calls can go
away and I think we should pursue that for 2.6.27.

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Paul Mundt]

On Mon, Jun 02, 2008 at 09:58:01AM +0300, Pekka Enberg wrote:
> Hi Paul,
> 
> On Mon, Jun 2, 2008 at 9:50 AM, Paul Mundt <lethal@linux-sh.org> wrote:
> > Well, with that modified version of your patch I posted, even if your
> > previous PG_slab patches aren't applied, kobjsize() doesn't behave any
> > worse than it presently does in terms of object size accuracy.
> >
> > In short: PG_slab doesn't get set and ksize() is never called, so we get
> > the same degree of accuracy as the existing implementation, and the
> > oopses get fixed (and the comments are still accurate, too!). So I think
> > it's worth applying. Verified on all of SLUB/SLOB/SLAB.
> 
> Agreed. Can you send this to Andrew?
> 
> Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>
> 
Will do!

> On Mon, Jun 2, 2008 at 9:50 AM, Paul Mundt <lethal@linux-sh.org> wrote:
> > As for the call-site question, perhaps I'm misunderstanding your
> > question. alloc_vfsmnt() is the first to call kmem_cache_zalloc() during
> > boot-up on my system, but I'm not sure what relevance this has to
> > anything? Accurately measuring kmem_cache_alloc() and static allocations
> > is going to need quite a bit more of a re-think, but that's out of scope
> > for 2.6.26. Presently I'd rather have my system booting first :-)
> 
> David already mentioned some (most?) of the kobjsize() calls can go
> away and I think we should pursue that for 2.6.27.

Ah, the kobjsize() calls is what you were getting at. Yes, getting rid of
those would be a good plan. I'll start looking at that more in-depth once
I've got some of my other 2.6.27 stuff out of the way. That's definitely
one interface that needs to be killed off with extreme prejudice..

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Paul Mundt]

On Sun, Jun 01, 2008 at 10:58:34AM +0300, Pekka Enberg wrote:
> Paul Mundt wrote:
> >Not until the page->index bits are killed, otherwise you aren't fixing
> >anything. SLOB on nommu with those page->index tests will automatically
> >oops today, before or after your patches. Until that's resolved, there's
> >no point in pretending like kobjsize() has been "fixed". As no one has
> >come up with a valid reason for those tests existing in the first place,
> >simply having your patches and killing the BUG_ON()'s seems ok.
> 
> Sorry if I'm starting to sound like a broken record, but can you explain 
> why removing the ->index bits are safe? I mean, if removing them is 
> really okay, that means we don't hit that code path with SLAB at all?
> 
I thought I already had several times, but I'll attempt to summarize again..

I would expect the only reason those BUG_ON()'s haven't been triggered in
the non-SLOB case is due to the fact PageSlab() takes a different path
out.

The page->index bits look like they are being used for determining
compound order, which is _completely_ bogus, and only happens to "work"
in a few cases. Christoph and I have repeatedly asked for someone to
explain what the hell those tests are there for, as right now they not
only look completely bogus, but they also stop us from booting on SLOB.
So far no one has provided any input on why those page->index BUG_ON()'s
have any right to exist.

So while having 2 out of 3 SLAB allocators in a bootable state might seem
like progress, I'd rather see kobjsize() fixed correctly. Even my initial
patches worked for all 3.

If no one can speak up to defend those bits, they should be killed off
before 2.6.26. Whether this is done in combination with your patch or
Christoph's patch or whatever else doesn't matter.

Re: [PATCH] nommu: fix kobjsize() for SLOB and SLUB

[Christoph Lameter]

On Thu, 29 May 2008, David Howells wrote:

> > Could the nommu people either tell us what this is all about? Or junk the 
> > code. This looks very wrong.
> 
> ELF-FDPIC is currently using kobjsize() so that it can expand the heap/stack
> segment to fill up the entirety of its allocation.  It's probably worth
> dropping that, though.
> 
> NOMMU mmap() is using kobjsize()/ksize() to keep track of the number of bytes
> allocated and the amount of dead space.  We can probably ditch that too.
> 
> However, fs/proc/task_nommu.c uses kobjsize() quite a bit to determine how
> much metadata space a process is carrying around.  We could just use sizeof(),
> I suppose, and not bother calculating the slack.

The nagging question here is: Why is page->private used in kobjsize()?  A 
special heap/stack setup?