linux-kernel - Re: [PATCH] cpuidle: avoid using smp_processor_id() in preemptible code (nr_iowait

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: <20100630125853.245e6b99.akpm@linux-foundation.org>
Date:	Wed, 30 Jun 2010 12:58:53 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Peter Zijlstra <peterz@...radead.org>
Cc:	Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
	Arjan van de Ven <arjan@...radead.org>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Maxim Levitsky <maximlevitsky@...il.com>,
	Len Brown <len.brown@...el.com>, Pavel Machek <pavel@....cz>,
	Jiri Slaby <jslaby@...e.cz>,
	linux-pm@...ts.linux-foundation.org, linux-kernel@...r.kernel.org,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH] cpuidle: avoid using smp_processor_id() in preemptible
 code (nr_iowait_cpu) v4

On Fri, 25 Jun 2010 16:39:33 +0200
Peter Zijlstra <peterz@...radead.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.

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