Re: [RFD] Automatic suspend

[Arve Hjønnevåg]

On Sat, Feb 28, 2009 at 2:18 AM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
> On Friday 27 February 2009, Arve Hjønnevåg wrote:
>> On Fri, Feb 27, 2009 at 12:55 PM, Rafael J. Wysocki <rjw@xxxxxxx> wrote:
>> > On Friday 27 February 2009, Pavel Machek wrote:
>> >> On Fri 2009-02-27 15:22:39, Rafael J. Wysocki wrote:
>> >> > On Friday 27 February 2009, Pavel Machek wrote:
>> >> > > Hi!
>> >> > >
>> >> > > > > > Then, the decision making logic will be able to use /sys/power/sleep whenever
>> >> > > > > > it wishes to and the kernel will be able to refuse to suspend if it's not
>> >> > > > > > desirable at the moment.
>> >> > > > > >
>> >> > > > > > It seems to be flexible enough to me.
>> >> > > > >
>> >> > > > > This seems flexible enough to avoid race conditions, but it forces the
>> >> > > > > user space power manager to poll when the kernel refuse suspend.
>> >> > > >
>> >> > > > And if the kernel is supposed to start automatic suspend, it has to monitor
>> >> > > > all of the wakelocks.  IMO, it's better to allow the power manager to poll the
>> >> > > > kernel if it refuses to suspend.
>> >> > >
>> >> > > polling is evil -- it keeps CPU wake up => wastes power.
>> >> > >
>> >> > > Wakelocks done right are single atomic_t... and if you set it to 0,
>> >> > > you just unblock "sleeper" thread or something. Zero polling and very
>> >> > > simple...
>> >> >
>> >> > Except that you have to check all of the wakelocks periodically in a loop =>
>> >> > polling.  So?
>> >>
>> >> No. I want to have single atomic_t for all the wakelocks... at least
>> >> in non-debug version. Debug version will be slower. I believe you
>> >> originally suggested that.
>> >
>> > I did, but please don't call it "wakelocks".  It's confusing.
>>
>> What you are talking about here is mostly an optimization of the
>> wakelock api. You have removed timeout support and made each wakelock
>> reference counted.
>
> I also removed the arbitrary number of wakelocks (I really _hate_ the name,
> can we please stop using it from now on?).

What do you mean by this? You removed the struct wake_lock?

>
>> If you ignore wakelocks with timeouts, the current
>> wakelock interface can be implemented with a global atomic_t to
>> prevent suspend, and a per wakelock atomic_t to prevent a single
>> client from changing the global reference count by more than one.
>>
>> There are a couple of reasons that I have not done this. It removes
>> the ability to easily inspect the system when it is not suspending.
>
> Care to elaborate?

If you have a single atomic_t and it is not 0, you do not know who
incremented it.

>> I do provide an option to turn off the wakelock stats, which makes
>> wake_lock/unlock significantly faster, but we never run with wakelock
>> stats off. Also, it pushes timeout handling to the drivers. I know may
>> of you don't like timeout support, but ignoring the problem is not a
>> solution. If each driver that needs timeouts uses its own timer, then
>> you will often wakeup from idle just to unlock a wakelock that will
>> not trigger suspend. This wakeup is a thousand times as costly on the
>> msm platform as a wakelock/unlock pair (with wakelock stats enabled).
>
> Well, at least a couple of people told you that the timeouts are hardly
> acceptable and they told you why.  Please stop repeating the same arguments you
> have given already for a couple of times.  They're not convincing.

And you keep ignoring them.

> Instead of trying to convince everyone to accept your solution that you're
> not willing to change, please try to listen and think how to do things
> differently so that everyone is comfortable with them.  I'm sure you're more
> than capable of doing that.

Can you summarize what the problems with my current api are? I get the
impression that you think the overhead of using a list is too high,
and that timeout support should be removed because you think all
drivers that use it are broken.

> I do realize that you consider your current solution as the best thing since
> the sliced bread, but please accept the fact that the other people think
> differently.

I certainly do not think my current solution is the best, it is very
invasive. I do however think your proposed solution is worse. The only
proposed alternative that we could actually ship a product on today is
to not use suspend at all.

>
>> I just checked my phone, and over a 24 hour awake time (370 hours
>> uptime) period, it acquired about 5 million wakelocks (mostly for
>> input events). If these were cache hits, and took as long as my
>> benchmark did, that accounts for 20 seconds of overhead (0.023% of
>> awake, 0.1% of not-idle (5.5h).
>
> Which proves what exactly?

You seem to be mainly focused on the overhead of the lock/unlock path,
so I thought some numbers would be useful.

-- 
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/