Re: [PATCH] Add file based RSS accounting for memory resourcecontroller (v2)

[Balbir Singh]

* KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx> [2009-04-17 09:14:59]:

> On Thu, 16 Apr 2009 17:33:16 +0530
> Balbir Singh <balbir@xxxxxxxxxxxxxxxxxx> wrote:
> 
> > * KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx> [2009-04-16 17:15:35]:
> > 
> > > 
> > > > Sorry, some troubles found. Ignore above Ack. 3points now.
> > > > 
> > > > 1. get_cpu should be after (*)
> > > > ==mem_cgroup_update_mapped_file_stat()
> > > > +	int cpu = get_cpu();
> > > > +
> > > > +	if (!page_is_file_cache(page))
> > > > +		return;
> > > > +
> > > > +	if (unlikely(!mm))
> > > > +		mm = &init_mm;
> > > > +
> > > > +	mem = try_get_mem_cgroup_from_mm(mm);
> > > > +	if (!mem)
> > > > +		return;
> > > > + ----------------------------------------(*)
> > > > +	stat = &mem->stat;
> > > > +	cpustat = &stat->cpustat[cpu];
> > > > +
> > > > +	__mem_cgroup_stat_add_safe(cpustat, MEM_CGROUP_STAT_MAPPED_FILE, val);
> > > > +	put_cpu();
> > > > +}
> > > > ==
> > 
> > Yes or I should have a goto
> > 
> > > > 
> > > > 2. In above, "mem" shouldn't be got from "mm"....please get "mem" from page_cgroup.
> > > > (Because it's file cache, pc->mem_cgroup is not NULL always.)
> > 
> > Hmmm.. not sure I understand this part. Are you suggesting that mm can
> > be NULL?
> No.
> 
> > I added the check for !mm as a safety check. Since this
> > routine is only called from rmap context, mm is not NULL, hence mem
> > should not be NULL. Did you find a race between mm->owner assignment
> > and lookup via mm->owner?
> > 
> No.
> 
> page_cgroup->mem_cgroup != try_get_mem_cgroup_from_mm(mm);  in many many cases.
> 
> For example, libc and /bin/*** is tend to be loaded into default cgroup at boot but
> used by many cgroups. But mapcount of page caches for /bin/*** is 0 if not running.
> 
> Then, File_Mapped can be greater than Cached easily if you use mm->owner.
> 
> I can't estimate RSS in *my* cgroup if File_Mapped includes pages which is under 
> other cgroups. It's meaningless.
> Especially, when Cached==0 but File_Mapped > 0, I think "oh, the kernel leaks somehing..hmm..."
> 
> By useing page_cgroup->mem_cgroup, we can avoid above mess.

Yes, I see your point. I wanted mapped_file to show up in the cgroup
that mapped the page. But this works for me as well, but that means
we'll nest the page cgroup lock under the PTE lock.

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