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

[David Gibson]

On Wed, Apr 10, 2013 at 11:17:30PM +0530, Aneesh Kumar K.V wrote:
> "Aneesh Kumar K.V" <aneesh.kumar at linux.vnet.ibm.com> writes:
> 
> > David Gibson <dwg at au1.ibm.com> writes:
> >
> >> On Wed, Apr 10, 2013 at 11:59:29AM +0530, Aneesh Kumar K.V wrote:
> >>> David Gibson <dwg at au1.ibm.com> writes:
> >>> > On Thu, Apr 04, 2013 at 11:27:44AM +0530, Aneesh Kumar K.V wrote:
> >> [snip]
> >>> >> @@ -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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20130411/c413b6c1/attachment-0001.sig>