[PATCH] nommu: Correct kobjsize() page validity checks.

[PATCH] nommu: Correct kobjsize() page validity checks.

[Paul Mundt]

This implements a few changes on top of the recent kobjsize() refactoring
introduced by commit 6cfd53fc03670c7a544a56d441eb1a6cc800d72b.

As Christoph points out:

	virt_to_head_page cannot return NULL. virt_to_page also
	does not return NULL. pfn_valid() needs to be used to
	figure out if a page is valid.  Otherwise the page struct
	reference that was returned may have PageReserved() set
	to indicate that it is not a valid page.

As discussed further in the thread, virt_addr_valid() is the preferable
way to validate the object pointer in this case. In addition to fixing
up the reserved page case, it also has the benefit of encapsulating the
hack introduced by commit 4016a1390d07f15b267eecb20e76a48fd5c524ef on
the impacted platforms, allowing us to get rid of the extra checking in
kobjsize() for the platforms that don't perform this type of bizarre
memory_end abuse (every nommu platform that isn't blackfin). If blackfin
decides to get in line with every other platform and use PageReserved
for the DMA pages in question, kobjsize() will also continue to work
fine.

It also turns out that compound_order() will give us back 0-order for
non-head pages, so we can get rid of the PageCompound check and just
use compound_order() directly. Clean that up while we're at it.

Signed-off-by: Paul Mundt <lethal@linux-sh.org>
Reviewed-by: Christoph Lameter <clameter@sgi.com>

---

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

diff --git a/mm/nommu.c b/mm/nommu.c
index 3abd084..4462b6a 100644
--- a/mm/nommu.c
+++ b/mm/nommu.c
@@ -104,21 +104,15 @@ 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)
-		return 0;
-
-	if ((unsigned long)objp >= memory_end)
+	if (!objp || !virt_addr_valid(objp))
 		return 0;
 
 	page = virt_to_head_page(objp);
-	if (!page)
-		return 0;
 
 	/*
 	 * If the allocator sets PageSlab, we know the pointer came from
@@ -129,18 +123,9 @@ unsigned int kobjsize(const void *objp)
 
 	/*
 	 * 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);
-
-	/*
-	 * Finally, handle arbitrary pointers that don't set PageSlab.
-	 * Default to 0-order in the case when we're unable to ksize()
-	 * the object.
+	 * returned by kmalloc(). So handle arbitrary pointers here.
 	 */
-	return PAGE_SIZE << order;
+	return PAGE_SIZE << compound_order(page);
 }
 
 /*

Re: [PATCH] nommu: Correct kobjsize() page validity checks.

[David Howells]

Paul Mundt <lethal@linux-sh.org> wrote:

> This implements a few changes on top of the recent kobjsize() refactoring
> introduced by commit 6cfd53fc03670c7a544a56d441eb1a6cc800d72b.
> ...
>
> Signed-off-by: Paul Mundt <lethal@linux-sh.org>
> Reviewed-by: Christoph Lameter <clameter@sgi.com>

Works for me.

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

Re: [PATCH] nommu: Correct kobjsize() page validity checks.

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> Works for me.

Having said that, it doesn't produce the right answers under SLOB, with or
without this patch:

	void task_mem(struct seq_file *m, struct mm_struct *mm)
	{
		struct vm_list_struct *vml;
		unsigned long bytes = 0, sbytes = 0, slack = 0;

		down_read(&mm->mmap_sem);
		for (vml = mm->context.vmlist; vml; vml = vml->next) {
			if (!vml->vma)
				continue;

			bytes += kobjsize(vml);

Here kobjsize() returns 16384 (PAGE_SIZE) when it should return something a
lot smaller.  This appears related to SLOB not setting PG_slab, so
/proc/pic/status gets messed up:

	Mem:      983040 bytes
	Slack:     44112 bytes
	Shared:  1687552 bytes

SLAB returns 64 at this point, and in /proc/pid/status shows:

	Mem:      594016 bytes
	Slack:     44112 bytes
	Shared:  1638688 bytes

with or without the patch, which might even be correct.


Maybe on SLOB we want kobjsize() to become sizeof() where we're dealing with
fixed size units such as structs.  On the other hand, getting rid of
kobjsize() entirely would be good.

David

Re: [PATCH] nommu: Correct kobjsize() page validity checks.

[Paul Mundt]

On Thu, Jun 12, 2008 at 11:20:53AM +0100, David Howells wrote:
> Here kobjsize() returns 16384 (PAGE_SIZE) when it should return something a
> lot smaller.  This appears related to SLOB not setting PG_slab, so
> /proc/pic/status gets messed up:
> 
> 	Mem:      983040 bytes
> 	Slack:     44112 bytes
> 	Shared:  1687552 bytes
> 
> SLAB returns 64 at this point, and in /proc/pid/status shows:
> 
> 	Mem:      594016 bytes
> 	Slack:     44112 bytes
> 	Shared:  1638688 bytes
> 
> with or without the patch, which might even be correct.
> 
Yes, this is the fundamental problem with kobjsize() as it is today.
Without the setting of PG_slab, non-compound page objects default to
PAGE_SIZE. Pekka posted a series of patches that hooked PG_slab in to
SLUB and SLOB that get the same degree of accuracy that SLAB has today,
but there was opposition to merging those.

> Maybe on SLOB we want kobjsize() to become sizeof() where we're dealing with
> fixed size units such as structs.  On the other hand, getting rid of
> kobjsize() entirely would be good.
> 
I think for the time being we have to live with the innacuracy, and
simply focus on getting rid of kobjsize() entirely. Without additional
help from the various allocators, there's really not that much we can do.
Rather than papering over that, bringing nommu more in line with MMU
semantics is certainly a preferable option.

This patch in particular is unrelated to the inaccuracy issues, although
it should fix the cases where kobjsize() would mistakenly account
reserved pages.