Re: [PATCH 4/4] memcg: simplify LRU handling by new rule

[KAMEZAWA Hiroyuki]

On Tue, 20 Dec 2011 17:16:15 +0100
Michal Hocko <mhocko@xxxxxxx> wrote:

> On Wed 14-12-11 16:52:26, KAMEZAWA Hiroyuki wrote:
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> > 
> > Now, at LRU handling, memory cgroup needs to do complicated works
> > to see valid pc->mem_cgroup, which may be overwritten.
> > 
> > This patch is for relaxing the protocol. This patch guarantees
> >    - when pc->mem_cgroup is overwritten, page must not be on LRU.
> 
> How the patch guarantees that? I do not see any enforcement. In fact we
> depend on the previous patches, don't we.
> 

Ah, yes. We depends on previous patch series.


> > 
> > By this, LRU routine can believe pc->mem_cgroup and don't need to
> > check bits on pc->flags. This new rule may adds small overheads to
> > swapin. But in most case, lru handling gets faster.
> > 
> > After this patch, PCG_ACCT_LRU bit is obsolete and removed.
> 
> It makes things much more simpler. I just think it needs a better
> description.
> 

O.K.

99% of memcg charging are done by following call path.

   - alloc_page() -> charge() -> map/enter radix-tree -> add to LRU.

We need some special case cares.

   - SwapCache - newly allocated/fully unmapped pages are added to LRU
                 before charge.
     => handled by previous patch.
   - FUSE      - unused pages are reused.
     => handled by previous patch.

   - move_account
     => we do isolate_page().

Now, we can guarantee pc->mem_cgroup is set when page is not added to
LRU or under zone->lru_lock + isolate from LRU.

I'll add some Documenation to...memcg_debug.txt

Thanks,
-Kame

> > 
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> > ---
> >  include/linux/page_cgroup.h |    8 -----
> >  mm/memcontrol.c             |   72 ++++++++++--------------------------------
> >  2 files changed, 17 insertions(+), 63 deletions(-)
> > 
> > diff --git a/include/linux/page_cgroup.h b/include/linux/page_cgroup.h
> > index aaa60da..2cddacf 100644
> > --- a/include/linux/page_cgroup.h
> > +++ b/include/linux/page_cgroup.h
> > @@ -10,8 +10,6 @@ enum {
> >  	/* flags for mem_cgroup and file and I/O status */
> >  	PCG_MOVE_LOCK, /* For race between move_account v.s. following bits */
> >  	PCG_FILE_MAPPED, /* page is accounted as "mapped" */
> > -	/* No lock in page_cgroup */
> > -	PCG_ACCT_LRU, /* page has been accounted for (under lru_lock) */
> >  	__NR_PCG_FLAGS,
> >  };
> >  
> > @@ -75,12 +73,6 @@ TESTPCGFLAG(Used, USED)
> >  CLEARPCGFLAG(Used, USED)
> >  SETPCGFLAG(Used, USED)
> >  
> > -SETPCGFLAG(AcctLRU, ACCT_LRU)
> > -CLEARPCGFLAG(AcctLRU, ACCT_LRU)
> > -TESTPCGFLAG(AcctLRU, ACCT_LRU)
> > -TESTCLEARPCGFLAG(AcctLRU, ACCT_LRU)
> > -
> > -
> >  SETPCGFLAG(FileMapped, FILE_MAPPED)
> >  CLEARPCGFLAG(FileMapped, FILE_MAPPED)
> >  TESTPCGFLAG(FileMapped, FILE_MAPPED)
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 2ae973d..d5e21e7 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -974,30 +974,8 @@ struct lruvec *mem_cgroup_lru_add_list(struct zone *zone, struct page *page,
> >  		return &zone->lruvec;
> >  
> >  	pc = lookup_page_cgroup(page);
> > -	VM_BUG_ON(PageCgroupAcctLRU(pc));
> > -	/*
> > -	 * putback:				charge:
> > -	 * SetPageLRU				SetPageCgroupUsed
> > -	 * smp_mb				smp_mb
> > -	 * PageCgroupUsed && add to memcg LRU	PageLRU && add to memcg LRU
> > -	 *
> > -	 * Ensure that one of the two sides adds the page to the memcg
> > -	 * LRU during a race.
> > -	 */
> > -	smp_mb();
> > -	/*
> > -	 * If the page is uncharged, it may be freed soon, but it
> > -	 * could also be swap cache (readahead, swapoff) that needs to
> > -	 * be reclaimable in the future.  root_mem_cgroup will babysit
> > -	 * it for the time being.
> > -	 */
> > -	if (PageCgroupUsed(pc)) {
> > -		/* Ensure pc->mem_cgroup is visible after reading PCG_USED. */
> > -		smp_rmb();
> > -		memcg = pc->mem_cgroup;
> > -		SetPageCgroupAcctLRU(pc);
> > -	} else
> > -		memcg = root_mem_cgroup;
> > +	memcg = pc->mem_cgroup;
> > +	VM_BUG_ON(!memcg);
> >  	mz = page_cgroup_zoneinfo(memcg, page);
> >  	/* compound_order() is stabilized through lru_lock */
> >  	MEM_CGROUP_ZSTAT(mz, lru) += 1 << compound_order(page);
> > @@ -1024,18 +1002,8 @@ void mem_cgroup_lru_del_list(struct page *page, enum lru_list lru)
> >  		return;
> >  
> >  	pc = lookup_page_cgroup(page);
> > -	/*
> > -	 * root_mem_cgroup babysits uncharged LRU pages, but
> > -	 * PageCgroupUsed is cleared when the page is about to get
> > -	 * freed.  PageCgroupAcctLRU remembers whether the
> > -	 * LRU-accounting happened against pc->mem_cgroup or
> > -	 * root_mem_cgroup.
> > -	 */
> > -	if (TestClearPageCgroupAcctLRU(pc)) {
> > -		VM_BUG_ON(!pc->mem_cgroup);
> > -		memcg = pc->mem_cgroup;
> > -	} else
> > -		memcg = root_mem_cgroup;
> > +	memcg = pc->mem_cgroup;
> > +	VM_BUG_ON(!memcg);
> >  	mz = page_cgroup_zoneinfo(memcg, page);
> >  	/* huge page split is done under lru_lock. so, we have no races. */
> >  	MEM_CGROUP_ZSTAT(mz, lru) -= 1 << compound_order(page);
> > @@ -2377,6 +2345,7 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
> >  
> >  	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), nr_pages);
> >  	unlock_page_cgroup(pc);
> > +	WARN_ON_ONCE(PageLRU(page));
> >  	/*
> >  	 * "charge_statistics" updated event counter. Then, check it.
> >  	 * Insert ancestor (and ancestor's ancestors), to softlimit RB-tree.
> > @@ -2388,7 +2357,7 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
> >  #ifdef CONFIG_TRANSPARENT_HUGEPAGE
> >  
> >  #define PCGF_NOCOPY_AT_SPLIT ((1 << PCG_LOCK) | (1 << PCG_MOVE_LOCK) |\
> > -			(1 << PCG_ACCT_LRU) | (1 << PCG_MIGRATION))
> > +			(1 << PCG_MIGRATION))
> >  /*
> >   * Because tail pages are not marked as "used", set it. We're under
> >   * zone->lru_lock, 'splitting on pmd' and compound_lock.
> > @@ -2399,6 +2368,8 @@ void mem_cgroup_split_huge_fixup(struct page *head)
> >  {
> >  	struct page_cgroup *head_pc = lookup_page_cgroup(head);
> >  	struct page_cgroup *pc;
> > +	struct mem_cgroup_per_zone *mz;
> > +	enum lru_list lru;
> >  	int i;
> >  
> >  	if (mem_cgroup_disabled())
> > @@ -2407,23 +2378,15 @@ void mem_cgroup_split_huge_fixup(struct page *head)
> >  		pc = head_pc + i;
> >  		pc->mem_cgroup = head_pc->mem_cgroup;
> >  		smp_wmb();/* see __commit_charge() */
> > -		/*
> > -		 * LRU flags cannot be copied because we need to add tail
> > -		 * page to LRU by generic call and our hooks will be called.
> > -		 */
> >  		pc->flags = head_pc->flags & ~PCGF_NOCOPY_AT_SPLIT;
> >  	}
> > -
> > -	if (PageCgroupAcctLRU(head_pc)) {
> > -		enum lru_list lru;
> > -		struct mem_cgroup_per_zone *mz;
> > -		/*
> > -		 * We hold lru_lock, then, reduce counter directly.
> > -		 */
> > -		lru = page_lru(head);
> > -		mz = page_cgroup_zoneinfo(head_pc->mem_cgroup, head);
> > -		MEM_CGROUP_ZSTAT(mz, lru) -= HPAGE_PMD_NR - 1;
> > -	}
> > +	/* 
> > +	 * Tail pages will be added to LRU.
> > +	 * We hold lru_lock,then,reduce counter directly.
> > +	 */
> > +	lru = page_lru(head);
> > +	mz = page_cgroup_zoneinfo(head_pc->mem_cgroup, head);
> > +	MEM_CGROUP_ZSTAT(mz, lru) -= HPAGE_PMD_NR - 1;
> >  }
> >  #endif
> >  
> > @@ -2656,10 +2619,9 @@ int mem_cgroup_cache_charge(struct page *page, struct mm_struct *mm,
> >  	if (!page_is_file_cache(page))
> >  		type = MEM_CGROUP_CHARGE_TYPE_SHMEM;
> >  
> > -	if (!PageSwapCache(page)) {
> > +	if (!PageSwapCache(page))
> >  		ret = mem_cgroup_charge_common(page, mm, gfp_mask, type);
> > -		WARN_ON_ONCE(PageLRU(page));
> > -	} else { /* page is swapcache/shmem */
> > +	else { /* page is swapcache/shmem */
> >  		ret = mem_cgroup_try_charge_swapin(mm, page, gfp_mask, &memcg);
> >  		if (!ret)
> >  			__mem_cgroup_commit_charge_swapin(page, memcg, type);
> > -- 
> > 1.7.4.1
> > 
> > 
> > --
> > 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>
> 
> -- 
> Michal Hocko
> SUSE Labs
> SUSE LINUX s.r.o.
> Lihovarska 1060/12
> 190 00 Praha 9    
> Czech Republic
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/