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

Previous thread: [PATCH] ftrace: fix up cmdline recording by Steven Rostedt on Thursday, May 22, 2008 - 8:49 am. (2 messages)

Next thread: [RFC/PATCH] nommu: fix ksize() abuse by Pekka J Enberg on Thursday, May 22, 2008 - 9:11 am. (5 messages)
From: Pekka J Enberg
Date: Thursday, May 22, 2008 - 9:09 am

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

From: Christoph Lameter
Date: Thursday, May 22, 2008 - 9:48 am

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

--

From: Paul Mundt
Date: Thursday, May 22, 2008 - 4:40 pm

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

From: Christoph Lameter
Date: Thursday, May 22, 2008 - 4:45 pm

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


--

From: Paul Mundt
Date: Thursday, May 22, 2008 - 4:50 pm

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

From: Christoph Lameter
Date: Thursday, May 22, 2008 - 5:04 pm

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

From: David Howells
Date: Wednesday, May 28, 2008 - 6:12 am

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

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

From: Pekka J Enberg
Date: Wednesday, May 28, 2008 - 6:17 am

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

From: David Howells
Date: Wednesday, May 28, 2008 - 6:40 am

Andrew and/or Linus.

David
--

From: David Howells
Date: Wednesday, May 28, 2008 - 7:09 am

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

From: Christoph Lameter
Date: Wednesday, May 28, 2008 - 10:26 am

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


--

From: David Howells
Date: Wednesday, May 28, 2008 - 10:38 am

I fixed it by setting ARCH_KMALLOC_MINALIGN and ARCH_SLAB_MINALIGN.

David
--

From: Mike Frysinger
Date: Wednesday, May 28, 2008 - 1:35 pm

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=50...
-mike
--

From: David Howells
Date: Thursday, May 29, 2008 - 6:03 am

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

From: Mike Frysinger
Date: Thursday, May 29, 2008 - 1:25 pm

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

From: Christoph Lameter
Date: Thursday, May 29, 2008 - 1:30 pm

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.


--

From: Mike Frysinger
Date: Thursday, May 29, 2008 - 1:51 pm

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

From: Christoph Lameter
Date: Thursday, May 29, 2008 - 2:29 pm

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

From: Mike Frysinger
Date: Thursday, May 29, 2008 - 9:18 pm

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

From: David Howells
Date: Wednesday, May 28, 2008 - 7:27 am

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, ...
Previous thread: [PATCH] ftrace: fix up cmdline recording by Steven Rostedt on Thursday, May 22, 2008 - 8:49 am. (2 messages)

Next thread: [RFC/PATCH] nommu: fix ksize() abuse by Pekka J Enberg on Thursday, May 22, 2008 - 9:11 am. (5 messages)