nommu: Push kobjsize() slab-specific logic down to ksize().

Moving the existing logic in to SLAB's ksize() and simply wrapping in to ksize() directly seems to do the right thing in all cases, and allows me to boot with any of the slab allocators enabled, rather than simply SLAB by itself.

I've done the same !PageSlab() test in SLAB as SLUB does in its ksize(), which also seems to produce the correct results. Hopefully someone more familiar with the history of kobjsize()/ksize() interaction can scream if this is the wrong thing to do. :-)

Quoted 5 lines · Paul Mundt

Moving the existing logic in to SLAB's ksize() and simply wrapping in to ksize() directly seems to do the right thing in all cases, and allows me to boot with any of the slab allocators enabled, rather than simply SLAB by itself.

Quoted 4 lines

I've done the same !PageSlab() test in SLAB as SLUB does in its ksize(), which also seems to produce the correct results. Hopefully someone more familiar with the history of kobjsize()/ksize() interaction can scream if this is the wrong thing to do. :-)

That seems reasonable. I can't test it until I get back to the UK next week.

Quoted 12 lines · David Howells

Quoted 9 lines · Paul Mundt

Moving the existing logic in to SLAB's ksize() and simply wrapping in to ksize() directly seems to do the right thing in all cases, and allows me to boot with any of the slab allocators enabled, rather than simply SLAB by itself.

I've done the same !PageSlab() test in SLAB as SLUB does in its ksize(), which also seems to produce the correct results. Hopefully someone more familiar with the history of kobjsize()/ksize() interaction can scream if this is the wrong thing to do. :-)

Quoted 1 line

That seems reasonable. I can't test it until I get back to the UK next week.

Hmm. That means we are sanctioning using ksize on arbitrary objects? SLUB supports that but SLAB wont and neither will SLOB. I think we need to stay with the strict definition that is needed by SLOB.

It seems also that the existing kobjsize function is wrong:

1. For compound pages the head page needs to be determined.

So do a virt_to_head_page() instead of a virt_to_page().

2. Why is page->index take as the page order?

Use compound_order instead?

I think the following patch will work for all allocators (can virt_to_page() really return NULL if the addr is invalid if so we may have to fix virt_to_head_page()?):

---
 mm/nommu.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Index: linux-2.6/mm/nommu.c
===================================================================

Quoted 3 lines

Hmm. That means we are sanctioning using ksize on arbitrary objects? SLUB supports that but SLAB wont and neither will SLOB. I think we need to stay with the strict definition that is needed by SLOB.

Of course SLUB won't be able to tell you the size of objects allocated statically, through bootmem, etc.

Quoted 6 lines · Matt Mackall

Quoted 3 lines

Hmm. That means we are sanctioning using ksize on arbitrary objects? SLUB supports that but SLAB wont and neither will SLOB. I think we need to stay with the strict definition that is needed by SLOB.

Of course SLUB won't be able to tell you the size of objects allocated statically, through bootmem, etc.

Right the function actually give misleading results even if you just pass a pointer to an int at an address that is page backed but not using a slab allocator. Then PAGE_SIZE will be returned?

So the semantics are screwed up here. kobjsize() should only be called for slab objects.

Remove kobjsize completely and replace with calls to ksize? Callers must not call ksize() on non slab objects.

Quoted 2 lines

Remove kobjsize completely and replace with calls to ksize? Callers must not call ksize() on non slab objects.

What'd you think of my idea of adding WARN_ONs to SLAB and SLUB for these cases? That is, warn whenever ksize() gets a non-kmalloced address?

Quoted 3 lines · Matt Mackall

Quoted 2 lines

Remove kobjsize completely and replace with calls to ksize? Callers must not call ksize() on non slab objects.

Quoted 3 lines

What'd you think of my idea of adding WARN_ONs to SLAB and SLUB for these cases? That is, warn whenever ksize() gets a non-kmalloced address?

How would that work given that both SLUB and SLOB forward >4k allocs to the page allocator? So any compound page allocation may be a slab allocation. Is there some way to distinguish between a allocations of the page allocator and a slab alloc?

Quoted 4 lines

How would that work given that both SLUB and SLOB forward >4k allocs to the page allocator? So any compound page allocation may be a slab allocation. Is there some way to distinguish between a allocations of the page allocator and a slab alloc?

We can't do it at all for SLOB. But when debugging is turned on, we can notice (in SLAB and SLUB) whenever anyone asks for the ksize() of something that lives on a non-kmalloc slab.

Quoted 5 lines · Matt Mackall

Quoted 4 lines

How would that work given that both SLUB and SLOB forward >4k allocs to the page allocator? So any compound page allocation may be a slab allocation. Is there some way to distinguish between a allocations of the page allocator and a slab alloc?

Quoted 3 lines

We can't do it at all for SLOB. But when debugging is turned on, we can notice (in SLAB and SLUB) whenever anyone asks for the ksize() of something that lives on a non-kmalloc slab.

We could mark the pages specially I guess. Add a slab flag for kmalloc? PageSlab and PageKmalloc?

Quoted 6 lines · Christoph Lameter

Quoted 5 lines · Matt Mackall

Quoted 4 lines

How would that work given that both SLUB and SLOB forward >4k allocs to the page allocator? So any compound page allocation may be a slab allocation. Is there some way to distinguish between a allocations of the page allocator and a slab alloc?

Quoted 6 lines

Quoted 3 lines

We can't do it at all for SLOB. But when debugging is turned on, we can notice (in SLAB and SLUB) whenever anyone asks for the ksize() of something that lives on a non-kmalloc slab.

We could mark the pages specially I guess. Add a slab flag for kmalloc? PageSlab and PageKmalloc?

No, just warn for the cases where we already have enough information.

Having WARN_ON()'s for !PageSlab() pages in ksize() in SLAB/SLUB would make these cases more visible, at least.

 mm/nommu.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

Index: linux-2.6/mm/nommu.c
===================================================================

mm/nommu.c +2 -1

@@ -109,16 +109,14 @@ 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 || (unsigned long)objp >= memory_end ||
+				!((page = virt_to_head_page(objp))))

Currently SLUB and SLOB both blow up in different ways on shnommu:

SLOB:

Freeing unused kernel memory: 620k freed

------------[ cut here ]------------
kernel BUG at mm/nommu.c:119!
Kernel BUG: 003e [#1]
Modules linked in:

Pid : 1, Comm: init
PC is at kobjsize+0x5c/0x8c
PC : 0c04000c SP : 0c20dd08 SR : 40000001 Not tainted
R0 : 00000000 R1 : 0000000a R2 : 001b8334 R3 : 0c180000 R4 : 00000840 R5 : 0c3a507c R6 : 000110e2 R7 : 00000020 R8 : 0c1e5334 R9 : 0c3a5210 R10 : 0c3a5210 R11 : 0c180000 R12 : 0c3a5250 R13 : 00000000 R14 : 0c20dd08
MACH: 00000221 MACL: 001b8334 GBR : 00000000 PR : 0c03fff4

Call trace:
[<0c0409ca>] do_mmap_pgoff+0x5ea/0x7f0
[<0c07046a>] load_flat_file+0x326/0x77c
[<0c07090a>] load_flat_binary+0x4a/0x2c8

...

SLUB:

Freeing unused kernel memory: 624k freed Unable to allocate RAM for process text/data, errno 12 Failed to execute /init
Unable to allocate RAM for process text/data, errno 12 Unable to allocate RAM for process text/data, errno 12

Kernel panic - not syncing: No init found.  Try passing init= option to kernel.

...

In both cases this is due to kobjsize() failures. By checking PageSlab() before calling in to ksize(), SLAB manages to work ok, as it aggressively sets these bits across compounds.

In situations where we are setting up private mappings or shared anonymous mappings (via do_mmap_private()) and the vm_file mmap() fop hands back an -ENOSYS, a new allocation is made and the file contents copied over. SLOB's page->index at the time of hitting the above BUG_ON() is the value of the non-page-aligned address returned by kmalloc() + the read size. page->index itself is otherwise sane when the object to be copied is larger than a page in size and aligned.

Both SLOB and SLUB presently have methods to determine whether an object passed in to their respective ksize() implementations are their own slab pages, or part of a compound page (heavily used by nommu for anonymous mappings), leaving only SLAB as the odd one out. The rationale here seems to be that SLAB's ksize() will trigger a BUG_ON() if !PageSlab(), and so the page->index shift is defaulted to instead for non-slab pages.

Moving the existing logic in to SLAB's ksize() and simply wrapping in to ksize() directly seems to do the right thing in all cases, and allows me to boot with any of the slab allocators enabled, rather than simply SLAB by itself.

I've done the same !PageSlab() test in SLAB as SLUB does in its ksize(), which also seems to produce the correct results. Hopefully someone more familiar with the history of kobjsize()/ksize() interaction can scream if this is the wrong thing to do. :-)

Signed-off-by Paul Mundt <[email protected]>

---

 mm/nommu.c |    8 +-------
 mm/slab.c  |    6 ++++++
 2 files changed, 7 insertions(+), 7 deletions(-)

mm/nommu.c +1 -7

index ef8c62c..3e11814 100644
@@ -112,13 +112,7 @@ unsigned int kobjsize(const void *objp)
 	if (!objp || (unsigned long)objp >= memory_end || !((page = virt_to_page(objp))))
 		return 0;
 
-	if (PageSlab(page))
-		return ksize(objp);
-
-	BUG_ON(page->index < 0);
-	BUG_ON(page->index >= MAX_ORDER);
-
-	return (PAGE_SIZE << page->index);
+	return ksize(objp);
 }
 
 /*

mm/slab.c +6 -0

index 06236e4..7a012bb 100644
@@ -4472,10 +4472,16 @@ const struct seq_operations slabstats_op = {
  */
 size_t ksize(const void *objp)
 {
+	struct page *page;
+
 	BUG_ON(!objp);
 	if (unlikely(objp == ZERO_SIZE_PTR))
 		return 0;
 
+	page = virt_to_head_page(objp);
+	if (unlikely(!PageSlab(page)))
+		return PAGE_SIZE << compound_order(page);
+
 	return obj_size(virt_to_cache(objp));
 }
 EXPORT_SYMBOL(ksize);

Quoted 9 lines · Paul Mundt

Moving the existing logic in to SLAB's ksize() and simply wrapping in to ksize() directly seems to do the right thing in all cases, and allows me to boot with any of the slab allocators enabled, rather than simply SLAB by itself.

I've done the same !PageSlab() test in SLAB as SLUB does in its ksize(), which also seems to produce the correct results. Hopefully someone more familiar with the history of kobjsize()/ksize() interaction can scream if this is the wrong thing to do. :-)

My investigation of this the last time around lead me to the conclusion that the nommu code here was broken as it can call ksize on objects that were statically allocated (IIRC, the initial task struct is one such example).

It also calls ksize on objects that are kmem_cache_alloced, which is also a no-no. Unfortunately, it's a no-no that just happens to work in SLAB/SLUB by virtue of implementing kmalloc on top of kmem_cache_alloc.

With SLOB, the object size for kmem_cache_alloced objects is only available statically. Further, we can only statically distinguish between a kmalloc'ed and kmem_cache_alloc'ed object. So when you pass a kmem_cache_alloc'ed object, we'll end up reading random data outside the object to find its 'size'. So this might 'work' for SLOB in the sense of not crashing, but it won't be correct.

mm/nommu.c +1 -7

index ef8c62c..3e11814 100644
@@ -112,13 +112,7 @@ unsigned int kobjsize(const void *objp)
 	if (!objp || (unsigned long)objp >= memory_end || !((page = virt_to_page(objp))))
 		return 0;
 
-	if (PageSlab(page))
-		return ksize(objp);
-
-	BUG_ON(page->index < 0);
-	BUG_ON(page->index >= MAX_ORDER);
-
-	return (PAGE_SIZE << page->index);
+	return ksize(objp);
 }
 
 /*

mm/slab.c +2 -0

index 06236e4..7a012bb 100644
@@ -4472,10 +4472,16 @@ const struct seq_operations slabstats_op = {
  */
 size_t ksize(const void *objp)
 {
+	struct page *page;
+
 	BUG_ON(!objp);

(Not really sure if we came to a conclusion with the discussion.)

Quoted 33 lines · Paul Mundt

mm/nommu.c +1 -7

index ef8c62c..3e11814 100644
@@ -112,13 +112,7 @@ unsigned int kobjsize(const void *objp)
 	if (!objp || (unsigned long)objp >= memory_end || !((page = virt_to_page(objp))))
 		return 0;
 
-	if (PageSlab(page))
-		return ksize(objp);
-
-	BUG_ON(page->index < 0);
-	BUG_ON(page->index >= MAX_ORDER);
-
-	return (PAGE_SIZE << page->index);
+	return ksize(objp);
 }
 
 /*

mm/slab.c +2 -0

index 06236e4..7a012bb 100644
@@ -4472,10 +4472,16 @@ const struct seq_operations slabstats_op = {
  */
 size_t ksize(const void *objp)
 {
+	struct page *page;
+
 	BUG_ON(!objp);
 	if (unlikely(objp == ZERO_SIZE_PTR))
 		return 0;
 

mm/slab.c const struct seq_operations slabstats_op = { · line 4487

Quoted 7 lines

mm/slab.c +4 -0

+	page = virt_to_head_page(objp);
+	if (unlikely(!PageSlab(page)))
+		return PAGE_SIZE << compound_order(page);
+
 	return obj_size(virt_to_cache(objp));
 }
 EXPORT_SYMBOL(ksize);

The patch looks good to me. Christoph, Matt, NAK/ACK?

Moving the existing logic in to SLAB's ksize() and simply wrapping in to ksize() directly seems to do the right thing in all cases, and allows me to boot with any of the slab allocators enabled, rather than simply SLAB by itself.

I've done the same !PageSlab() test in SLAB as SLUB does in its ksize(), which also seems to produce the correct results. Hopefully someone more familiar with the history of kobjsize()/ksize() interaction can scream if this is the wrong thing to do. :-)

So I suggest we fix up kobjsize() instead. Paul, does the following untested patch work for you?

As pointed out by Christoph, it. ksize() works with SLUB and SLOB accidentally because they do page allocator pass-through and thus need to deal with non-PageSlab pages. SLAB, however, does not do that which is why all pages passed to it must have PageSlab set (we ought to add a WARN_ON() there btw).

So I suggest we fix up kobjsize() instead. Paul, does the following untested patch work for you?

mm/nommu.c +0 -0

index ef8c62c..a573aeb 100644

Hi!

Quoted 9 lines · Paul Mundt

Moving the existing logic in to SLAB's ksize() and simply wrapping in to ksize() directly seems to do the right thing in all cases, and allows me to boot with any of the slab allocators enabled, rather than simply SLAB by itself.

I've done the same !PageSlab() test in SLAB as SLUB does in its ksize(), which also seems to produce the correct results. Hopefully someone more familiar with the history of kobjsize()/ksize() interaction can scream if this is the wrong thing to do. :-)

As pointed out by Christoph, it. ksize() works with SLUB and SLOB accidentally because they do page allocator pass-through and thus need to deal with non-PageSlab pages. SLAB, however, does not do that which is why all pages passed to it must have PageSlab set (we ought to add a WARN_ON() there btw).

So I suggest we fix up kobjsize() instead. Paul, does the following untested patch work for you?

It seems to, but I wonder if compound_order() needs to take a virt_to_head_page(objp) instead of virt_to_page()?