diff mbox

[3/3] ACPI: EC: Change EC noirq tuning to be an optional behavior

Message ID 157972fa5572b0f0c157142ea151b8508e7640b4.1497418749.git.lv.zheng@intel.com (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Lv Zheng June 14, 2017, 5:59 a.m. UTC 
According to the bug report, though the busy polling mode can make noirq
stages executed faster, it causes abnormal fan blowing in noirq stages.

This patch prepares an option so that the automatic busy polling mode
switching for noirq stages can be enabled by who wants to tune it, not all
users.
Noticed that the new global option cannot be changed during noirq stages.
There is no need to lock its value changes to sync with polling mode
settings switches.

For reporters and testers in the thread, as there are too many reporters
on the bug link, this patch only picks names from most active commenters.
Sorry for the neglet.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=191181
Reported-by: Tatsuyuki Ishi <ishitatsuyuki@gmail.com>
Reported-by: Claudio Sacerdoti Coen <claudio.sacerdoticoen@unibo.it>
Tested-by: Nicolo' <nicolopiazzalunga@gmail.com>
Reported-by: Jens Axboe <axboe@kernel.dk>
Tested-by: Gjorgji Jankovski <j.gjorgji@gmail.com>
Tested-by: Damjan Georgievski <gdamjan@gmail.com>
Tested-by: Fernando Chaves <nanochaves@gmail.com>
Signed-off-by: Lv Zheng <lv.zheng@intel.com>
---
 drivers/acpi/ec.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

Comments

Rafael J. Wysocki June 28, 2017, 9:31 p.m. UTC | #1 
On Wednesday, June 14, 2017 01:59:24 PM Lv Zheng wrote:
> According to the bug report, though the busy polling mode can make noirq
> stages executed faster, it causes abnormal fan blowing in noirq stages.
> 
> This patch prepares an option so that the automatic busy polling mode
> switching for noirq stages can be enabled by who wants to tune it, not all
> users.
> Noticed that the new global option cannot be changed during noirq stages.
> There is no need to lock its value changes to sync with polling mode
> settings switches.
> 
> For reporters and testers in the thread, as there are too many reporters
> on the bug link, this patch only picks names from most active commenters.
> Sorry for the neglet.
> 
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=191181
> Reported-by: Tatsuyuki Ishi <ishitatsuyuki@gmail.com>
> Reported-by: Claudio Sacerdoti Coen <claudio.sacerdoticoen@unibo.it>
> Tested-by: Nicolo' <nicolopiazzalunga@gmail.com>
> Reported-by: Jens Axboe <axboe@kernel.dk>
> Tested-by: Gjorgji Jankovski <j.gjorgji@gmail.com>
> Tested-by: Damjan Georgievski <gdamjan@gmail.com>
> Tested-by: Fernando Chaves <nanochaves@gmail.com>
> Signed-off-by: Lv Zheng <lv.zheng@intel.com>

First of all, this seems to be a fix for commit c3a696b6e8f8 (ACPI / EC: Use busy polling
mode when GPE is not enabled), so there should be a Fixes: tag pointing to that
one.

Moreover, if that is just a performance optimization and not a matter of correctness,
why don't we simply drop acpi_ec_enter/leave_noirq() entirely?

What is going to break if we do that?

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lv Zheng July 3, 2017, 5:15 a.m. UTC | #2 
Hi, Rafael

> From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> Subject: Re: [PATCH 3/3] ACPI: EC: Change EC noirq tuning to be an optional behavior
> 
> On Wednesday, June 14, 2017 01:59:24 PM Lv Zheng wrote:
> > According to the bug report, though the busy polling mode can make noirq
> > stages executed faster, it causes abnormal fan blowing in noirq stages.
> >
> > This patch prepares an option so that the automatic busy polling mode
> > switching for noirq stages can be enabled by who wants to tune it, not all
> > users.
> > Noticed that the new global option cannot be changed during noirq stages.
> > There is no need to lock its value changes to sync with polling mode
> > settings switches.
> >
> > For reporters and testers in the thread, as there are too many reporters
> > on the bug link, this patch only picks names from most active commenters.
> > Sorry for the neglet.
> >
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=191181
> > Reported-by: Tatsuyuki Ishi <ishitatsuyuki@gmail.com>
> > Reported-by: Claudio Sacerdoti Coen <claudio.sacerdoticoen@unibo.it>
> > Tested-by: Nicolo' <nicolopiazzalunga@gmail.com>
> > Reported-by: Jens Axboe <axboe@kernel.dk>
> > Tested-by: Gjorgji Jankovski <j.gjorgji@gmail.com>
> > Tested-by: Damjan Georgievski <gdamjan@gmail.com>
> > Tested-by: Fernando Chaves <nanochaves@gmail.com>
> > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> 
> First of all, this seems to be a fix for commit c3a696b6e8f8 (ACPI / EC: Use busy polling
> mode when GPE is not enabled), so there should be a Fixes: tag pointing to that
> one.
> 
> Moreover, if that is just a performance optimization and not a matter of correctness,
> why don't we simply drop acpi_ec_enter/leave_noirq() entirely?
> 
> What is going to break if we do that?

Let me Cc Yu for justification.
I just added busy poll support for suspend/boot according to the root cause reported by him.
He should know the end user requirements better than me.

Thanks and best regards
Lv
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Chen Yu July 6, 2017, 8:30 a.m. UTC | #3 
Hi Lv,
On Mon, Jul 03, 2017 at 01:15:26PM +0800, Zheng, Lv wrote:
> Hi, Rafael
> 
> > From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net]
> > Subject: Re: [PATCH 3/3] ACPI: EC: Change EC noirq tuning to be an optional behavior
> > 
> > On Wednesday, June 14, 2017 01:59:24 PM Lv Zheng wrote:
> > > According to the bug report, though the busy polling mode can make noirq
> > > stages executed faster, it causes abnormal fan blowing in noirq stages.
> > >
> > > This patch prepares an option so that the automatic busy polling mode
> > > switching for noirq stages can be enabled by who wants to tune it, not all
> > > users.
> > > Noticed that the new global option cannot be changed during noirq stages.
> > > There is no need to lock its value changes to sync with polling mode
> > > settings switches.
> > >
> > > For reporters and testers in the thread, as there are too many reporters
> > > on the bug link, this patch only picks names from most active commenters.
> > > Sorry for the neglet.
> > >
> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=191181
> > > Reported-by: Tatsuyuki Ishi <ishitatsuyuki@gmail.com>
> > > Reported-by: Claudio Sacerdoti Coen <claudio.sacerdoticoen@unibo.it>
> > > Tested-by: Nicolo' <nicolopiazzalunga@gmail.com>
> > > Reported-by: Jens Axboe <axboe@kernel.dk>
> > > Tested-by: Gjorgji Jankovski <j.gjorgji@gmail.com>
> > > Tested-by: Damjan Georgievski <gdamjan@gmail.com>
> > > Tested-by: Fernando Chaves <nanochaves@gmail.com>
> > > Signed-off-by: Lv Zheng <lv.zheng@intel.com>
> > 
> > First of all, this seems to be a fix for commit c3a696b6e8f8 (ACPI / EC: Use busy polling
> > mode when GPE is not enabled), so there should be a Fixes: tag pointing to that
> > one.
> > 
> > Moreover, if that is just a performance optimization and not a matter of correctness,
> > why don't we simply drop acpi_ec_enter/leave_noirq() entirely?
> > 
> > What is going to break if we do that?
> 
> Let me Cc Yu for justification.
> I just added busy poll support for suspend/boot according to the root cause reported by him.
> He should know the end user requirements better than me.
> 
I remembered the original issue was reported against the slowness during suspend/resume
due to the ec is running with GPE disabled, and falls into schedule_timeout() loop.

If the busy polling mode is controlled by the boot options rather than
acpi_ec_enter/leave_noirq(), then user should be noticed of the result
that the cpu usage might be always higer if they enable the busy polling
mode.

Thanks,
	Yu
> Thanks and best regards
> Lv
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
index f3ff591..de5dde6 100644
--- a/drivers/acpi/ec.c
+++ b/drivers/acpi/ec.c
@@ -151,6 +151,10 @@  static bool ec_freeze_events __read_mostly = true;
 module_param(ec_freeze_events, bool, 0644);
 MODULE_PARM_DESC(ec_freeze_events, "Disabling event handling during suspend/resume");
 
+static bool ec_tune_noirq __read_mostly = false;
+module_param(ec_tune_noirq, bool, 0644);
+MODULE_PARM_DESC(ec_tune_noirq, "Automatic enabling busy polling to tune noirq stages faster");
+
 struct acpi_ec_query_handler {
 	struct list_head node;
 	acpi_ec_query_func func;
@@ -979,9 +983,11 @@  static void acpi_ec_enter_noirq(struct acpi_ec *ec)
 	unsigned long flags;
 
 	spin_lock_irqsave(&ec->lock, flags);
-	ec->busy_polling = true;
-	ec->polling_guard = 0;
-	ec_log_drv("interrupt blocked");
+	if (ec_tune_noirq) {
+		ec->busy_polling = true;
+		ec->polling_guard = 0;
+		ec_log_drv("interrupt blocked");
+	}
 	spin_unlock_irqrestore(&ec->lock, flags);
 }
 
@@ -990,9 +996,11 @@  static void acpi_ec_leave_noirq(struct acpi_ec *ec)
 	unsigned long flags;
 
 	spin_lock_irqsave(&ec->lock, flags);
-	ec->busy_polling = ec_busy_polling;
-	ec->polling_guard = ec_polling_guard;
-	ec_log_drv("interrupt unblocked");
+	if (ec_tune_noirq) {
+		ec->busy_polling = ec_busy_polling;
+		ec->polling_guard = ec_polling_guard;
+		ec_log_drv("interrupt unblocked");
+	}
 	spin_unlock_irqrestore(&ec->lock, flags);
 }