lkml.org 
[lkml]   [2009]   [Feb]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [RFD] Automatic suspend
    From
    On Sat, Feb 28, 2009 at 2:18 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
    > On Friday 27 February 2009, Arve Hjønnevåg wrote:
    >> On Fri, Feb 27, 2009 at 12:55 PM, Rafael J. Wysocki <rjw@sisk.pl> 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@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2009-02-28 22:59    [W:0.043 / U:91.012 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site