Re: [patch 2/4] mm: introduce page_lru_type()

[Minchan Kim <minchan.kim@gmail.com>]

On Wed, Jul 22, 2009 at 6:07 PM, Johannes Weiner<hannes@cmpxchg.org> wrote:
> Hello Minchan,
>
> On Wed, Jul 22, 2009 at 10:52:21AM +0900, Minchan Kim wrote:
>> Hi.
>>
>> On Tue, Jul 21, 2009 at 5:56 PM, Johannes Weiner<hannes@cmpxchg.org> wrote:
>> > Instead of abusing page_is_file_cache() for LRU list index arithmetic,
>> > add another helper with a more appropriate name and convert the
>> > non-boolean users of page_is_file_cache() accordingly.
>> >
>> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
>> > ---
>> >  include/linux/mm_inline.h |   19 +++++++++++++++++--
>> >  mm/swap.c                 |    4 ++--
>> >  mm/vmscan.c               |    6 +++---
>> >  3 files changed, 22 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/include/linux/mm_inline.h b/include/linux/mm_inline.h
>> > index 7fbb972..ec975f2 100644
>> > --- a/include/linux/mm_inline.h
>> > +++ b/include/linux/mm_inline.h
>> > @@ -60,6 +60,21 @@ del_page_from_lru(struct zone *zone, struct page *page)
>> >  }
>> >
>> >  /**
>> > + * page_lru_type - which LRU list type should a page be on?
>> > + * @page: the page to test
>> > + *
>> > + * Used for LRU list index arithmetic.
>> > + *
>> > + * Returns the base LRU type - file or anon - @page should be on.
>> > + */
>> > +static enum lru_list page_lru_type(struct page *page)
>> > +{
>> > +       if (page_is_file_cache(page))
>> > +               return LRU_INACTIVE_FILE;
>> > +       return LRU_INACTIVE_ANON;
>> > +}
>>
>> page_lru_type function's semantics is general but this function only
>> considers INACTIVE case.
>> So we always have to check PageActive to know exact lru type.
>>
>> Why do we need double check(ex, page_lru_type and PageActive) to know
>> exact lru type ?
>>
>> It wouldn't be better to check it all at once ?
>
> page_lru() does that for you already.
>
> But look at the users of page_lru_type(), they know the active bit
> when they want to find out the base type, see check_move_unevictable
> e.g.

Yes. You already mentioned proper function name. :)

How about changing function name from page_lru_type to page_lru_base_type ?
it might be a nitpick. :)

-- 
Kind regards,
Minchan Kim

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>