[PATCH] USB: improve ehci_watchdog's side effect in CPU power management

[PATCH] USB: improve ehci_watchdog's side effect in CPU power management

[Yi Yang]

ehci_watchdog will wake up CPU very frequently so that CPU
stays at C3 very short, average residence time is about 50
ms on Aspire One, but we expect it should be about 1 second
or more, so this kind of periodic timer is very bad for power
saving.

We can't remove this timer because of some bad USB controller
chipset, but at least we should reduce its side effect to as
possible as low.

This patch can make CPU stay at C3 longer, average residence time
is about twice as long as original. 

Please consider to apply it, thanks

Signed-off-by: Yi Yang <yi.y.yang@intel.com>
---
 ehci.h |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 5799298..9d530d9 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -181,14 +181,16 @@ timer_action (struct ehci_hcd *ehci, enum ehci_timer_action action)
 	 * the async ring; just the I/O watchdog.  Note that if a
 	 * SHRINK were pending, OFF would never be requested.
 	 */
-	if (timer_pending(&ehci->watchdog)
-			&& ((BIT(TIMER_ASYNC_SHRINK) | BIT(TIMER_ASYNC_OFF))
-				& ehci->actions))
-		return;
+	enum ehci_timer_action oldactions = ehci->actions;
 
 	if (!test_and_set_bit (action, &ehci->actions)) {
 		unsigned long t;
 
+		if (timer_pending(&ehci->watchdog)
+			&& ((BIT(TIMER_ASYNC_SHRINK) | BIT(TIMER_ASYNC_OFF))
+				& oldactions))
+			return;
+
 		switch (action) {
 		case TIMER_IO_WATCHDOG:
 			t = EHCI_IO_JIFFIES;
@@ -204,7 +206,7 @@ timer_action (struct ehci_hcd *ehci, enum ehci_timer_action action)
 			t = DIV_ROUND_UP(EHCI_SHRINK_FRAMES * HZ, 1000) + 1;
 			break;
 		}
-		mod_timer(&ehci->watchdog, t + jiffies);
+		mod_timer(&ehci->watchdog, round_jiffies(t + jiffies));
 	}
 }
 

Re: [PATCH] USB: improve ehci_watchdog's side effect in CPU power management

[Andrew Morton]

On Thu, 25 Sep 2008 17:25:44 +0800
Yi Yang <yi.y.yang@intel.com> wrote:

> ehci_watchdog will wake up CPU very frequently so that CPU
> stays at C3 very short, average residence time is about 50
> ms on Aspire One, but we expect it should be about 1 second
> or more, so this kind of periodic timer is very bad for power
> saving.
> 
> We can't remove this timer because of some bad USB controller
> chipset, but at least we should reduce its side effect to as
> possible as low.
> 
> This patch can make CPU stay at C3 longer, average residence time
> is about twice as long as original. 
> 
> Please consider to apply it, thanks
> 
> Signed-off-by: Yi Yang <yi.y.yang@intel.com>
> ---
>  ehci.h |   12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
> index 5799298..9d530d9 100644
> --- a/drivers/usb/host/ehci.h
> +++ b/drivers/usb/host/ehci.h
> @@ -181,14 +181,16 @@ timer_action (struct ehci_hcd *ehci, enum ehci_timer_action action)
>  	 * the async ring; just the I/O watchdog.  Note that if a
>  	 * SHRINK were pending, OFF would never be requested.
>  	 */
> -	if (timer_pending(&ehci->watchdog)
> -			&& ((BIT(TIMER_ASYNC_SHRINK) | BIT(TIMER_ASYNC_OFF))
> -				& ehci->actions))
> -		return;
> +	enum ehci_timer_action oldactions = ehci->actions;
>  
>  	if (!test_and_set_bit (action, &ehci->actions)) {
>  		unsigned long t;
>  
> +		if (timer_pending(&ehci->watchdog)
> +			&& ((BIT(TIMER_ASYNC_SHRINK) | BIT(TIMER_ASYNC_OFF))
> +				& oldactions))
> +			return;
> +
>  		switch (action) {
>  		case TIMER_IO_WATCHDOG:
>  			t = EHCI_IO_JIFFIES;
> @@ -204,7 +206,7 @@ timer_action (struct ehci_hcd *ehci, enum ehci_timer_action action)
>  			t = DIV_ROUND_UP(EHCI_SHRINK_FRAMES * HZ, 1000) + 1;
>  			break;
>  		}
> -		mod_timer(&ehci->watchdog, t + jiffies);
> +		mod_timer(&ehci->watchdog, round_jiffies(t + jiffies));
>  	}
>  }
>  

<looks>

<regrets it>


Why does this:

	t = DIV_ROUND_UP(EHCI_SHRINK_FRAMES * HZ, 1000) + 1;

add "1000" to a jiffies value when it doesn't know what HZ is?  It'll
be adding anywhere from one second up to ten seconds to the timeout
interval depending upon compile-time options.

I suspect s/1000/HZ/ would improve things here.  Or just delete it -
doesn't the subsequent round_jiffies() do the same thing, only better? 
This code needs help, I suspect.


Also, do we really need to inline this large function into at least
five callsites?

Re: [PATCH] USB: improve ehci_watchdog's side effect in CPU power management

[Alan Stern]

Responding just Andrew's comments, disregarding whether or not the 
patch itself is worthwhile...

On Thu, 2 Oct 2008, Andrew Morton wrote:

> <looks>
> 
> <regrets it>
> 
> 
> Why does this:
> 
> 	t = DIV_ROUND_UP(EHCI_SHRINK_FRAMES * HZ, 1000) + 1;
> 
> add "1000" to a jiffies value when it doesn't know what HZ is?  It'll
> be adding anywhere from one second up to ten seconds to the timeout
> interval depending upon compile-time options.

Look again.  The macro doesn't _add_ 1000 to a jiffies value; it 
_divides_ the value by 1000.  This is because EHCI_SHRINK_FRAMES is in 
milliseconds.

However this could be changed to

	t = msecs_to_jiffies(EHCI_SHRINK_FRAMES) + 1;

even though that would involve more runtime code.

> I suspect s/1000/HZ/ would improve things here.  Or just delete it -
> doesn't the subsequent round_jiffies() do the same thing, only better? 
> This code needs help, I suspect.

That subsequent round_jiffies() is most likely a mistake.

> Also, do we really need to inline this large function into at least
> five callsites?

I agree; this function should not be inline.

Alan Stern

Re: [PATCH] USB: improve ehci_watchdog's side effect in CPU power management

[Yi Yang]

On Thu, 2008-10-02 at 16:56 -0700, Andrew Morton wrote:
> On Thu, 25 Sep 2008 17:25:44 +0800
> Yi Yang <yi.y.yang@intel.com> wrote:
> 
> > ehci_watchdog will wake up CPU very frequently so that CPU
> > stays at C3 very short, average residence time is about 50
> > ms on Aspire One, but we expect it should be about 1 second
> > or more, so this kind of periodic timer is very bad for power
> > saving.
> > 
> > We can't remove this timer because of some bad USB controller
> > chipset, but at least we should reduce its side effect to as
> > possible as low.
> > 
> > This patch can make CPU stay at C3 longer, average residence time
> > is about twice as long as original. 
> > 
> > Please consider to apply it, thanks
> > 
> > Signed-off-by: Yi Yang <yi.y.yang@intel.com>
> > ---
> >  ehci.h |   12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
> > index 5799298..9d530d9 100644
> > --- a/drivers/usb/host/ehci.h
> > +++ b/drivers/usb/host/ehci.h
> > @@ -181,14 +181,16 @@ timer_action (struct ehci_hcd *ehci, enum ehci_timer_action action)
> >  	 * the async ring; just the I/O watchdog.  Note that if a
> >  	 * SHRINK were pending, OFF would never be requested.
> >  	 */
> > -	if (timer_pending(&ehci->watchdog)
> > -			&& ((BIT(TIMER_ASYNC_SHRINK) | BIT(TIMER_ASYNC_OFF))
> > -				& ehci->actions))
> > -		return;
> > +	enum ehci_timer_action oldactions = ehci->actions;
> >  
> >  	if (!test_and_set_bit (action, &ehci->actions)) {
> >  		unsigned long t;
> >  
> > +		if (timer_pending(&ehci->watchdog)
> > +			&& ((BIT(TIMER_ASYNC_SHRINK) | BIT(TIMER_ASYNC_OFF))
> > +				& oldactions))
> > +			return;
> > +
> >  		switch (action) {
> >  		case TIMER_IO_WATCHDOG:
> >  			t = EHCI_IO_JIFFIES;
> > @@ -204,7 +206,7 @@ timer_action (struct ehci_hcd *ehci, enum ehci_timer_action action)
> >  			t = DIV_ROUND_UP(EHCI_SHRINK_FRAMES * HZ, 1000) + 1;
> >  			break;
> >  		}
> > -		mod_timer(&ehci->watchdog, t + jiffies);
> > +		mod_timer(&ehci->watchdog, round_jiffies(t + jiffies));
> >  	}
> >  }
> >  
> 
> <looks>
> 
> <regrets it>
> 
> 
> Why does this:
> 
> 	t = DIV_ROUND_UP(EHCI_SHRINK_FRAMES * HZ, 1000) + 1;
> 
> add "1000" to a jiffies value when it doesn't know what HZ is?  It'll
> be adding anywhere from one second up to ten seconds to the timeout
> interval depending upon compile-time options.
I don't know why original author did so.

> 
> I suspect s/1000/HZ/ would improve things here.  Or just delete it -
> doesn't the subsequent round_jiffies() do the same thing, only better? 
> This code needs help, I suspect.
If 1000 is substituted with HZ, this computation is meaningless, we can
invoke EHCI_SHRINK_FRAMES directly.

I think the best way to do with this bad case is to remove such a
watchdog completely unless we detect a bad usb chipset which will lost
data frame if we don't start watchdog timer.
> 
> 
> Also, do we really need to inline this large function into at least
> five callsites?
> 

Re: [PATCH] USB: improve ehci_watchdog's side effect in CPU power management

[Yi Yang]

CC to Alan Stern and Leonid, maybe they know why "t =
DIV_ROUND_UP(EHCI_SHRINK_FRAMES * HZ, 1000) + 1".

commit b963801164618e25fbdc0cd452ce49c3628b46c8 did this change.

On Thu, 2008-10-02 at 16:56 -0700, Andrew Morton wrote:
> On Thu, 25 Sep 2008 17:25:44 +0800
> Yi Yang <yi.y.yang@intel.com> wrote:
> 
> > ehci_watchdog will wake up CPU very frequently so that CPU
> > stays at C3 very short, average residence time is about 50
> > ms on Aspire One, but we expect it should be about 1 second
> > or more, so this kind of periodic timer is very bad for power
> > saving.
> > 
> > We can't remove this timer because of some bad USB controller
> > chipset, but at least we should reduce its side effect to as
> > possible as low.
> > 
> > This patch can make CPU stay at C3 longer, average residence time
> > is about twice as long as original. 
> > 
> > Please consider to apply it, thanks
> > 
> > Signed-off-by: Yi Yang <yi.y.yang@intel.com>
> > ---
> >  ehci.h |   12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
> > index 5799298..9d530d9 100644
> > --- a/drivers/usb/host/ehci.h
> > +++ b/drivers/usb/host/ehci.h
> > @@ -181,14 +181,16 @@ timer_action (struct ehci_hcd *ehci, enum ehci_timer_action action)
> >  	 * the async ring; just the I/O watchdog.  Note that if a
> >  	 * SHRINK were pending, OFF would never be requested.
> >  	 */
> > -	if (timer_pending(&ehci->watchdog)
> > -			&& ((BIT(TIMER_ASYNC_SHRINK) | BIT(TIMER_ASYNC_OFF))
> > -				& ehci->actions))
> > -		return;
> > +	enum ehci_timer_action oldactions = ehci->actions;
> >  
> >  	if (!test_and_set_bit (action, &ehci->actions)) {
> >  		unsigned long t;
> >  
> > +		if (timer_pending(&ehci->watchdog)
> > +			&& ((BIT(TIMER_ASYNC_SHRINK) | BIT(TIMER_ASYNC_OFF))
> > +				& oldactions))
> > +			return;
> > +
> >  		switch (action) {
> >  		case TIMER_IO_WATCHDOG:
> >  			t = EHCI_IO_JIFFIES;
> > @@ -204,7 +206,7 @@ timer_action (struct ehci_hcd *ehci, enum ehci_timer_action action)
> >  			t = DIV_ROUND_UP(EHCI_SHRINK_FRAMES * HZ, 1000) + 1;
> >  			break;
> >  		}
> > -		mod_timer(&ehci->watchdog, t + jiffies);
> > +		mod_timer(&ehci->watchdog, round_jiffies(t + jiffies));
> >  	}
> >  }
> >  
> 
> <looks>
> 
> <regrets it>
> 
> 
> Why does this:
> 
> 	t = DIV_ROUND_UP(EHCI_SHRINK_FRAMES * HZ, 1000) + 1;
> 
> add "1000" to a jiffies value when it doesn't know what HZ is?  It'll
> be adding anywhere from one second up to ten seconds to the timeout
> interval depending upon compile-time options.
> 
> I suspect s/1000/HZ/ would improve things here.  Or just delete it -
> doesn't the subsequent round_jiffies() do the same thing, only better? 
> This code needs help, I suspect.
> 
> 
> Also, do we really need to inline this large function into at least
> five callsites?
> 

Re: [PATCH] USB: improve ehci_watchdog's side effect in CPU power management

[Alan Stern]

On Wed, 8 Oct 2008, Yi Yang wrote:

> CC to Alan Stern and Leonid, maybe they know why "t =
> DIV_ROUND_UP(EHCI_SHRINK_FRAMES * HZ, 1000) + 1".

The "DIV_ROUND_UP(EHCI_SHRINK_FRAMES * HZ, 1000)" converts from 
milliseconds to jiffies.  It could easily be replaced with 
msecs_to_jiffies().

The "+ 1" part is there to prevent ties, in case the conversion just
happens to be exact.

Alan Stern

Re: [PATCH] USB: improve ehci_watchdog's side effect in CPU power management

[David Brownell]

On Thursday 25 September 2008, Yi Yang wrote:
> ehci_watchdog will wake up CPU very frequently so that CPU
> stays at C3 very short, average residence time is about 50
> ms on Aspire One, but we expect it should be about 1 second
> or more, so this kind of periodic timer is very bad for power
> saving.

How did you finger this timer?  I think you don't understand
what it's really doing.

Near as I can tell, f0d781d59cb621e1795d510039df973d0f8b23fc
should just be reverted.

Alan Stern had some good comments.  Although GCC will usually
end up optimizing most of this code away, this function may now
be too large now for inlining.

 
> We can't remove this timer because of some bad USB controller
> chipset, but at least we should reduce its side effect to as
> possible as low.

That's actually a different timer ... the IAA watchdog timer
is coping with a quirk that's been observed on most VIA chips.
On sane hardware, it should never fire (since the IAA interrupt
actually happens).


> This patch can make CPU stay at C3 longer, average residence time
> is about twice as long as original. 

Did you actually measure this patch?  It looks very wrong to me.

Recall that the primary purpose of *this* timer is to reduce
the DMA load ... first by shrinking the async ring by taking
out unused QH entries, and then by disabling the async ring
entirely.

So leaving needless DMA loads active for longer, you prevent
entry to C3... contrary to what you're attempting.

To improve C3 times, you'd want to stop DMA *sooner* not later...
there's a tradeoff of course, since stopping DMA too soon will
shrink performance (and causes various hardware races to be
more common).


> Please consider to apply it, thanks
> 
> Signed-off-by: Yi Yang <yi.y.yang@intel.com>
> ---
>  ehci.h |   12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
> index 5799298..9d530d9 100644
> --- a/drivers/usb/host/ehci.h
> +++ b/drivers/usb/host/ehci.h
> @@ -181,14 +181,16 @@ timer_action (struct ehci_hcd *ehci, enum ehci_timer_action action)
>  	 * the async ring; just the I/O watchdog.  Note that if a
>  	 * SHRINK were pending, OFF would never be requested.
>  	 */
> -	if (timer_pending(&ehci->watchdog)
> -			&& ((BIT(TIMER_ASYNC_SHRINK) | BIT(TIMER_ASYNC_OFF))
> -				& ehci->actions))
> -		return;
> +	enum ehci_timer_action oldactions = ehci->actions;

Moving that test invalidates the comment desribing what it was
doing.  This change *also* looks fishy... either it's not needed,
or it was thet only change you needed (but, with comment fix).


>  
>  	if (!test_and_set_bit (action, &ehci->actions)) {
>  		unsigned long t;
>  
> +		if (timer_pending(&ehci->watchdog)
> +			&& ((BIT(TIMER_ASYNC_SHRINK) | BIT(TIMER_ASYNC_OFF))
> +				& oldactions))
> +			return;
> +
>  		switch (action) {
>  		case TIMER_IO_WATCHDOG:
>  			t = EHCI_IO_JIFFIES;
> @@ -204,7 +206,7 @@ timer_action (struct ehci_hcd *ehci, enum ehci_timer_action action)
>  			t = DIV_ROUND_UP(EHCI_SHRINK_FRAMES * HZ, 1000) + 1;
>  			break;
>  		}
> -		mod_timer(&ehci->watchdog, t + jiffies);
> +		mod_timer(&ehci->watchdog, round_jiffies(t + jiffies));

So basically, instead of having the DMA load shrink down to zero
and finally to off, in on the order of 1/20 of a second ... you
instead leave DMA active for much longer periods.

An async ring with three empty entries will be doing DMA for one,
two, three seconds ... preventing C3 all the while ... before
turning off.

Vs previous behavior where will shrink to empty and then stop
doing DMA, in under 1/10 of a second.

This round_jiffies() call is the very least that needs reverting.



>  	}
>  }
>  
> 
> 
> 

Re: [PATCH] USB: improve ehci_watchdog's side effect in CPU power management

[Yi Yang]

在 2008-11-21五的 04:11 +0800，David Brownell写道：
> On Thursday 25 September 2008, Yi Yang wrote:
> > ehci_watchdog will wake up CPU very frequently so that CPU
> > stays at C3 very short, average residence time is about 50
> > ms on Aspire One, but we expect it should be about 1 second
> > or more, so this kind of periodic timer is very bad for power
> > saving.
> 
> How did you finger this timer?  I think you don't understand
> what it's really doing.
> 
> Near as I can tell, f0d781d59cb621e1795d510039df973d0f8b23fc
> should just be reverted.
> 
> Alan Stern had some good comments.  Although GCC will usually
> end up optimizing most of this code away, this function may now
> be too large now for inlining.
> 
>  
> > We can't remove this timer because of some bad USB controller
> > chipset, but at least we should reduce its side effect to as
> > possible as low.
> 
> That's actually a different timer ... the IAA watchdog timer
> is coping with a quirk that's been observed on most VIA chips.
> On sane hardware, it should never fire (since the IAA interrupt
> actually happens).
Thank you for your very clear explanation. It does depend on USB
device's chip origin.

But on Asus EeePC, i saw another periodical timer usb_hcd_poll_rh_status
which fires every 256 milisecond, is this also doing with some
hardware's quirk?

> 
> 
> > This patch can make CPU stay at C3 longer, average residence time
> > is about twice as long as original. 
> 
> Did you actually measure this patch?  It looks very wrong to me.
Yes, i did test it, it can dramatically increase C3 residence time, but
i didn't do data transfer verification. On Acer Aspire One, there is a
webcam from Suyin Corp which maybe uses VIA USB chip, but USB Controller
is from Intel.

> 
> Recall that the primary purpose of *this* timer is to reduce
> the DMA load ... first by shrinking the async ring by taking
> out unused QH entries, and then by disabling the async ring
> entirely.
> 
> So leaving needless DMA loads active for longer, you prevent
> entry to C3... contrary to what you're attempting.
I didn't use it to transfer data because it (webcam) is idle. :-)

Really thank you for your clear comments, i'll double check it.

Anyway, i agree this patch is reverted. These timers are really what
we must concern, they are culprits resulting in short C3 residence time.
> 
> To improve C3 times, you'd want to stop DMA *sooner* not later...
> there's a tradeoff of course, since stopping DMA too soon will
> shrink performance (and causes various hardware races to be
> more common).
> 
> 
> > Please consider to apply it, thanks
> > 
> > Signed-off-by: Yi Yang <yi.y.yang@intel.com>
> > ---
> >  ehci.h |   12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
> > index 5799298..9d530d9 100644
> > --- a/drivers/usb/host/ehci.h
> > +++ b/drivers/usb/host/ehci.h
> > @@ -181,14 +181,16 @@ timer_action (struct ehci_hcd *ehci, enum ehci_timer_action action)
> >  	 * the async ring; just the I/O watchdog.  Note that if a
> >  	 * SHRINK were pending, OFF would never be requested.
> >  	 */
> > -	if (timer_pending(&ehci->watchdog)
> > -			&& ((BIT(TIMER_ASYNC_SHRINK) | BIT(TIMER_ASYNC_OFF))
> > -				& ehci->actions))
> > -		return;
> > +	enum ehci_timer_action oldactions = ehci->actions;
> 
> Moving that test invalidates the comment desribing what it was
> doing.  This change *also* looks fishy... either it's not needed,
> or it was thet only change you needed (but, with comment fix).
> 
> 
> >  
> >  	if (!test_and_set_bit (action, &ehci->actions)) {
> >  		unsigned long t;
> >  
> > +		if (timer_pending(&ehci->watchdog)
> > +			&& ((BIT(TIMER_ASYNC_SHRINK) | BIT(TIMER_ASYNC_OFF))
> > +				& oldactions))
> > +			return;
> > +
> >  		switch (action) {
> >  		case TIMER_IO_WATCHDOG:
> >  			t = EHCI_IO_JIFFIES;
> > @@ -204,7 +206,7 @@ timer_action (struct ehci_hcd *ehci, enum ehci_timer_action action)
> >  			t = DIV_ROUND_UP(EHCI_SHRINK_FRAMES * HZ, 1000) + 1;
> >  			break;
> >  		}
> > -		mod_timer(&ehci->watchdog, t + jiffies);
> > +		mod_timer(&ehci->watchdog, round_jiffies(t + jiffies));
> 
> So basically, instead of having the DMA load shrink down to zero
> and finally to off, in on the order of 1/20 of a second ... you
> instead leave DMA active for much longer periods.
> 
> An async ring with three empty entries will be doing DMA for one,
> two, three seconds ... preventing C3 all the while ... before
> turning off.
> 
> Vs previous behavior where will shrink to empty and then stop
> doing DMA, in under 1/10 of a second.
> 
> This round_jiffies() call is the very least that needs reverting.
> 
> 
> 
> >  	}
> >  }
> >  
> > 
> > 
> > 
> 
> 

Re: [PATCH] USB: improve ehci_watchdog's side effect in CPU power management

[Alan Stern]

On Fri, 21 Nov 2008, Yi Yang wrote:

> But on Asus EeePC, i saw another periodical timer usb_hcd_poll_rh_status
> which fires every 256 milisecond, is this also doing with some
> hardware's quirk?

That timer is associated mostly with UHCI controllers.  It is not 
because of a hardware quirk.  If the controller is suspended (for 
example, because no devices are attached to it) then the timer should 
stop.

Alan Stern