Re: [PATCH 1/2 v4]mm: simplify code of swap.c

[Shaohua Li <shaohua.li@intel.com>]

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@intel.com>
> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> 
> 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.

Thanks,
Shaohua


--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>