Re: [patch] page->flags cleanup

[Andrew Morton <akpm@zip.com.au>]

Hugh Dickins wrote:
> 
> ...
> > - UnlockPage is removed.  All callers updated to use unlock_page().
> >   it's a real function - there's no need to hide that fact.
> 
> Hmm, well, I'd prefer not to change that very widely used name;
> but I've no serious objection if you wish to.

UnlockPage() looks like a simple clear_bit(), but it very much isn't.
I think it's better this way.

> ...
> 
> > - PageSwapCache() is renamed to page_swap_cache(), so it doesn't
> >   pretend to be a page->flags bit test.
> 
> Again, I'd prefer to leave PageSwapCache as is: it used to have a
> page->flags bit, it might be given a page->flags bit again in future
> (multiple swapper_spaces?).  I don't think "Page" in the macro name
> should have to imply implementation using a page->flags bit.  But
> again, I've no serious objection if you wish to make this change.

OK, I re-renamed the predicate back to PageSwapCache, added some
commentary about this.
 
> ...
> 
> 1. The two "if (PageSwapCache(page)) BUG();"s  in mm/page_alloc.c
>    are redundant and should just be deleted rather than converted:
>    we have just checked that page->mapping is unset, so (excepting
>    a volatile mod of a kind which would need an infinite number of
>    identical tests to protect against) of course it isn't &swapper_space
>    (but the compiler doesn't optimize away).  The two PageSwapCache BUG
>    tests in mm/page_io.c similarly redundant and should also be deleted.

OK, I've done this in a separate patch.
 
> 2. I can't see from your mail (patch against an earlier version?) what
>    happened to the comment immediately above #define PageError(page)
>    in 2.5.9/include/linux/mm.h - the comment beginning "The first mb".
>    That comment originally belonged to UnlockPage(), and should have
>    been moved to unlock_page() when that went into mm/filemap.c.
>    I sometimes wonder whether those two "mb"s are actually still
>    required (quite a lot has changed since they went in), but that's
>    a different kind of question, and certainly not one I can answer.

I moved the comment to unlock_page().

> 3. You're removing PG_skip and shifting highers down (in patch you
>    mailed separately): good, but please remove PG_unused too and shift
>    highers down (I cautiously renamed PG_swap_cache to PG_unused when
>    changing PageSwapCache macro, the time for that caution has past).

I wondered what that was doing there ;)  Done.

Updated patch series (compiles, untested, should be OK) is at
http://www.zip.com.au/~akpm/linux/patches/2.5/2.5.10/

Thanks.

Sometime I'll bite the bullet and actually change all .c
files to include page-flags.h direct.  I did that yesterday
but wasn't happy with it.  There are some unfortunate dependencies
in pagemap.h and highmem.h which need cleaning up first.

-