On Tue, Mar 15, 2011 at 10:45 AM, Shaohua Li <shaohua.li@xxxxxxxxx> wrote:
> On Mon, 2011-03-14 at 22:34 +0800, Minchan Kim wrote:
>> Sorry for the late review.
>>
>> On Thu, Mar 10, 2011 at 01:30:18PM +0800, Shaohua Li wrote:
>> > Clean up code and remove duplicate code. Next patch will use
>> > pagevec_lru_move_fn introduced here too.
>> >
>> > Signed-off-by: Shaohua Li <shaohua.li@xxxxxxxxx>
>> Reviewed-by: Minchan Kim <minchan.kim@xxxxxxxxx>
>>
>> There is a just nitpick below but I don't care about it if you don't mind it.
>> It's up to you or Andrew.
>>
>> >
>> > ---
>> > Âmm/swap.c | Â133 +++++++++++++++++++++++++++-----------------------------------
>> > Â1 file changed, 58 insertions(+), 75 deletions(-)
>> >
>> > Index: linux/mm/swap.c
>> > ===================================================================
>> > --- linux.orig/mm/swap.c  Â2011-03-09 12:47:09.000000000 +0800
>> > +++ linux/mm/swap.c 2011-03-09 13:39:26.000000000 +0800
>> > @@ -179,15 +179,13 @@ void put_pages_list(struct list_head *pa
>> > Â}
>> > ÂEXPORT_SYMBOL(put_pages_list);
>> >
>> > -/*
>> > - * pagevec_move_tail() must be called with IRQ disabled.
>> > - * Otherwise this may cause nasty races.
>> > - */
>> > -static void pagevec_move_tail(struct pagevec *pvec)
>> > +static void pagevec_lru_move_fn(struct pagevec *pvec,
>> > + Â Â Â Â Â Â Â Â Â Â Â Â Â void (*move_fn)(struct page *page, void *arg),
>> > + Â Â Â Â Â Â Â Â Â Â Â Â Â void *arg)
>> > Â{
>> > Â Â int i;
>> > - Â int pgmoved = 0;
>> > Â Â struct zone *zone = NULL;
>> > + Â unsigned long flags = 0;
>> >
>> > Â Â for (i = 0; i < pagevec_count(pvec); i++) {
>> > Â Â Â Â Â Â struct page *page = pvec->pages[i];
>> > @@ -195,30 +193,50 @@ static void pagevec_move_tail(struct pag
>> >
>> > Â Â Â Â Â Â if (pagezone != zone) {
>> > Â Â Â Â Â Â Â Â Â Â if (zone)
>> > - Â Â Â Â Â Â Â Â Â Â Â Â Â spin_unlock(&zone->lru_lock);
>> > + Â Â Â Â Â Â Â Â Â Â Â Â Â spin_unlock_irqrestore(&zone->lru_lock, flags);
>> > Â Â Â Â Â Â Â Â Â Â zone = pagezone;
>> > - Â Â Â Â Â Â Â Â Â spin_lock(&zone->lru_lock);
>> > - Â Â Â Â Â }
>> > - Â Â Â Â Â if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
>> > - Â Â Â Â Â Â Â Â Â enum lru_list lru = page_lru_base_type(page);
>> > - Â Â Â Â Â Â Â Â Â list_move_tail(&page->lru, &zone->lru[lru].list);
>> > - Â Â Â Â Â Â Â Â Â mem_cgroup_rotate_reclaimable_page(page);
>> > - Â Â Â Â Â Â Â Â Â pgmoved++;
>> > + Â Â Â Â Â Â Â Â Â spin_lock_irqsave(&zone->lru_lock, flags);
>> > Â Â Â Â Â Â }
>> > +
>> > + Â Â Â Â Â (*move_fn)(page, arg);
>> > Â Â }
>> > Â Â if (zone)
>> > - Â Â Â Â Â spin_unlock(&zone->lru_lock);
>> > - Â __count_vm_events(PGROTATED, pgmoved);
>> > + Â Â Â Â Â spin_unlock_irqrestore(&zone->lru_lock, flags);
>> > Â Â release_pages(pvec->pages, pvec->nr, pvec->cold);
>> > Â Â pagevec_reinit(pvec);
>> > Â}
>> >
>> > +static void pagevec_move_tail_fn(struct page *page, void *arg)
>> > +{
>> > + Â int *pgmoved = arg;
>> > + Â struct zone *zone = page_zone(page);
>> > +
>> > + Â if (PageLRU(page) && !PageActive(page) && !PageUnevictable(page)) {
>> > + Â Â Â Â Â enum lru_list lru = page_lru_base_type(page);
>> > + Â Â Â Â Â list_move_tail(&page->lru, &zone->lru[lru].list);
>> > + Â Â Â Â Â mem_cgroup_rotate_reclaimable_page(page);
>> > + Â Â Â Â Â (*pgmoved)++;
>> > + Â }
>> > +}
>> > +
>> > +/*
>> > + * pagevec_move_tail() must be called with IRQ disabled.
>> > + * Otherwise this may cause nasty races.
>> > + */
>> > +static void pagevec_move_tail(struct pagevec *pvec)
>> > +{
>> > + Â int pgmoved = 0;
>> > +
>> > + Â pagevec_lru_move_fn(pvec, pagevec_move_tail_fn, &pgmoved);
>> > + Â __count_vm_events(PGROTATED, pgmoved);
>> > +}
>> > +
>>
>> Do we really need 3rd argument of pagevec_lru_move_fn?
>> It seems to be used just only pagevec_move_tail_fn.
>> But let's think about it again.
>> The __count_vm_events(pgmoved) could be done in pagevec_move_tail_fn.
>>
>> I don't like unnecessary argument passing although it's not a big overhead.
>> I want to make the code simple if we don't have any reason.
> Sure, making code simple is always preferred.
> ___pagevec_lru_add_fn uses the third the parameter too.
Oops. Sorry. I missed that.
I have no objection from right now. :)
Thanks.
--
Kind regards,
Minchan Kim
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href