Re: [PATCH] cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait_cpu) v4

[Sergey Senozhatsky <sergey.senozhatsky@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3366 bytes --]

On (06/30/10 12:58), Andrew Morton wrote:
> Subject: Re: [PATCH] cpuidle: avoid using smp_processor_id() in
>  preemptible
>  code (nr_iowait_cpu) v4
> X-Mailer: Sylpheed 2.4.8 (GTK+ 2.12.9; x86_64-pc-linux-gnu)
> 
> On Fri, 25 Jun 2010 16:39:33 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Thu, 2010-06-17 at 09:29 +0300, Sergey Senozhatsky wrote:
> > > Fix
> > >    
> > >  BUG: using smp_processor_id() in preemptible [00000000] code: s2disk/3392
> > 
> > > The initial fix was to use get_cpu/put_cpu in nr_iowait_cpu.  However,
> > > Arjan stated that "the bug is that it needs to be nr_iowait_cpu(int cpu)".
> > > 
> > > This patch introduces nr_iowait_cpu(int cpu) and changes to its callers.
> > > 
> > > Arjan also pointed out that we can't use get_cpu/put_cpu in update_ts_time_stats
> > > since we "pick the current cpu, rather than the one denoted by ts" in that case.
> > > To match given *ts and cpu denoted by *ts we use new field in the struct tick_sched: int cpu.
> > 
> > 
> > > diff --git a/include/linux/tick.h b/include/linux/tick.h
> > > index b232ccc..db14691 100644
> > > --- a/include/linux/tick.h
> > > +++ b/include/linux/tick.h
> > > @@ -51,6 +51,7 @@ struct tick_sched {
> > >  	unsigned long			check_clocks;
> > >  	enum tick_nohz_mode		nohz_mode;
> > >  	ktime_t				idle_tick;
> > > +	int				cpu;
> > >  	int				inidle;
> > >  	int				tick_stopped;
> > >  	unsigned long			idle_jiffies;
> > 
> > > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> > > index 1d7b9bc..1907037 100644
> > > --- a/kernel/time/tick-sched.c
> > > +++ b/kernel/time/tick-sched.c
> > > @@ -38,6 +38,9 @@ static ktime_t last_jiffies_update;
> > >  
> > >  struct tick_sched *tick_get_tick_sched(int cpu)
> > >  {
> > > +	/*FIXME: Arjan van de Ven:
> > > +	 can we do this bit once, when the ts structure gets initialized?*/
> > > +	per_cpu(tick_cpu_sched, cpu).cpu = cpu;
> > >  	return &per_cpu(tick_cpu_sched, cpu);
> > >  }
> > 
> > > @@ -161,7 +164,7 @@ update_ts_time_stats(struct tick_sched *ts, ktime_t now, u64 *last_update_time)
> > >  	if (ts->idle_active) {
> > >  		delta = ktime_sub(now, ts->idle_entrytime);
> > >  		ts->idle_sleeptime = ktime_add(ts->idle_sleeptime, delta);
> > > -		if (nr_iowait_cpu() > 0)
> > > +		if (nr_iowait_cpu(ts->cpu) > 0)
> > >  			ts->iowait_sleeptime = ktime_add(ts->iowait_sleeptime, delta);
> > >  		ts->idle_entrytime = now;
> > >  	}
> > 
> > 
> > This all seems extremely silly, why not something like:
> 
> Does it work?
> 
> c'mon guys, it's taking us weeks and weeks to fix one simple bug.  It's
> a regression!  We should be in panic mode.
> 

Hello,

Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote:                                                                                                                                  
>> Well, there is something I'm missing. How can I match given *ts and
>> cpu in update_ts_time_stats (except for introducing
>> update_ts_time_stats(..., int cpu)) ?

Arjan van de Ven <arjan@linux.intel.com> wrote:
>that'd be option one
>option two is to add a "cpu" member to struct tick_sched.....

So, it's been discussed. I chose option #2 however and made a mistake.
Personally I prefer Peter's patch. I need some time to test it.


	Sergey

[-- Attachment #2: Type: application/pgp-signature, Size: 316 bytes --]