LinuxLists.cc - [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG

[permalink] [raw]

Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

2016-03-21 14:41:09

[permalink] [raw]

Subject: RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

> -----Original Message-----
> From: Oliver Neukum [mailto:[email protected]]
> Sent: Monday, March 21, 2016 4:36 AM
> To: Geert Uytterhoeven
> Cc: Woojung Huh - C21699; UNGLinuxDriver; David S. Miller; Guenter Roeck;
> Rafael J. Wysocki; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef
> CONFIG_PM

Thanks for all comments.
Will look into it and submit new patch.

- Woojung

2016-03-21 14:57:50

[permalink] [raw]

Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

On Mon, 21 Mar 2016, Oliver Neukum wrote:

> On Sun, 2016-03-20 at 11:43 +0100, Geert Uytterhoeven wrote:
> > If CONFIG_PM=n:
> >
> > drivers/net/usb/lan78xx.c: In function ‘lan78xx_get_stats64’:
> > drivers/net/usb/lan78xx.c:3274: error: ‘struct dev_pm_info’ has no
> > member named ‘runtime_auto’
> >
> > If PM is disabled, the runtime_auto flag is not available, but auto
> > suspend is not enabled anyway. Hence protect the check for
> > runtime_auto
> > by #ifdef CONFIG_PM to fix this.
> >
> > Fixes: a59f8c5b048dc938 ("lan78xx: add ndo_get_stats64")
> > Reported-by: Guenter Roeck <[email protected]>
> > Signed-off-by: Geert Uytterhoeven <[email protected]>
> > ---
> > Alternatively, we can add a dev_pm_runtime_auto_is_enabled() wrapper
> > to
> > include/linux/pm.h, which always return false if CONFIG_PM is
> > disabled.
>
> That is what we do for almost everything else in include/pm_runtime.h
> So it is the best solution to add the stub.

Guenter's question about whether drivers should try to access
runtime_auto in the first place was a good one. A similar problem
arises in the block layer: When a block device has removable media, the
block layer probes at 1-second intervals looking for media changes.
This defeats autosuspend in the same way as we're talking about here.

One possible solution is to export a sysfs parameter to prevent
statistics collection (or more generally, to change the interval at
which it occurs).

But checking the runtime_auto flag is probably not a great idea. Even
if it isn't set, collecting statistics is likely to wait up a device
that otherwise would have remained suspended.

Perhaps the best solution is to collect the statistics only when the
device is not suspended or is about to suspend.

Alan Stern

2016-03-21 14:59:42

[permalink] [raw]

Subject: RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

> -----Original Message-----
> From: Guenter Roeck [mailto:[email protected]]
> Sent: Sunday, March 20, 2016 6:59 PM
> To: Geert Uytterhoeven; Woojung Huh - C21699; UNGLinuxDriver; David S.
> Miller
> Cc: Rafael J. Wysocki; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef
> CONFIG_PM
>
> Not that it matters anymore since David reverted the original patch,
> but my reason for not sending a similar patch was that I wasn't sure
> if .runtime_auto should be accessed from drivers in the first place,
> or if there is some logical problem with the code.
>

Because driver can enable/disable autosuspend by usb_enable_autosuspend() and
usb_disable_autosuspend(), it would be nice to have helper to find out autosuspend status.

The code path was to utilize autosuspend power saving, when it is enabled.

Woojung

2016-03-21 17:34:38

[permalink] [raw]

Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

On Mon, 2016-03-21 at 10:57 -0400, Alan Stern wrote:

> One possible solution is to export a sysfs parameter to prevent
> statistics collection (or more generally, to change the interval at
> which it occurs).

Surely, not performing a task can hardly be beaten in terms of power
consumption. That is not meant to be flippant, but I think these
issues are orthogonal. The question of how much to do doesn't
solve the question of doing efficiently what we do.

> But checking the runtime_auto flag is probably not a great idea. Even
> if it isn't set, collecting statistics is likely to wait up a device
> that otherwise would have remained suspended.
>
> Perhaps the best solution is to collect the statistics only when the
> device is not suspended or is about to suspend.

If we know when the next activity will come, why not pass this
information down?

Regards
Oliver

2016-03-21 18:24:12

[permalink] [raw]

Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

On Mon, 21 Mar 2016, Oliver Neukum wrote:

> On Mon, 2016-03-21 at 10:57 -0400, Alan Stern wrote:
>
> > One possible solution is to export a sysfs parameter to prevent
> > statistics collection (or more generally, to change the interval at
> > which it occurs).
>
> Surely, not performing a task can hardly be beaten in terms of power
> consumption. That is not meant to be flippant, but I think these
> issues are orthogonal. The question of how much to do doesn't
> solve the question of doing efficiently what we do.

In other words, what's the best way to collect the statistics without
interfering with runtime PM, right?

If the device is suspended, presumably we know there's nothing to
collect -- especially if we already collected the statistics at the
time the device got suspended. Hence my suggestion to avoid querying
the device while it is suspended.

But this leaves open the issue that querying the device too often will
prevent it from going into autosuspend. It seems to me that the best
way to deal with this is to make sure that the autosuspend timeout is
shorter than the interal between queries, not to make the querying
conditional on !runtime_auto.

> > But checking the runtime_auto flag is probably not a great idea. Even
> > if it isn't set, collecting statistics is likely to wait up a device
> > that otherwise would have remained suspended.
> >
> > Perhaps the best solution is to collect the statistics only when the
> > device is not suspended or is about to suspend.
>
> If we know when the next activity will come, why not pass this
> information down?

I don't follow.

Alan Stern

2016-03-21 19:18:46

[permalink] [raw]

Subject: RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

> But this leaves open the issue that querying the device too often will
> prevent it from going into autosuspend. It seems to me that the best
> way to deal with this is to make sure that the autosuspend timeout is
> shorter than the interal between queries, not to make the querying
> conditional on !runtime_auto.

Short autosuspend timeout can affect performance. For instance our experiments showed that
shorter than 10sec timeout made Ethernet performance degrade because of wakeup delays.
So, just putting shorter timeout may have some side effects.

Woojung

2016-03-21 19:23:53

[permalink] [raw]

Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

On Mon, 2016-03-21 at 14:24 -0400, Alan Stern wrote:
> On Mon, 21 Mar 2016, Oliver Neukum wrote:
>
> > On Mon, 2016-03-21 at 10:57 -0400, Alan Stern wrote:
> >
> > > One possible solution is to export a sysfs parameter to prevent
> > > statistics collection (or more generally, to change the interval at
> > > which it occurs).
> >
> > Surely, not performing a task can hardly be beaten in terms of power
> > consumption. That is not meant to be flippant, but I think these
> > issues are orthogonal. The question of how much to do doesn't
> > solve the question of doing efficiently what we do.
>
> In other words, what's the best way to collect the statistics without
> interfering with runtime PM, right?

Yes.

> If the device is suspended, presumably we know there's nothing to
> collect -- especially if we already collected the statistics at the
> time the device got suspended. Hence my suggestion to avoid querying
> the device while it is suspended.

That is perfectly alright if we just collect statistics.
As a generic mechanism it is bad. Think about the polling
for media detection.

> But this leaves open the issue that querying the device too often will
> prevent it from going into autosuspend. It seems to me that the best
> way to deal with this is to make sure that the autosuspend timeout is
> shorter than the interal between queries, not to make the querying
> conditional on !runtime_auto.
[..]
> > If we know when the next activity will come, why not pass this
> > information down?

We have an autosuspend timeout because we think that IO, if it will
come at all, is likeliest to come soon. If, however, the IO is
periodic that heuristics is false.
To save most power the driver must either decide that the interval
is too short or suspend immediately. So if we are lucky enough
to have the frequency in the kernel, we should use that information.

Regards
Oliver

2016-03-21 19:27:05

[permalink] [raw]

Subject: RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

On Mon, 21 Mar 2016 [email protected] wrote:

> > But this leaves open the issue that querying the device too often will
> > prevent it from going into autosuspend. It seems to me that the best
> > way to deal with this is to make sure that the autosuspend timeout is
> > shorter than the interal between queries, not to make the querying
> > conditional on !runtime_auto.
>
> Short autosuspend timeout can affect performance. For instance our experiments showed that
> shorter than 10sec timeout made Ethernet performance degrade because of wakeup delays.
> So, just putting shorter timeout may have some side effects.

Sure. This just means that you need a long statistics interval --
longer than the autosuspend timeout. That's why I suggested making the
interval adjustable.

Alternatively, you could automatically set the statistics interval to
the autosuspend timeout (or 0 if autosuspend is disabled) plus some
fixed value.

Alan Stern

2016-03-21 19:30:59

[permalink] [raw]

Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

On Mon, 21 Mar 2016, Oliver Neukum wrote:

> On Mon, 2016-03-21 at 14:24 -0400, Alan Stern wrote:
> > On Mon, 21 Mar 2016, Oliver Neukum wrote:
> >
> > > On Mon, 2016-03-21 at 10:57 -0400, Alan Stern wrote:
> > >
> > > > One possible solution is to export a sysfs parameter to prevent
> > > > statistics collection (or more generally, to change the interval at
> > > > which it occurs).
> > >
> > > Surely, not performing a task can hardly be beaten in terms of power
> > > consumption. That is not meant to be flippant, but I think these
> > > issues are orthogonal. The question of how much to do doesn't
> > > solve the question of doing efficiently what we do.
> >
> > In other words, what's the best way to collect the statistics without
> > interfering with runtime PM, right?
>
> Yes.
>
> > If the device is suspended, presumably we know there's nothing to
> > collect -- especially if we already collected the statistics at the
> > time the device got suspended. Hence my suggestion to avoid querying
> > the device while it is suspended.
>
> That is perfectly alright if we just collect statistics.
> As a generic mechanism it is bad. Think about the polling
> for media detection.

True. Here I'm talking specifically about collecting statistics.
Media detection has its own requirements.

> > But this leaves open the issue that querying the device too often will
> > prevent it from going into autosuspend. It seems to me that the best
> > way to deal with this is to make sure that the autosuspend timeout is
> > shorter than the interal between queries, not to make the querying
> > conditional on !runtime_auto.
> [..]
> > > If we know when the next activity will come, why not pass this
> > > information down?
>
> We have an autosuspend timeout because we think that IO, if it will
> come at all, is likeliest to come soon. If, however, the IO is
> periodic that heuristics is false.
> To save most power the driver must either decide that the interval
> is too short or suspend immediately. So if we are lucky enough
> to have the frequency in the kernel, we should use that information.

The autosuspend timeout is set by userspace. The kernel may assign a
default value, but the user can always override it. Given this, I
don't see how the kernel can use frequency information (and I'm not
sure where that information would come from in the first place).

Alan Stern

2016-03-21 20:09:18

[permalink] [raw]

Subject: RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

> > > But this leaves open the issue that querying the device too often will
> > > prevent it from going into autosuspend. It seems to me that the best
> > > way to deal with this is to make sure that the autosuspend timeout is
> > > shorter than the interal between queries, not to make the querying
> > > conditional on !runtime_auto.
> >
> > Short autosuspend timeout can affect performance. For instance our
> experiments showed that
> > shorter than 10sec timeout made Ethernet performance degrade because
> of wakeup delays.
> > So, just putting shorter timeout may have some side effects.
>
> Sure. This just means that you need a long statistics interval --
> longer than the autosuspend timeout. That's why I suggested making the
> interval adjustable.

What do you mean statistics interval?
Interval calling ndo_get_stats64 or another thread/timer or else getting statistics?

Thanks.
Woojung

2016-03-21 21:02:38

[permalink] [raw]

Subject: RE: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

On Mon, 21 Mar 2016 [email protected] wrote:

> > > > But this leaves open the issue that querying the device too often will
> > > > prevent it from going into autosuspend. It seems to me that the best
> > > > way to deal with this is to make sure that the autosuspend timeout is
> > > > shorter than the interal between queries, not to make the querying
> > > > conditional on !runtime_auto.
> > >
> > > Short autosuspend timeout can affect performance. For instance our
> > experiments showed that
> > > shorter than 10sec timeout made Ethernet performance degrade because
> > of wakeup delays.
> > > So, just putting shorter timeout may have some side effects.
> >
> > Sure. This just means that you need a long statistics interval --
> > longer than the autosuspend timeout. That's why I suggested making the
> > interval adjustable.
>
> What do you mean statistics interval?
> Interval calling ndo_get_stats64 or another thread/timer or else getting statistics?

The time between calls to ndo_get_stats64, since that's the routine
which queries the device.

Alan Stern

2016-03-22 09:53:06

[permalink] [raw]

Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

On Mon, 2016-03-21 at 15:30 -0400, Alan Stern wrote:
> On Mon, 21 Mar 2016, Oliver Neukum wrote:
>

> > We have an autosuspend timeout because we think that IO, if it will
> > come at all, is likeliest to come soon. If, however, the IO is
> > periodic that heuristics is false.
> > To save most power the driver must either decide that the interval
> > is too short or suspend immediately. So if we are lucky enough
> > to have the frequency in the kernel, we should use that information.
>
> The autosuspend timeout is set by userspace. The kernel may assign a

Thus it should apply to all IO originating in user space.
But only to that IO.

> default value, but the user can always override it. Given this, I
> don't see how the kernel can use frequency information (and I'm not
> sure where that information would come from in the first place).

It can ignore internal IO for the purpose of the timeout.
If such IO is performed while the device is active, don't
alter the timer. Otherwise resume the device and look at
the provided hint and suspend again immediately if the period is long
enough.
If IO is generated periodically in the kernel, the kernel must know that
period.

Regards
Oliver

2016-03-22 14:21:55

[permalink] [raw]

Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

On Tue, 22 Mar 2016, Oliver Neukum wrote:

> On Mon, 2016-03-21 at 15:30 -0400, Alan Stern wrote:
> > On Mon, 21 Mar 2016, Oliver Neukum wrote:
> >
>
> > > We have an autosuspend timeout because we think that IO, if it will
> > > come at all, is likeliest to come soon. If, however, the IO is
> > > periodic that heuristics is false.
> > > To save most power the driver must either decide that the interval
> > > is too short or suspend immediately. So if we are lucky enough
> > > to have the frequency in the kernel, we should use that information.
> >
> > The autosuspend timeout is set by userspace. The kernel may assign a
>
> Thus it should apply to all IO originating in user space.
> But only to that IO.

Fair enough.

> > default value, but the user can always override it. Given this, I
> > don't see how the kernel can use frequency information (and I'm not
> > sure where that information would come from in the first place).
>
> It can ignore internal IO for the purpose of the timeout.
> If such IO is performed while the device is active, don't
> alter the timer.

Come to think of it, we don't. If pm_runtime_get_sync() and then
pm_runtime_put() are called while the device is already at full power,
the PM core doesn't update the last_busy time. So if the driver
doesn't update it either, the statistics collection won't interfere
with autosuspend (except when it races with the autosuspend timer
expiration).

> Otherwise resume the device and look at
> the provided hint and suspend again immediately if the period is long
> enough.

I don't see any point in resuming the device just in order to collect
operating statistics. If it was already suspended then it wasn't
operating, so there will be no statistics to collect.

> If IO is generated periodically in the kernel, the kernel must know that
> period.

Alan Stern

2016-03-22 14:31:30

[permalink] [raw]

Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

On Tue, 2016-03-22 at 10:21 -0400, Alan Stern wrote:
> I don't see any point in resuming the device just in order to collect
> operating statistics. If it was already suspended then it wasn't
> operating, so there will be no statistics to collect.

Indeed. In that case the point is moot. But it is correct to ask
the core whether the device is autosuspended at that point rather
than keep a private flag if you can.

All that is relevant only if the upper layers ask for information
that the driver cannot provide without resuming the device.
Those are fundamentally different issues.

Regards
Oliver

2016-03-22 15:13:18

[permalink] [raw]

Subject: Re: [PATCH] lan78xx: Protect runtime_auto check by #ifdef CONFIG_PM

On Tue, 22 Mar 2016, Oliver Neukum wrote:

> On Tue, 2016-03-22 at 10:21 -0400, Alan Stern wrote:
> > I don't see any point in resuming the device just in order to collect
> > operating statistics. If it was already suspended then it wasn't
> > operating, so there will be no statistics to collect.
>
> Indeed. In that case the point is moot. But it is correct to ask
> the core whether the device is autosuspended at that point rather
> than keep a private flag if you can.

That's why we have pm_runtime_status_suspended().

> All that is relevant only if the upper layers ask for information
> that the driver cannot provide without resuming the device.
> Those are fundamentally different issues.

Of course.

Alan Stern

2016-03-22 15:32:10