Re: [PATCH -V4 04/10] memcg: Add HugeTLB extension

[Aneesh Kumar K.V]

On Mon, 19 Mar 2012 11:38:38 +0900, KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx> wrote:
> (2012/03/17 2:39), Aneesh Kumar K.V wrote:
> 
> > From: "Aneesh Kumar K.V" <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
> > 
> > This patch implements a memcg extension that allows us to control
> > HugeTLB allocations via memory controller.
> > 
> 
> 
> If you write some details here, it will be helpful for review and
> seeing log after merge.

Will add more info.

> 
> 
> > Signed-off-by: Aneesh Kumar K.V <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
> > ---
> >  include/linux/hugetlb.h    |    1 +
> >  include/linux/memcontrol.h |   42 +++++++++++++
> >  init/Kconfig               |    8 +++
> >  mm/hugetlb.c               |    2 +-
> >  mm/memcontrol.c            |  138 ++++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 190 insertions(+), 1 deletions(-)

....

> > +#ifdef CONFIG_MEM_RES_CTLR_HUGETLB
> > +static bool mem_cgroup_have_hugetlb_usage(struct mem_cgroup *memcg)
> > +{
> > +	int idx;
> > +	for (idx = 0; idx < hugetlb_max_hstate; idx++) {
> > +		if (memcg->hugepage[idx].usage > 0)
> > +			return 1;
> > +	}
> > +	return 0;
> > +}
> 
> 
> Please use res_counter_read_u64() rather than reading the value directly.
> 

The open-coded variant is mostly derived from mem_cgroup_force_empty. I
have updated the patch to use res_counter_read_u64. 

> 
> > +
> > +int mem_cgroup_hugetlb_charge_page(int idx, unsigned long nr_pages,
> > +				   struct mem_cgroup **ptr)
> > +{
> > +	int ret = 0;
> > +	struct mem_cgroup *memcg;
> > +	struct res_counter *fail_res;
> > +	unsigned long csize = nr_pages * PAGE_SIZE;
> > +
> > +	if (mem_cgroup_disabled())
> > +		return 0;
> > +again:
> > +	rcu_read_lock();
> > +	memcg = mem_cgroup_from_task(current);
> > +	if (!memcg)
> > +		memcg = root_mem_cgroup;
> > +	if (mem_cgroup_is_root(memcg)) {
> > +		rcu_read_unlock();
> > +		goto done;
> > +	}
> 
> 
> One concern is.... Now, yes, memory cgroup doesn't account root cgroup
> and doesn't update res->usage to avoid updating shared counter overheads
> when memcg is not mounted. But memory.usage_in_bytes files works
> for root memcg with reading percpu statistics.
> 
> So, how about counting usage for root cgroup even if it cannot be limited ?
> Considering hugetlb fs usage, updating res_counter here doesn't have
> performance problem of false sharing..
> Then, you can remove root_mem_cgroup() checks inserted several places.
> 

Yes. That is a good idea. Will update the patch.


> <snip>
> 
> >  	struct mem_cgroup *memcg = mem_cgroup_from_cont(cont);
> > +	/*
> > +	 * Don't allow memcg removal if we have HugeTLB resource
> > +	 * usage.
> > +	 */
> > +	if (mem_cgroup_have_hugetlb_usage(memcg))
> > +		return -EBUSY;
> >  
> >  	return mem_cgroup_force_empty(memcg, false);
> >  }
> 
> 
> Is this fixed by patch 8+9 ?

Yes. 

-aneesh

--
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/