On Thu, 25 Sep 2008 17:25:44 +0800
Yi Yang <[email protected]> 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 <[email protected]>
> ---
> 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?
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
On Thu, 2008-10-02 at 16:56 -0700, Andrew Morton wrote:
> On Thu, 25 Sep 2008 17:25:44 +0800
> Yi Yang <[email protected]> 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 <[email protected]>
> > ---
> > 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?
>
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 <[email protected]> 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 <[email protected]>
> > ---
> > 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?
>
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