linux-kernel - Re: [PATCH 1/6] memcg: simplify move

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: <20120206143853.4cd732c4.akpm@linux-foundation.org>
Date:	Mon, 6 Feb 2012 14:38:53 -0800
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Cc:	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"hannes@...xchg.org" <hannes@...xchg.org>,
	Michal Hocko <mhocko@...e.cz>, Ying Han <yinghan@...gle.com>,
	Hugh Dickins <hughd@...gle.com>
Subject: Re: [PATCH 1/6] memcg: simplify move_account() check.

On Mon, 6 Feb 2012 19:07:59 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com> wrote:

> >From c75cc843ca0cb36de97ab814e59fb4ab7b1ffbd1 Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
> Date: Thu, 2 Feb 2012 10:02:39 +0900
> Subject: [PATCH 1/6] memcg: simplify move_account() check.
> 
> In memcg, for avoiding take-lock-irq-off at accessing page_cgroup,
> a logic, flag + rcu_read_lock(), is used. This works as following
> 
>      CPU-A                     CPU-B
>                              rcu_read_lock()
>     set flag
>                              if(flag is set)
>                                    take heavy lock
>                              do job.
>     synchronize_rcu()        rcu_read_unlock()
> 
> In recent discussion, it's argued that using per-cpu value for this
> flag just complicates the code because 'set flag' is very rare.
> 
> This patch changes 'flag' implementation from percpu to atomic_t.
> This will be much simpler.
> 

To me, "RFC" says "might not be ready for merging yet".  You're up to
v3 - why is it still RFC?  You're still expecting to make significant
changes?

>
>  }
> +/*
> + * memcg->moving_account is used for checking possibility that some thread is
> + * calling move_account(). When a thread on CPU-A starts moving pages under
> + * a memcg, other threads sholud check memcg->moving_account under

"should"

> + * rcu_read_lock(), like this:
> + *
> + *         CPU-A                                    CPU-B
> + *                                              rcu_read_lock()
> + *         memcg->moving_account+1              if (memcg->mocing_account)
> + *                                                   take havier locks.
> + *         syncronize_rcu()                     update something.
> + *                                              rcu_read_unlock()
> + *         start move here.
> + */
>  
>  static void mem_cgroup_start_move(struct mem_cgroup *memcg)
>  {
> -	int cpu;
> -
> -	get_online_cpus();
> -	spin_lock(&memcg->pcp_counter_lock);
> -	for_each_online_cpu(cpu)
> -		per_cpu(memcg->stat->count[MEM_CGROUP_ON_MOVE], cpu) += 1;
> -	memcg->nocpu_base.count[MEM_CGROUP_ON_MOVE] += 1;
> -	spin_unlock(&memcg->pcp_counter_lock);
> -	put_online_cpus();
> -
> +	atomic_inc(&memcg->moving_account);
>  	synchronize_rcu();
>  }
>  
>  static void mem_cgroup_end_move(struct mem_cgroup *memcg)
>  {
> -	int cpu;
> -
> -	if (!memcg)
> -		return;
> -	get_online_cpus();
> -	spin_lock(&memcg->pcp_counter_lock);
> -	for_each_online_cpu(cpu)
> -		per_cpu(memcg->stat->count[MEM_CGROUP_ON_MOVE], cpu) -= 1;
> -	memcg->nocpu_base.count[MEM_CGROUP_ON_MOVE] -= 1;
> -	spin_unlock(&memcg->pcp_counter_lock);
> -	put_online_cpus();
> +	if (memcg)
> +		atomic_dec(&memcg->moving_account);
>  }

It's strange that end_move handles a NULL memcg but start_move does not.

>  /*
>   * 2 routines for checking "mem" is under move_account() or not.
> @@ -1298,7 +1297,7 @@ static void mem_cgroup_end_move(struct mem_cgroup *memcg)
>  static bool mem_cgroup_stealed(struct mem_cgroup *memcg)
>  {
>  	VM_BUG_ON(!rcu_read_lock_held());
> -	return this_cpu_read(memcg->stat->count[MEM_CGROUP_ON_MOVE]) > 0;
> +	return atomic_read(&memcg->moving_account);
>  }

So a bool-returning function can return something > 1?

I don't know what the compiler would make of that.  Presumably "if (b)"
will work OK, but will "if (b1 == b2)"?

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