Re: [PATCH -V5 06/25] powerpc: Reduce PTE table memory wastage

On Wed, Apr 10, 2013 at 11:17:30PM +0530, Aneesh Kumar K.V wrote:

"Aneesh Kumar K.V" [off-list ref] writes:

quoted

David Gibson [off-list ref] writes:

quoted

On Wed, Apr 10, 2013 at 11:59:29AM +0530, Aneesh Kumar K.V wrote:

quoted

David Gibson [off-list ref] writes:

quoted

On Thu, Apr 04, 2013 at 11:27:44AM +0530, Aneesh Kumar K.V wrote:

[snip]

quoted

quoted

quoted

@@ -97,13 +100,45 @@ void __destroy_context(int context_id)
 }
 EXPORT_SYMBOL_GPL(__destroy_context);
 
+#ifdef CONFIG_PPC_64K_PAGES
+static void destroy_pagetable_page(struct mm_struct *mm)
+{
+	int count;
+	struct page *page;
+
+	page = mm->context.pgtable_page;
+	if (!page)
+		return;
+
+	/* drop all the pending references */
+	count = atomic_read(&page->_mapcount) + 1;
+	/* We allow PTE_FRAG_NR(16) fragments from a PTE page */
+	count = atomic_sub_return(16 - count, &page->_count);

You should really move PTE_FRAG_NR to a header so you can actually use
it here rather than hard coding 16.

It took me a fair while to convince myself that there is no race here
with something altering mapcount and count between the atomic_read()
and the atomic_sub_return().  It could do with a comment to explain
why that is safe.

Re-using the mapcount field for your index also seems odd, and it took
me a while to convince myself that that's safe too.  Wouldn't it be
simpler to store a pointer to the next sub-page in the mm_context
instead? You can get from that to the struct page easily enough with a
shift and pfn_to_page().

I found using _mapcount simpler in this case. I was looking at it not
as an index, but rather how may fragments are mapped/used already.

Except that it's actually (#fragments - 1).  Using subpage pointer
makes the fragments calculation (very slightly) harder, but the
calculation of the table address easier.  More importantly it avoids
adding effectively an extra variable - which is then shoehorned into a
structure not really designed to hold it.

Even with subpage pointer we would need mm->context.pgtable_page or
something similar. We don't add any other extra variable right ?. Let me
try what you are suggesting here and see if that make it simpler.

Here is what I ended up with. I will fold this in next update

Yeah, that looks better to me.  Note that ~PAGE_MASK is the more usual
idiom, rather than (PAGE_SIZE - 1).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson