Re: [PATCH 2/2] memcg: coalescing charge by percpu (Oct/9)

[KAMEZAWA Hiroyuki]

On Tue, 13 Oct 2009 16:57:19 +0900
Daisuke Nishimura <nishimura@xxxxxxxxxxxxxxxxx> wrote:

> On Sun, 11 Oct 2009 11:37:35 +0900 (JST), "KAMEZAWA Hiroyuki" <kamezawa.hiroyu@xxxxxxxxxxxxxx> wrote:
> > Andrew Morton wrote:
> > > On Fri, 9 Oct 2009 17:01:05 +0900
> > > KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx> wrote:
> > >
> > >> +static void drain_all_stock_async(void)
> > >> +{
> > >> +	int cpu;
> > >> +	/* This function is for scheduling "drain" in asynchronous way.
> > >> +	 * The result of "drain" is not directly handled by callers. Then,
> > >> +	 * if someone is calling drain, we don't have to call drain more.
> > >> +	 * Anyway, work_pending() will catch if there is a race. We just do
> > >> +	 * loose check here.
> > >> +	 */
> > >> +	if (atomic_read(&memcg_drain_count))
> > >> +		return;
> > >> +	/* Notify other cpus that system-wide "drain" is running */
> > >> +	atomic_inc(&memcg_drain_count);
> Shouldn't we use atomic_inc_not_zero() ?
> (Do you mean this problem by "is not very good" below ?)
> 
As comment says, "we just do loose check". There is no terrible race except
for wasting cpu time.

I'm now thinking about following.

==
for_each_online_cpu(cpu) {
	struct memcg_stock_pcp *stock = &per_cpu(memcg_stock, cpu);
	if (work_pending(&stock->work))
		continue;
+       atomic_inc(&memcg_drain_count);
	INIT_WORK(&stock->work, drain_local_stock);
	schedule_work_on(cpu, &stock->work);
}
==
Or using cpumask to avoid scheduleing twice.

atomic_dec will be added to worker routine, after drain.

I'm now prepareing slides for JLS (ah, yes, deadline has gone.), so plz give me time..
If you want to review it, plz let me know.

Thanks,
-Kame


> 
> Thanks,
> Daisuke Nishimura.
> 
> > >> +	get_online_cpus();
> > >> +	for_each_online_cpu(cpu) {
> > >> +		struct memcg_stock_pcp *stock = &per_cpu(memcg_stock, cpu);
> > >> +		if (work_pending(&stock->work))
> > >> +			continue;
> > >> +		INIT_WORK(&stock->work, drain_local_stock);
> > >> +		schedule_work_on(cpu, &stock->work);
> > >> +	}
> > >> + 	put_online_cpus();
> > >> +	atomic_dec(&memcg_drain_count);
> > >> +	/* We don't wait for flush_work */
> > >> +}
> > >
> > > It's unusual to run INIT_WORK() each time we use a work_struct.
> > > Usually we will run INIT_WORK a single time, then just repeatedly use
> > > that structure.  Because after the work has completed, it is still in a
> > > ready-to-use state.
> > >
> > > Running INIT_WORK() repeatedly against the same work_struct adds a risk
> > > that we'll scribble on an in-use work_struct, which would make a big
> > > mess.
> > >
> > Ah, ok. I'll prepare a fix. (And I think atomic_dec/inc placement is not
> > very good....I'll do total review, again.)
> > 
> > Thank you for review.
> > 
> > Regards,
> > -Kame
> > 
> > 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/