Re: [PATCH 1/6] memcg: simplify move_account() check.

[KAMEZAWA Hiroyuki]

On Mon, 6 Feb 2012 14:38:53 -0800
Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Mon, 6 Feb 2012 19:07:59 +0900
> KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx> wrote:
> 
> > >From c75cc843ca0cb36de97ab814e59fb4ab7b1ffbd1 Mon Sep 17 00:00:00 2001
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> > 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?
> 

Yes, I made changes discussed in v2. and need to show how it looks.
I'm sorry that changelog wasn't enough.

> >
> >  }
> > +/*
> > + * 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"
> 

Sure..

> > + * 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.
> 

Ah, the reason was that mem_cgroup_end_move() can called in mem_cgroup_clear_mc().
This mem_cgroup_clear_mc() can call mem_cgroup_end_move(NULL)...
Then, this function has NULL check in callee side.
I'll add comments.


> >  /*
> >   * 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)"?
> 

        if (!mem_cgroup_stealed(memcg))
ffffffff8116e278:       85 c0                   test   %eax,%eax
ffffffff8116e27a:       74 1f                   je     ffffffff8116e29b <__mem_cgroup_begin_update_page_stat+0x7b>
                return;
ffffffff8116e29b:       5b                      pop    %rbx
ffffffff8116e29c:       41 5c                   pop    %r12
ffffffff8116e29e:       41 5d                   pop    %r13
ffffffff8116e2a0:       41 5e                   pop    %r14
ffffffff8116e2a2:       c9                      leaveq
ffffffff8116e2a3:       c3                      retq

Maybe works as expected but... I'll rewrite..how about this ?.