Re: [PATCH 3/3] USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled

2013-08-20 Thread Ming Lei 
On Mon, Aug 19, 2013 at 10:33 PM, Greg Kroah-Hartman
gre...@linuxfoundation.org wrote:

 Why would those events not need to be handled?

For IAA_WATCHDOG event, it needn't to handle when
IAA interrupt comes.

For START_UNLINK_INTR event, we don't need to unlink intr
qh any more if intr urb submit request comes.

 What does this help with?  Measurements please.

The patch may avoid unnecessary CPU wakeups caused by
timer expiration, and measurement is provided below.

On Mon, Aug 19, 2013 at 11:30 PM, Alan Stern st...@rowland.harvard.edu wrote:
 On Mon, 19 Aug 2013, Ming Lei wrote:

 This patch introduces ehci_disable_event(), which is applied on
 IAA_WATCHDOG and START_UNLINK_INTR events in case that the two
 events needn't to be handled, so that we may avoid unnecessary CPU
 wakeup.

 @@ -100,6 +100,20 @@ static void ehci_enable_event(struct ehci_hcd *ehci, 
 unsigned event,
  }



 Only one blank line here, please.

OK.


 +/* Warning: don't call this function from hrtimer handler context */
 +static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event)
 +{
 + ehci-enabled_hrtimer_events = ~(1  event);
 + if (!ehci-enabled_hrtimer_events) {
 + ehci-next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
 + hrtimer_cancel(ehci-hrtimer);
 + } else if (ehci-next_hrtimer_event == event) {
 + ehci-next_hrtimer_event =
 + ffs(ehci-enabled_hrtimer_events) - 1;

 Should the timer be rescheduled here?  It's hard to say without seeing
 some test results.

You are right, the timer should be rescheduled here, otherwise there is no
effect on HCD with need_io_watchdog set.

With below change on ehci_disable_event(), about 7~8 times timer
expiration can be saved when one asix network NIC is connected
to EHCI without network traffic(the device responds interrupt poll
7~8 times per second constantly for link status query), no matter
ehci-need_io_watchdog is set or not.

BTW, the timer expiration event is observed via /proc/timer_stats.

/* Warning: don't call this function from hrtimer handler context */
static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event)
{
ehci-enabled_hrtimer_events = ~(1  event);

if (!ehci-enabled_hrtimer_events) {
ehci-next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
hrtimer_cancel(ehci-hrtimer);
} else if (ehci-next_hrtimer_event == event) {
ehci-next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
enum ehci_hrtimer_event next =
ffs(ehci-enabled_hrtimer_events) - 1;

ehci_enable_event(ehci, next, 0);
}
}


Thanks,
--
Ming Lei
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled

2013-08-20 Thread Alan Stern 
On Tue, 20 Aug 2013, Ming Lei wrote:

 On Mon, Aug 19, 2013 at 10:33 PM, Greg Kroah-Hartman
 gre...@linuxfoundation.org wrote:
 
  Why would those events not need to be handled?
 
 For IAA_WATCHDOG event, it needn't to handle when
 IAA interrupt comes.
 
 For START_UNLINK_INTR event, we don't need to unlink intr
 qh any more if intr urb submit request comes.

This could be explained more clearly.  The events mentioned in the
patch description aren't real events at all; they are timeouts.  The
actual events are the IAA interrupt and URB submission.  The idea of
the patch is that once the actual event occurs, the corresponding
timeout isn't needed any more and so the timer can be cancelled.

  +/* Warning: don't call this function from hrtimer handler context */
  +static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event)
  +{
  + ehci-enabled_hrtimer_events = ~(1  event);
  + if (!ehci-enabled_hrtimer_events) {
  + ehci-next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
  + hrtimer_cancel(ehci-hrtimer);
  + } else if (ehci-next_hrtimer_event == event) {
  + ehci-next_hrtimer_event =
  + ffs(ehci-enabled_hrtimer_events) - 1;
 
  Should the timer be rescheduled here?  It's hard to say without seeing
  some test results.
 
 You are right, the timer should be rescheduled here, otherwise there is no
 effect on HCD with need_io_watchdog set.
 
 With below change on ehci_disable_event(), about 7~8 times timer
 expiration can be saved when one asix network NIC is connected
 to EHCI without network traffic(the device responds interrupt poll
 7~8 times per second constantly for link status query), no matter
 ehci-need_io_watchdog is set or not.
 
 BTW, the timer expiration event is observed via /proc/timer_stats.
 
 /* Warning: don't call this function from hrtimer handler context */
 static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event)
 {
 ehci-enabled_hrtimer_events = ~(1  event);
 
 if (!ehci-enabled_hrtimer_events) {
 ehci-next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
 hrtimer_cancel(ehci-hrtimer);
 } else if (ehci-next_hrtimer_event == event) {
 ehci-next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
 enum ehci_hrtimer_event next =
 ffs(ehci-enabled_hrtimer_events) - 1;

Don't declare new variables in the middle of a block.

 ehci_enable_event(ehci, next, 0);

The third argument should be false, not 0.  It is a bool.

Alan Stern

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled

2013-08-19 Thread Ming Lei 
This patch introduces ehci_disable_event(), which is applied on
IAA_WATCHDOG and START_UNLINK_INTR events in case that the two
events needn't to be handled, so that we may avoid unnecessary CPU
wakeup.

Cc: Alan Stern st...@rowland.harvard.edu
Signed-off-by: Ming Lei ming@canonical.com
---
 drivers/usb/host/ehci-hcd.c   |   12 +---
 drivers/usb/host/ehci-sched.c |4 +++-
 drivers/usb/host/ehci-timer.c |   14 ++
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 73c7299..549da36 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -738,17 +738,7 @@ static irqreturn_t ehci_irq (struct usb_hcd *hcd)
if (status  STS_IAA) {
 
/* Turn off the IAA watchdog */
-   ehci-enabled_hrtimer_events = ~BIT(EHCI_HRTIMER_IAA_WATCHDOG);
-
-   /*
-* Mild optimization: Allow another IAAD to reset the
-* hrtimer, if one occurs before the next expiration.
-* In theory we could always cancel the hrtimer, but
-* tests show that about half the time it will be reset
-* for some other event anyway.
-*/
-   if (ehci-next_hrtimer_event == EHCI_HRTIMER_IAA_WATCHDOG)
-   ++ehci-next_hrtimer_event;
+   ehci_disable_event(ehci, EHCI_HRTIMER_IAA_WATCHDOG);
 
/* guard against (alleged) silicon errata */
if (cmd  CMD_IAAD)
diff --git a/drivers/usb/host/ehci-sched.c b/drivers/usb/host/ehci-sched.c
index 6631089..83be03f 100644
--- a/drivers/usb/host/ehci-sched.c
+++ b/drivers/usb/host/ehci-sched.c
@@ -610,9 +610,11 @@ static void cancel_unlink_wait_intr(struct ehci_hcd *ehci, 
struct ehci_qh *qh)
list_del_init(qh-unlink_node);
 
/*
-* TODO: disable the event of EHCI_HRTIMER_START_UNLINK_INTR for
+* disable the event of EHCI_HRTIMER_START_UNLINK_INTR for
 * avoiding unnecessary CPU wakeup
 */
+   if (list_empty(ehci-intr_unlink_wait))
+   ehci_disable_event(ehci, EHCI_HRTIMER_START_UNLINK_INTR);
 }
 
 static void start_unlink_intr(struct ehci_hcd *ehci, struct ehci_qh *qh)
diff --git a/drivers/usb/host/ehci-timer.c b/drivers/usb/host/ehci-timer.c
index 424ac5d..89bce50 100644
--- a/drivers/usb/host/ehci-timer.c
+++ b/drivers/usb/host/ehci-timer.c
@@ -100,6 +100,20 @@ static void ehci_enable_event(struct ehci_hcd *ehci, 
unsigned event,
 }
 
 
+/* Warning: don't call this function from hrtimer handler context */
+static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event)
+{
+   ehci-enabled_hrtimer_events = ~(1  event);
+   if (!ehci-enabled_hrtimer_events) {
+   ehci-next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
+   hrtimer_cancel(ehci-hrtimer);
+   } else if (ehci-next_hrtimer_event == event) {
+   ehci-next_hrtimer_event =
+   ffs(ehci-enabled_hrtimer_events) - 1;
+   }
+}
+
+
 /* Poll the STS_ASS status bit; see when it agrees with CMD_ASE */
 static void ehci_poll_ASS(struct ehci_hcd *ehci)
 {
-- 
1.7.9.5

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled

2013-08-19 Thread Greg Kroah-Hartman 
On Mon, Aug 19, 2013 at 07:04:20PM +0800, Ming Lei wrote:
 This patch introduces ehci_disable_event(), which is applied on
 IAA_WATCHDOG and START_UNLINK_INTR events in case that the two
 events needn't to be handled, so that we may avoid unnecessary CPU
 wakeup.

Why would those events not need to be handled?

What does this help with?  Measurements please.

greg k-h
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] USB: EHCI: disable IAA_WATCHDOG and START_UNLINK_INTR if they needn't to be handled

2013-08-19 Thread Alan Stern 
On Mon, 19 Aug 2013, Ming Lei wrote:

 This patch introduces ehci_disable_event(), which is applied on
 IAA_WATCHDOG and START_UNLINK_INTR events in case that the two
 events needn't to be handled, so that we may avoid unnecessary CPU
 wakeup.

 @@ -100,6 +100,20 @@ static void ehci_enable_event(struct ehci_hcd *ehci, 
 unsigned event,
  }
  
  

Only one blank line here, please.

 +/* Warning: don't call this function from hrtimer handler context */
 +static void ehci_disable_event(struct ehci_hcd *ehci, unsigned event)
 +{
 + ehci-enabled_hrtimer_events = ~(1  event);
 + if (!ehci-enabled_hrtimer_events) {
 + ehci-next_hrtimer_event = EHCI_HRTIMER_NO_EVENT;
 + hrtimer_cancel(ehci-hrtimer);
 + } else if (ehci-next_hrtimer_event == event) {
 + ehci-next_hrtimer_event =
 + ffs(ehci-enabled_hrtimer_events) - 1;

Should the timer be rescheduled here?  It's hard to say without seeing 
some test results.

Alan Stern

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html