linux-kernel - Re: [PATCH RFC] mm/memcg: recalculate chargeable space after waiting migrating charges

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20120712110838.GE21013@tiehlicka.suse.cz>
Date:	Thu, 12 Jul 2012 13:08:38 +0200
From:	Michal Hocko <mhocko@...e.cz>
To:	Wanpeng Li <liwp.linux@...il.com>
Cc:	linux-mm@...ck.org, Johannes Weiner <hannes@...xchg.org>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	cgroups@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] mm/memcg: recalculate chargeable space after waiting
 migrating charges

On Thu 12-07-12 18:39:21, Wanpeng Li wrote:
> From: Wanpeng Li <liwp@...ux.vnet.ibm.com>
> 
> Function mem_cgroup_do_charge will call mem_cgroup_reclaim,
> there are two break points in mem_cgroup_reclaim:
> if (total && (flag & MEM_CGROUP_RECLAIM_SHIRINK))
> 	break;
> if (mem_cgroup_margin(memcg))
> 	break;
> so mem_cgroup_reclaim can't guarantee reclaim enough pages(nr_pages) 
> which is requested from mem_cgroup_do_charge, if mem_cgroup_margin
> (mem_over_limit) >= nr_pages is not true, the process will go to
> mem_cgroup_wait_acct_move to wait doubly charge counted caused by
> task move. 

I am sorry but I have no idea what you are trying to say. The
mem_cgroup_wait_acct_move just makes sure that we are waiting until
charge is moved (which can potentially free some charges) rather than
OOM which should be the last resort so it makes sense to retry the
charge.

> But this time still can't guarantee enough pages(nr_pages) is
> ready, directly return CHARGE_RETRY is incorret. 

So you think it is better to oom? Why? What prevents you from a race
that your mem_cgroup_margin returns true but another CPU consumes those
charges right after that. See? The check is pointless. It doesn't
guarantee anything.

> We should add a check to confirm enough pages is ready, otherwise go
> to oom.

What you are trying to fix? How have you tested it? Why do you think it
makes any sense at all?
You are just changing the behavior without any rationale. And that's a
_no_go_!

> 
> Signed-off-by: Wanpeng Li <liwp.linux@...il.com>
> ---
>  mm/memcontrol.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index f72b5e5..4ae3848 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2210,7 +2210,8 @@ static int mem_cgroup_do_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
>  	 * At task move, charge accounts can be doubly counted. So, it's
>  	 * better to wait until the end of task_move if something is going on.
>  	 */
> -	if (mem_cgroup_wait_acct_move(mem_over_limit))
> +	if (mem_cgroup_wait_acct_move(mem_over_limit)
> +			&& mem_cgroup_margin(mem_over_limit) >= nr_pages)
>  		return CHARGE_RETRY;
>  
>  	/* If we don't need to call oom-killer at el, return immediately */
> -- 
> 1.7.5.4
> 

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/