Re: [patch 4/4] mm: return boolean from page_has_private()

[Johannes Weiner <hannes@cmpxchg.org>]

On Wed, Jul 22, 2009 at 12:49:44PM -0400, Christoph Lameter wrote:
> On Tue, 21 Jul 2009, Johannes Weiner wrote:
> 
> > Make page_has_private() return a true boolean value and remove the
> > double negations from the two callsites using it for arithmetic.
> 
> page_has_private_data()?

I am not so fond of changing that, because then we should probably
also rename page_private() and that is moot for slightly improved
source code English.

> Also note that you are adding unecessary double negation to the other
> callers. Does the compiler catch that?

Yes, callsites using it in a conditionals do not change here with gcc
4.3.3.

> > +static inline int page_has_private(struct page *page)
> > +{
> > +	return !!(page->flags & ((1 << PG_private) | (1 << PG_private_2)));
> > +}
> 
> Two private bits? How did that happen?

fscache :)

> Could we define a PAGE_FLAGS_PRIVATE in page-flags.h?

It would certainly look nicer, I will add that.

> > diff --git a/mm/vmscan.c b/mm/vmscan.c
> > index 6b368d3..67e2824 100644
> > --- a/mm/vmscan.c
> > +++ b/mm/vmscan.c
> > @@ -286,7 +286,7 @@ static inline int page_mapping_inuse(struct page *page)
> >
> >  static inline int is_page_cache_freeable(struct page *page)
> >  {
> > -	return page_count(page) - !!page_has_private(page) == 2;
> > +	return page_count(page) - page_has_private(page) == 2;
> 
> That looks funky and in need of comments.

Agreed, I will add one in a different patch.

	Hannes

---