[PATCH 0/8] Suspend block api (version 8)
in [Kernel]
|
Prev: [ANN] Linux Security Summit 2010 - Announcement and CFP
Next: [PATCH 4/8] PM: suspend_block: Add debugfs file
From: Thomas Gleixner on 1 Jun 2010 08:20 On Mon, 31 May 2010, Arve Hj�nnev�g wrote: > 2010/5/31 Rafael J. Wysocki <rjw(a)sisk.pl>: > > On Monday 31 May 2010, Arve Hj�nnev�g wrote: > >> 2010/5/30 Rafael J. Wysocki <rjw(a)sisk.pl>: > > ... > >> > >> I think it makes more sense to block suspend while wakeup events are > >> pending than blocking it everywhere timers are used by code that could > >> be called while handling wakeup events or other critical work. Also, > >> even if you did block suspend everywhere timers where used you still > >> have the race where a wakeup interrupt happens right after you decided > >> to suspend. In other words, you still need to block suspend in all the > >> same places as with the current opportunistic suspend code, so what is > >> the benefit of delaying suspend until idle? > > > > Assume for a while that you don't use suspend blockers, OK? �I realize you > > think that anything else doesn't make sense, but evidently some other people > > have that opinion about suspend blockers. > > > > It sounded like you were suggesting that initiating suspend from idle > would somehow avoid the race condition with wakeup events. All I'm > saying is that you would need to block suspend in all the same places. > If you don't care about ignoring wakeup events, then sure you can > initiate suspend from idle. And why should you miss a wakeup there ? If you get an interrupt in the transition, then you are not longer idle. Thanks, tglx
From: mark gross on 1 Jun 2010 09:50 On Mon, May 31, 2010 at 04:26:17PM -0700, mark gross wrote: > On Mon, May 31, 2010 at 11:38:55PM +0200, Rafael J. Wysocki wrote: > > On Monday 31 May 2010, Arve Hj�nnev�g wrote: > > > 2010/5/29 Alan Stern <stern(a)rowland.harvard.edu>: > > > > On Sat, 29 May 2010, Arve Hj�nnev�g wrote: > > > > > > > >> > In place of in-kernel suspend blockers, there will be a new type of QoS > > > >> > constraint -- call it QOS_EVENTUALLY. It's a very weak constraint, > > > >> > compatible with all cpuidle modes in which runnable threads are allowed > > > >> > to run (which is all of them), but not compatible with suspend. > > > >> > > > > >> This sound just like another API rename. It will work, but given that > > > >> suspend blockers was the name least objectionable last time around, > > > >> I'm not sure what this would solve. > > > > > > > > It's not just a rename. By changing this into a QoS constraint, we > > > > make it more generally useful. Instead of standing on its own, it > > > > becomes part of the PM-QOS framework. > > > > > > > > > > We cannot use the existing pm-qos framework. It is not safe to call > > > from atomic context. > > > > We've just merged a patch that fixed that if I'm not mistaken. Mark, did your > > PM QoS update fix that? > > > > I'm pretty sure it can be called in atomic context, and if its not I'm > sure we can fix that. It can be called in atomic context. I don't > think it was ever a problem to call it in atomic context. The problem it > had was that crappy list of string compares. Thats been fixed. > um, the notifiers that are currently used may not be cool in atomic context :( --mgross > --mgross > > > > Also, it does not have any state constraints, so it iterates over every > > > registered constraint each time one of them changes. > > > > That's fixable IMO. > > > > > Nor does is currently provide any stats for debugging. > > > > That's why Alan is proposing to add that. > > > > > The original wakelock patchset supported a wakelock type so it could > > > be used to block more then suspend, but I had to remove this because > > > it "overlapped" with pm-qos. So, yes I do consider this just another > > > rename. > > > > It's an extension of an existing framework rather than an addition of a new > > one, with entirely new API and so on. Extending existing APIs is much > > preferred to adding new ones, in general. > > > > Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Matthew Garrett on 1 Jun 2010 10:00 On Mon, May 31, 2010 at 04:21:09PM -0500, James Bottomley wrote: > You're the one mentioning x86, not me. I already explained that some > MSM hardware (the G1 for example) has lower power consumption in S3 > (which I'm using as an ACPI shorthand for suspend to ram) than any > suspend from idle C state. The fact that current x86 hardware has the > same problem may be true, but it's not entirely relevant. As long as you can set a wakeup timer, an S state is just a C state with side effects. The significant one is that entering an S state stops the process scheduler and any in-kernel timers. I don't think Google care at all about whether suspend is entered through an explicit transition or something hooked into cpuidle - the relevant issue is that they want to be able to express a set of constraints that lets them control whether or not the scheduler keeps on scheduling, and which doesn't let them lose wakeup events in the process. -- Matthew Garrett | mjg59(a)srcf.ucam.org -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: mark gross on 1 Jun 2010 10:10 On Tue, Jun 01, 2010 at 09:07:37AM +0200, Florian Mickler wrote: > On Mon, 31 May 2010 16:26:17 -0700 > mark gross <640e9920(a)gmail.com> wrote: > > > On Mon, May 31, 2010 at 11:38:55PM +0200, Rafael J. Wysocki wrote: > > > On Monday 31 May 2010, Arve Hj�nnev�g wrote: > > > > 2010/5/29 Alan Stern <stern(a)rowland.harvard.edu>: > > > > > On Sat, 29 May 2010, Arve Hj�nnev�g wrote: > > > > > > > > > >> > In place of in-kernel suspend blockers, there will be a new type of QoS > > > > >> > constraint -- call it QOS_EVENTUALLY. It's a very weak constraint, > > > > >> > compatible with all cpuidle modes in which runnable threads are allowed > > > > >> > to run (which is all of them), but not compatible with suspend. > > > > >> > > > > > >> This sound just like another API rename. It will work, but given that > > > > >> suspend blockers was the name least objectionable last time around, > > > > >> I'm not sure what this would solve. > > > > > > > > > > It's not just a rename. By changing this into a QoS constraint, we > > > > > make it more generally useful. Instead of standing on its own, it > > > > > becomes part of the PM-QOS framework. > > > > > > > > > > > > > We cannot use the existing pm-qos framework. It is not safe to call > > > > from atomic context. > > > > > > We've just merged a patch that fixed that if I'm not mistaken. Mark, did your > > > PM QoS update fix that? > > > > > > > I'm pretty sure it can be called in atomic context, and if its not I'm > > sure we can fix that. It can be called in atomic context. I don't > > think it was ever a problem to call it in atomic context. The problem it > > had was that crappy list of string compares. Thats been fixed. > > > > --mgross > > > > Well, the register call uses kzalloc. Apart from that I > think we're good. > > The outstanding list traversals can be fixed also. (see below) > > Cheers, > Flo > > From 66fdd76f8cc4be722dba3859ddadfe07e7a4b755 Mon Sep 17 00:00:00 2001 > From: Florian Mickler <florian(a)mickler.org> > Date: Tue, 1 Jun 2010 09:04:26 +0200 > Subject: [PATCH] pm_qos: remove unnecessary list-traversal > > The new extreme_value is only depending on the old extreme_value and > the changing value. > > Signed-off-by: Florian Mickler <florian(a)mickler.org> > --- > kernel/pm_qos_params.c | 20 ++++++++++++++------ > 1 files changed, 14 insertions(+), 6 deletions(-) > > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c > index f42d3f7..6618e2c 100644 > --- a/kernel/pm_qos_params.c > +++ b/kernel/pm_qos_params.c > @@ -136,6 +136,16 @@ static s32 min_compare(s32 v1, s32 v2) > } > > > +static void update_target_val(int pm_qos_class, s32 val) > +{ > + s32 extreme_value; > + s32 new_value; > + extreme_value = atomic_read(&pm_qos_array[pm_qos_class]->target_value); > + new_value = pm_qos_array[pm_qos_class]->comparitor(val,extreme_value); > + if (extreme_value != new_value) > + atomic_set(&pm_qos_array[pm_qos_class]->target_value,new_value); > +} > + Only works 1/2 the time, but I like the idea! It fails to get the righ answer when constraints are reduced. But, this idea is a good improvement i'll roll into the next pm_qos update! thanks! --mgross > static void update_target(int pm_qos_class) > { > s32 extreme_value; > @@ -227,8 +237,8 @@ struct pm_qos_request_list *pm_qos_add_request(int pm_qos_class, s32 value) > spin_lock_irqsave(&pm_qos_lock, flags); > list_add(&dep->list, > &pm_qos_array[pm_qos_class]->requests.list); > + update_target_val(pm_qos_class,dep->value); > spin_unlock_irqrestore(&pm_qos_lock, flags); > - update_target(pm_qos_class); > } > > return dep; > @@ -249,23 +259,21 @@ void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req, > s32 new_value) > { > unsigned long flags; > - int pending_update = 0; > s32 temp; > > if (pm_qos_req) { /*guard against callers passing in null */ > + int target = pm_qos_req->pm_qos_class; > spin_lock_irqsave(&pm_qos_lock, flags); > if (new_value == PM_QOS_DEFAULT_VALUE) > - temp = pm_qos_array[pm_qos_req->pm_qos_class]->default_value; > + temp = pm_qos_array[target]->default_value; > else > temp = new_value; > > if (temp != pm_qos_req->value) { > - pending_update = 1; > pm_qos_req->value = temp; > + update_target_val(target,temp); > } > spin_unlock_irqrestore(&pm_qos_lock, flags); > - if (pending_update) > - update_target(pm_qos_req->pm_qos_class); > } > } > EXPORT_SYMBOL_GPL(pm_qos_update_request); > -- > 1.7.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Alan Stern on 1 Jun 2010 10:50
On Tue, 1 Jun 2010, Neil Brown wrote: > > With wakeup events the problem isn't so bad. Wakeup events are always > > noticed, and if the system is designed properly they will either abort > > a suspend-in-progress or else cause the system to resume as soon as the > > suspend is complete. (Linux is not yet properly designed in this > > sense.) > > This is exactly the issue. Linux doesn't get this right. A wakeup that is > processed by the driver just before suspend may not get all the way out to > it's final destination (possibly in meat-space) until after some other event > wakes the device. It is not possible to guarantee full delivery of a wakeup > event with Linux in its current state. This is a bug. That wasn't what I meant. An important requirement is that a wakeup event received after the device has been suspended, but before the system suspend is complete, should cause the system suspend to abort. This is one of the things suspend blockers were meant to help with. In theory it should already work correctly -- but it doesn't. That's a real bug. The other things you're talking about aren't really bugs. That is, they are things which the kernel _ought_ to do but it wasn't _intended_ to do. They are misfeatures: failures of design, not failures of execution. > I don't think I'm missing that. Wakeup events are the only events of > interest. But wakeup events are more than just low-level hardware events. > They are also the virtual events that are a direct consequence of the > original hardware event. This is a matter of semantics and it is highly debatable. > A UART asserts 'data ready' which is routed to an IO interrupt line that > wakes the device. This is a wakeup event. Yes. > So are the bits that appear on the data line which represent the data > So are the characters that appear on the serial port > So is the "here is a message from the GSM processor" event > So is the "there is a new TXT message" event > So is the "'beep beep beep' out the speaker" event None of those things can cause the system to wake up after it is suspended. Indeed, they can't happen at all if the CPU isn't running (unless the GSM processor runs autonomously -- let's ignore such details). Hence it makes little sense to call them "wakeup" events. > All these events are wakeup events, and should abort or instant-resume the > system. Currently only the first can be guaranteed to do this (... and maybe > the second, depending on the details of the implementation). > suspend-blocks effectively pass a lock from one event to the next to ensure > that all of these event block the suspend. There are various other ways to > achieve the same thing, and I think it can be done with no significant change > to the API. But some how we need to allow all of these events to be > linked wake-up events, not just the first one. That's not entirely clear. Yes, for Android's use case that's what you want. But in other situations maybe you don't. Consider the example of someone who closes the lid of their laptop and throws it in a backpack. The computer should suspend immediately; it shouldn't be delayed by these "virtual wakeup" events. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |