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.
--
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=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>