From: Eduardo Valentin <edubezval@gmail.com> To: Thomas Gleixner <tglx@linutronix.de> Cc: Preeti Murthy <preeti.lkml@gmail.com>, Preeti U Murthy <preeti@linux.vnet.ibm.com>, Viresh Kumar <viresh.kumar@linaro.org>, Frederic Weisbecker <fweisbec@gmail.com>, Fengguang Wu <fengguang.wu@intel.com>, Frederic Weisbecker <frederic@kernel.org>, "Pan, Jacob jun" <jacob.jun.pan@intel.com>, LKML <linux-kernel@vger.kernel.org>, LKP <lkp@01.org>, Peter Zijlstra <peterz@infradead.org>, Zhang Rui <rui.zhang@intel.com> Subject: Re: [PATCH] tick/powerclamp: Remove tick_nohz_idle abuse Date: Thu, 18 Dec 2014 10:01:12 -0400 [thread overview] Message-ID: <20141218140106.GA6323@developer> (raw) In-Reply-To: <alpine.DEB.2.11.1412181110110.17382@nanos> [-- Attachment #1: Type: text/plain, Size: 3266 bytes --] Hello Thomas, On Thu, Dec 18, 2014 at 11:51:01AM +0100, Thomas Gleixner wrote: > commit 4dbd27711cd9 "tick: export nohz tick idle symbols for module > use" was merged via the thermal tree without an explicit ack from the > relevant maintainers. > This is a shame. Rui, do you have any comments on this? > The exports are abused by the intel powerclamp driver which implements > a fake idle state from a sched FIFO task. This causes all kinds of > wreckage in the NOHZ core code which rightfully assumes that > tick_nohz_idle_enter/exit() are only called from the idle task itself. > OK. > Recent changes in the NOHZ core lead to a failure of the powerclamp > driver and now people try to hack completely broken and backwards > workarounds into the NOHZ core code. This is completely unacceptable. > I see. > The real solution is to fix the powerclamp driver by rewriting it with > a sane concept, but that's beyond the scope of this. > Do you have suggestions on what exactly is the expected rewriting or the correct sane concepts? > So the only solution for now is to remove the calls into the core NOHZ > code from the powerclamp trainwreck along with the exports. > > Fixes: d6d71ee4a14a "PM: Introduce Intel PowerClamp Driver" If I got it right, the driver is currently broken due to changes in NOHZ core. So, does this patch fix power clamp behavior ? If I got your proposal right, in the end power clamp will be still broken, but at least won't be abusing NOHZ. Is that what you are proposing? > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > diff --git a/drivers/thermal/intel_powerclamp.c b/drivers/thermal/intel_powerclamp.c > index b46c706e1cac..e98b4249187c 100644 > --- a/drivers/thermal/intel_powerclamp.c > +++ b/drivers/thermal/intel_powerclamp.c > @@ -435,7 +435,6 @@ static int clamp_thread(void *arg) > * allowed. thus jiffies are updated properly. > */ > preempt_disable(); > - tick_nohz_idle_enter(); > /* mwait until target jiffies is reached */ > while (time_before(jiffies, target_jiffies)) { > unsigned long ecx = 1; > @@ -451,7 +450,6 @@ static int clamp_thread(void *arg) > start_critical_timings(); > atomic_inc(&idle_wakeup_counter); > } > - tick_nohz_idle_exit(); > preempt_enable(); > } > del_timer_sync(&wakeup_timer); > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c > index 4d54b7540585..1363d58f07e9 100644 > --- a/kernel/time/tick-sched.c > +++ b/kernel/time/tick-sched.c > @@ -847,7 +847,6 @@ void tick_nohz_idle_enter(void) > > local_irq_enable(); > } > -EXPORT_SYMBOL_GPL(tick_nohz_idle_enter); > > /** > * tick_nohz_irq_exit - update next tick event from interrupt exit > @@ -974,7 +973,6 @@ void tick_nohz_idle_exit(void) > > local_irq_enable(); > } > -EXPORT_SYMBOL_GPL(tick_nohz_idle_exit); > > static int tick_nohz_reprogram(struct tick_sched *ts, ktime_t now) > { > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ [-- Attachment #2: Digital signature --] [-- Type: application/pgp-signature, Size: 473 bytes --]
WARNING: multiple messages have this Message-ID (diff)
From: Eduardo Valentin <edubezval@gmail.com> To: lkp@lists.01.org Subject: Re: [PATCH] tick/powerclamp: Remove tick_nohz_idle abuse Date: Thu, 18 Dec 2014 10:01:12 -0400 [thread overview] Message-ID: <20141218140106.GA6323@developer> (raw) In-Reply-To: <alpine.DEB.2.11.1412181110110.17382@nanos> [-- Attachment #1: Type: text/plain, Size: 3268 bytes --] Hello Thomas, On Thu, Dec 18, 2014 at 11:51:01AM +0100, Thomas Gleixner wrote: > commit 4dbd27711cd9 "tick: export nohz tick idle symbols for module > use" was merged via the thermal tree without an explicit ack from the > relevant maintainers. > This is a shame. Rui, do you have any comments on this? > The exports are abused by the intel powerclamp driver which implements > a fake idle state from a sched FIFO task. This causes all kinds of > wreckage in the NOHZ core code which rightfully assumes that > tick_nohz_idle_enter/exit() are only called from the idle task itself. > OK. > Recent changes in the NOHZ core lead to a failure of the powerclamp > driver and now people try to hack completely broken and backwards > workarounds into the NOHZ core code. This is completely unacceptable. > I see. > The real solution is to fix the powerclamp driver by rewriting it with > a sane concept, but that's beyond the scope of this. > Do you have suggestions on what exactly is the expected rewriting or the correct sane concepts? > So the only solution for now is to remove the calls into the core NOHZ > code from the powerclamp trainwreck along with the exports. > > Fixes: d6d71ee4a14a "PM: Introduce Intel PowerClamp Driver" If I got it right, the driver is currently broken due to changes in NOHZ core. So, does this patch fix power clamp behavior ? If I got your proposal right, in the end power clamp will be still broken, but at least won't be abusing NOHZ. Is that what you are proposing? > Signed-off-by: Thomas Gleixner <tglx@linutronix.de> > --- > diff --git a/drivers/thermal/intel_powerclamp.c b/drivers/thermal/intel_powerclamp.c > index b46c706e1cac..e98b4249187c 100644 > --- a/drivers/thermal/intel_powerclamp.c > +++ b/drivers/thermal/intel_powerclamp.c > @@ -435,7 +435,6 @@ static int clamp_thread(void *arg) > * allowed. thus jiffies are updated properly. > */ > preempt_disable(); > - tick_nohz_idle_enter(); > /* mwait until target jiffies is reached */ > while (time_before(jiffies, target_jiffies)) { > unsigned long ecx = 1; > @@ -451,7 +450,6 @@ static int clamp_thread(void *arg) > start_critical_timings(); > atomic_inc(&idle_wakeup_counter); > } > - tick_nohz_idle_exit(); > preempt_enable(); > } > del_timer_sync(&wakeup_timer); > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c > index 4d54b7540585..1363d58f07e9 100644 > --- a/kernel/time/tick-sched.c > +++ b/kernel/time/tick-sched.c > @@ -847,7 +847,6 @@ void tick_nohz_idle_enter(void) > > local_irq_enable(); > } > -EXPORT_SYMBOL_GPL(tick_nohz_idle_enter); > > /** > * tick_nohz_irq_exit - update next tick event from interrupt exit > @@ -974,7 +973,6 @@ void tick_nohz_idle_exit(void) > > local_irq_enable(); > } > -EXPORT_SYMBOL_GPL(tick_nohz_idle_exit); > > static int tick_nohz_reprogram(struct tick_sched *ts, ktime_t now) > { > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo(a)vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 473 bytes --]
next prev parent reply other threads:[~2014-12-18 14:01 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2014-12-18 10:51 [PATCH] tick/powerclamp: Remove tick_nohz_idle abuse Thomas Gleixner 2014-12-18 10:51 ` Thomas Gleixner 2014-12-18 14:01 ` Eduardo Valentin [this message] 2014-12-18 14:01 ` Eduardo Valentin 2014-12-18 14:43 ` Thomas Gleixner 2014-12-18 14:43 ` Thomas Gleixner 2014-12-18 17:28 ` Preeti U Murthy 2014-12-18 17:28 ` Preeti U Murthy [not found] ` <DD415AA12F8FF042BF4EA69DF123C1478AF91730@ORSMSX101.amr.corp.intel.com> 2014-12-18 19:52 ` Jacob Pan 2014-12-18 19:52 ` Jacob Pan 2014-12-18 21:12 ` Thomas Gleixner 2014-12-18 21:12 ` Thomas Gleixner 2014-12-19 18:39 ` Jacob Pan 2014-12-19 18:39 ` Jacob Pan 2014-12-19 19:56 ` Thomas Gleixner 2014-12-19 19:56 ` Thomas Gleixner 2014-12-20 1:31 ` Preeti U Murthy 2014-12-20 1:31 ` Preeti U Murthy 2014-12-23 2:57 ` Jacob Pan 2014-12-23 2:57 ` Jacob Pan 2014-12-31 5:04 ` Preeti U Murthy 2014-12-31 5:04 ` Preeti U Murthy 2014-12-19 13:09 ` [tip:timers/urgent] " tip-bot for Thomas Gleixner 2014-12-19 13:09 ` tip-bot for Thomas Gleixner
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20141218140106.GA6323@developer \ --to=edubezval@gmail.com \ --cc=fengguang.wu@intel.com \ --cc=frederic@kernel.org \ --cc=fweisbec@gmail.com \ --cc=jacob.jun.pan@intel.com \ --cc=linux-kernel@vger.kernel.org \ --cc=lkp@01.org \ --cc=peterz@infradead.org \ --cc=preeti.lkml@gmail.com \ --cc=preeti@linux.vnet.ibm.com \ --cc=rui.zhang@intel.com \ --cc=tglx@linutronix.de \ --cc=viresh.kumar@linaro.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.