[patch] i386: pageattr remove __put_page

[patch] i386: pageattr remove __put_page

[Nick Piggin]

Stop using __put_page and page_count in i386 pageattr.c

Signed-off-by: Nick Piggin <npiggin@suse.de>

Index: linux-2.6/arch/i386/mm/pageattr.c
===================================================================
--- linux-2.6.orig/arch/i386/mm/pageattr.c
+++ linux-2.6/arch/i386/mm/pageattr.c
@@ -51,6 +51,9 @@ static struct page *split_large_page(uns
 	if (!base) 
 		return NULL;
 
+	SetPagePrivate(base);
+	page_private(base) = 0;
+
 	address = __pa(address);
 	addr = address & LARGE_PAGE_MASK; 
 	pbase = (pte_t *)page_address(base);
@@ -143,11 +146,12 @@ __change_page_attr(struct page *page, pg
 				return -ENOMEM;
 			set_pmd_pte(kpte,address,mk_pte(split, ref_prot));
 			kpte_page = split;
-		}	
-		get_page(kpte_page);
+		}
+		page_private(kpte_page)++;
 	} else if ((pte_val(*kpte) & _PAGE_PSE) == 0) { 
 		set_pte_atomic(kpte, mk_pte(page, PAGE_KERNEL));
-		__put_page(kpte_page);
+		BUG_ON(page_private(kpte_page) == 0);
+		page_private(kpte_page)--;
 	} else
 		BUG();
 
@@ -157,10 +161,8 @@ __change_page_attr(struct page *page, pg
 	 * replace it with a largepage.
 	 */
 	if (!PageReserved(kpte_page)) {
-		/* memleak and potential failed 2M page regeneration */
-		BUG_ON(!page_count(kpte_page));
-
-		if (cpu_has_pse && (page_count(kpte_page) == 1)) {
+		if (cpu_has_pse && (page_private(kpte_page) == 0)) {
+			ClearPagePrivate(kpte_page);
 			list_add(&kpte_page->lru, &df_list);
 			revert_page(kpte_page, address);
 		}

Re: [patch] i386: pageattr remove __put_page

[Andrew Morton]

Nick Piggin <npiggin@suse.de> wrote:
>
> Stop using __put_page and page_count in i386 pageattr.c
> 

who, where, what, why, when??  The patch appears to ascribe some special
significance to page->private, but you don't tell us what it is.  And if
that's not obvious from reading the patch, it won't be obvious to people
who are later reading the code.

iow: you owe us a nice comment, please.

> 
> Index: linux-2.6/arch/i386/mm/pageattr.c
> ===================================================================
> --- linux-2.6.orig/arch/i386/mm/pageattr.c
> +++ linux-2.6/arch/i386/mm/pageattr.c
> @@ -51,6 +51,9 @@ static struct page *split_large_page(uns
>  	if (!base) 
>  		return NULL;
>  
> +	SetPagePrivate(base);
> +	page_private(base) = 0;

A "function call" as an lval give me hiccups.  Use set_page_private(p, v),
please.

>  	address = __pa(address);
>  	addr = address & LARGE_PAGE_MASK; 
>  	pbase = (pte_t *)page_address(base);
> @@ -143,11 +146,12 @@ __change_page_attr(struct page *page, pg
>  				return -ENOMEM;
>  			set_pmd_pte(kpte,address,mk_pte(split, ref_prot));
>  			kpte_page = split;
> -		}	
> -		get_page(kpte_page);
> +		}
> +		page_private(kpte_page)++;

Ditto, really.   If we're going to be nice about this it should be

	set_page_private(page, page_private(page) + 1);

> +		page_private(kpte_page)--;

Ditto.


Or we just forget about page_private() and go back to using page->private -
page_private() was rather a stopgap thing.

Then again, we perform unnatural acts upon the pageframe so regularly that
I suspect the abstraction might prove useful in the future..

Re: [patch] i386: pageattr remove __put_page

[Nick Piggin]

On Wed, Jan 18, 2006 at 07:00:28PM -0800, Andrew Morton wrote:
> Nick Piggin <npiggin@suse.de> wrote:
> >
> > Stop using __put_page and page_count in i386 pageattr.c
> > 
> 
> who, where, what, why, when??  The patch appears to ascribe some special
> significance to page->private, but you don't tell us what it is.  And if
> that's not obvious from reading the patch, it won't be obvious to people
> who are later reading the code.
> 
> iow: you owe us a nice comment, please.
> 

OK sure, I'll redo it.

> > 
> > Index: linux-2.6/arch/i386/mm/pageattr.c
> > ===================================================================
> > --- linux-2.6.orig/arch/i386/mm/pageattr.c
> > +++ linux-2.6/arch/i386/mm/pageattr.c
> > @@ -51,6 +51,9 @@ static struct page *split_large_page(uns
> >  	if (!base) 
> >  		return NULL;
> >  
> > +	SetPagePrivate(base);
> > +	page_private(base) = 0;
> 
> A "function call" as an lval give me hiccups.  Use set_page_private(p, v),
> please.
> 

Hmm yes of course. That makes it pretty ugly.

> >  	address = __pa(address);
> >  	addr = address & LARGE_PAGE_MASK; 
> >  	pbase = (pte_t *)page_address(base);
> > @@ -143,11 +146,12 @@ __change_page_attr(struct page *page, pg
> >  				return -ENOMEM;
> >  			set_pmd_pte(kpte,address,mk_pte(split, ref_prot));
> >  			kpte_page = split;
> > -		}	
> > -		get_page(kpte_page);
> > +		}
> > +		page_private(kpte_page)++;
> 
> Ditto, really.   If we're going to be nice about this it should be
> 
> 	set_page_private(page, page_private(page) + 1);
> 
> > +		page_private(kpte_page)--;
> 
> Ditto.
> 
> 
> Or we just forget about page_private() and go back to using page->private -
> page_private() was rather a stopgap thing.
> 

Could we? We can do anonymous unions now, right?

Re: [patch] i386: pageattr remove __put_page

[Andrew Morton]

Nick Piggin <npiggin@suse.de> wrote:
>
> > Or we just forget about page_private() and go back to using page->private -
> > page_private() was rather a stopgap thing.
> > 
> 
> Could we? We can do anonymous unions now, right?

Yes, that code's merged up now - we're using anonymous-struct in
anonymous-union and page->private had its leading underscore taken away.

But we should be consistent..