Re: [patch 03/21] use an array for the LRU pagevecs

KOSAKI Motohiro
2008-03-01T07:03:21Z

Hi Andy

sorry, almost mistake maked by me.

> > #define for_each_lru(l) for (l = 0; l < NR_LRU_LISTS; l++)
> >
> > +static inline int is_active_lru(enum lru_list l)
> > +{
> > + if (l == LRU_ACTIVE)
> > + return 1;
> > + return 0;
>
> Can this not be:
>
> return (l == LRU_ACTIVE);

yes, your code is more better.

Thanks.

> > @@ -98,6 +97,19 @@ void put_pages_list(struct list_head *pa
> > EXPORT_SYMBOL(put_pages_list);
> >
> > /*
> > + * Returns the LRU list a page should be on.
> > + */
> > +enum lru_list page_lru(struct page *page)
> > +{
> > + enum lru_list lru = LRU_BASE;
> > +
> > + if (PageActive(page))
> > + lru += LRU_ACTIVE;
>
> This is introducing an assumption that LRU_BASE and LRU_INACTIVE are
> synonymous? Would it not be better to explicitly use LRU_INACTIVE:

Yes.
this patch series assume LRU_BASE == LRU_INACTIVE.

> So either:
>
> if (PageActive(page))
> lru = LRU_ACTIVE;
> else
> lru = LRU_INACTIVE;

if add LRU_FILE, this statement makes messy.
key point of indexed arraynize is able to arithmetic calculationed.

> Or if (as I assume) this is later going to have other mappings added in
> you could do it more like the following. This should produce identicle
> asm, but removes any possiblity of LRU_BASE/INACTIVE slippage breaking
> things:
>
> enum lru_list lru = LRU_INACTIVE;
>
> if (PageActive(page))
> lru += (LRU_ACTIVE - LRU_INACTIVE);

I think your code is more descriptive, but not best simply.
I think LRU_BASE == LRU_INACTIVE assumption make simply code.

> > - struct pagevec *pvec = &get_cpu_var(lru_add_active_pvecs);
> > + if (PageActive(page)) {
> > + ClearPageActive(page);
> > + }
>
> {}'s are not needed here.

agghh
thank you good catch ;)

Thank you again for your very careful reviews.

- kosaki