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@xxxxxxx> 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?

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/