[Devel] [PATCH rh7] memcg: allow to disable cleancache per memcg

[Vladimir Davydov]

On Thu, Oct 08, 2015 at 06:41:40PM +0300, Kirill Tkhai wrote:
...
> >> @@ -326,6 +326,10 @@ struct mem_cgroup {
> >>  	/* For oom notifier event fd */
> >>  	struct list_head oom_notify;
> >>  
> >> +#ifdef CONFIG_CLEANCACHE
> >> +	bool cleancache_disabled;
> >> +#endif
> > 
> 
> This is not a good place to put 1-byte variable. Above and below are
> unsigned long variables, so this lose 7 byte just by design.
>  
> I advise you to put it together with any another bool variable.

np

>  
> >> +
> >>  	/*
> >>  	 * Should we move charges of a task when a task is moved into this
> >>  	 * mem_cgroup ? And what type of charges should we move ?
> >> @@ -1577,6 +1581,37 @@ bool mem_cgroup_low(struct mem_cgroup *root, struct mem_cgroup *memcg)
> >>  	return true;
> >>  }
> >>  
> >> +#ifdef CONFIG_CLEANCACHE
> >> +bool mem_cgroup_cleancache_disabled(struct page *page)
> >> +{
> >> +	struct mem_cgroup *memcg = NULL;
> >> +	struct page_cgroup *pc;
> >> +	bool ret = false;
> >> +
> >> +	if (mem_cgroup_disabled())
> >> +		goto out;
> >> +
> >> +	pc = lookup_page_cgroup(page);
> >> +	if (!PageCgroupUsed(pc))
> >> +		goto out;
> >> +
> >> +	lock_page_cgroup(pc);
> >> +	if (!PageCgroupUsed(pc))
> >> +		goto out_unlock;
> >> +
> >> +	for (memcg = pc->mem_cgroup; memcg; memcg = parent_mem_cgroup(memcg)) {
> >> +		if (memcg->cleancache_disabled) {
> >> +			ret = true;
> >> +			break;
> >> +		}
> >> +	}
> >> +out_unlock:
> >> +	unlock_page_cgroup(pc);
> >> +out:
> >> +	return ret;
> >> +}
> > 
> I really dislike the design. This code is likely case and it should be as lightweight
> as possible. But the patch complicates it instead of making that on unlikely path.
>  
> Can't we in mem_cgroup_disable_cleancache_write() propagate mem_cgroup::cleancache value
> to all of our children? Yeah, we will have to have a possibility to differ manually and
> propagated values, but an implementation of a couple of flags instead of bool value should
> not be difficult here.

Makes sense, will rework this part.

Thanks,
Vladimir