On Sat, Nov 16, 2019 at 11:15:02AM +0800, Alex Shi wrote:
> @@ -192,26 +190,17 @@ static void pagevec_lru_move_fn(struct pagevec *pvec,
> void *arg)
> {
> int i;
> - struct pglist_data *pgdat = NULL;
> - struct lruvec *lruvec;
> - unsigned long flags = 0;
> + struct lruvec *lruvec = NULL;
>
> for (i = 0; i < pagevec_count(pvec); i++) {
> struct page *page = pvec->pages[i];
> - struct pglist_data *pagepgdat = page_pgdat(page);
>
> - if (pagepgdat != pgdat) {
> - if (pgdat)
> - spin_unlock_irqrestore(&pgdat->lru_lock, flags);
> - pgdat = pagepgdat;
> - spin_lock_irqsave(&pgdat->lru_lock, flags);
> - }
> + lruvec = lock_page_lruvec_irqsave(page, page_pgdat(page));
>
> - lruvec = mem_cgroup_page_lruvec(page, pgdat);
> (*move_fn)(page, lruvec, arg);
> + spin_unlock_irqrestore(&lruvec->lru_lock, lruvec->irqflags);
> }
> - if (pgdat)
> - spin_unlock_irqrestore(&pgdat->lru_lock, flags);
> +
> release_pages(pvec->pages, pvec->nr);
> pagevec_reinit(pvec);
> }
Why can't you keep the locking pattern where we only drop and reacquire if the
lruvec changes? It'd save a lot of locks and unlocks if most pages were from
the same memcg and node, or the memory controller were unused.
Thanks for running the -readtwice benchmark, by the way.
