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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. 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>