On Wed, Apr 10, 2013 at 11:17:30PM +0530, Aneesh Kumar K.V wrote:
> "Aneesh Kumar K.V" <aneesh.kumar@xxxxxxxxxxxxxxxxxx> writes:
>
> > David Gibson <dwg@xxxxxxxxxxx> writes:
> >
> >> On Wed, Apr 10, 2013 at 11:59:29AM +0530, Aneesh Kumar K.V wrote:
> >>> David Gibson <dwg@xxxxxxxxxxx> 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
Attachment:
signature.asc
Description: Digital signature