Re: [PATCH -mmotm 4/5] memcg: avoid oom during recharge at task move

[Balbir Singh <balbir@linux.vnet.ibm.com>]

* nishimura@mxp.nes.nec.co.jp <nishimura@mxp.nes.nec.co.jp> [2009-11-19 13:30:30]:

> This recharge-at-task-move feature has extra charges(pre-charges) on "to"
> mem_cgroup during recharging. This means unnecessary oom can happen.
> 
> This patch tries to avoid such oom.
> 
> Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> ---
>  mm/memcontrol.c |   28 ++++++++++++++++++++++++++++
>  1 files changed, 28 insertions(+), 0 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index df363da..3a07383 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -249,6 +249,7 @@ struct recharge_struct {
>  	struct mem_cgroup *from;
>  	struct mem_cgroup *to;
>  	unsigned long precharge;
> +	struct task_struct *working;	/* a task moving the target task */

working does not sound like an appropriate name

>  };
>  static struct recharge_struct recharge;
> 
> @@ -1494,6 +1495,30 @@ static int __mem_cgroup_try_charge(struct mm_struct *mm,
>  		if (mem_cgroup_check_under_limit(mem_over_limit))
>  			continue;
> 
> +		/* try to avoid oom while someone is recharging */
> +		if (recharge.working && current != recharge.working) {
> +			struct mem_cgroup *dest;
> +			bool do_continue = false;
> +			/*
> +			 * There is a small race that "dest" can be freed by
> +			 * rmdir, so we use css_tryget().
> +			 */
> +			rcu_read_lock();
> +			dest = recharge.to;
> +			if (dest && css_tryget(&dest->css)) {
> +				if (dest->use_hierarchy)
> +					do_continue = css_is_ancestor(
> +							&dest->css,
> +							&mem_over_limit->css);
> +				else
> +					do_continue = (dest == mem_over_limit);
> +				css_put(&dest->css);
> +			}
> +			rcu_read_unlock();
> +			if (do_continue)
> +				continue;

IIUC, if dest is the current cgroup we are trying to charge to or an
ancestor of the current cgroup, we don't OOM?

> +		}
> +
>  		if (!nr_retries--) {
>  			if (oom) {
>  				mem_cgroup_out_of_memory(mem_over_limit, gfp_mask);
> @@ -3474,6 +3499,7 @@ static void mem_cgroup_clear_recharge(void)
>  	}
>  	recharge.from = NULL;
>  	recharge.to = NULL;
> +	recharge.working = NULL;
>  }
> 
>  static int mem_cgroup_can_attach(struct cgroup_subsys *ss,
> @@ -3498,9 +3524,11 @@ static int mem_cgroup_can_attach(struct cgroup_subsys *ss,
>  			VM_BUG_ON(recharge.from);
>  			VM_BUG_ON(recharge.to);
>  			VM_BUG_ON(recharge.precharge);
> +			VM_BUG_ON(recharge.working);
>  			recharge.from = from;
>  			recharge.to = mem;
>  			recharge.precharge = 0;
> +			recharge.working = current;
> 
>  			ret = mem_cgroup_prepare_recharge(mm);
>  			if (ret)

Sorry, if I missed it, but I did not see any time overhead of moving a
task after these changes. Could you please help me understand the cost
of moving say a task with 1G anonymous memory to another group and
the cost of moving a task with 512MB anonymous and 512 page cache
mapped, etc. It would be nice to understand the overall cost.

-- 
	Balbir

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>