Skip to contentSkip to conversation

nommu: Correct kobjsize() page validity checks.

PATCH In review Paul Mundt 2 participants 4 messages Jun 12, 2008

Subsystems (2)
lore
Share & tools
lore

Paul Mundt

author
#0 lore raw

PATCH 1/1 nommu: Correct kobjsize() page validity checks.

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.

---

 mm/nommu.c |   21 +++------------------
 1 file changed, 3 insertions(+), 18 deletions(-)
mm/nommu.c +3 -18
index 3abd084..4462b6a 100644
@@ -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);
 }
 
 /*
#2 lore raw
Quoted 1 line · David Howells

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

author
#3 lore raw
Quoted 8 lines · David Howells

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
Quoted 8 lines

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.

Quoted 4 lines

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.