Message-ID: <20100602180614.729246ea@notabene.brown>
Date:	Wed, 2 Jun 2010 18:06:14 +1000
From:	Neil Brown <neilb@...e.de>
To:	Arve Hjønnevåg <arve@...roid.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	"Rafael J. Wysocki" <rjw@...k.pl>,
	Alan Stern <stern@...land.harvard.edu>,
	Felipe Balbi <felipe.balbi@...ia.com>,
	Peter Zijlstra <peterz@...radead.org>,
	"Paul@...p1.linux-foundation.org" <Paul@...p1.linux-foundation.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Florian Mickler <florian@...kler.org>,
	Linux OMAP Mailing List <linux-omap@...r.kernel.org>,
	Linux PM <linux-pm@...ts.linux-foundation.org>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	James Bottomley <James.Bottomley@...e.de>
Subject: Re: [PATCH] - race-free suspend. Was: Re: [linux-pm] [PATCH 0/8] 
 Suspend block api (version 8)

On Wed, 2 Jun 2010 00:05:14 -0700
Arve Hjønnevåg <arve@...roid.com> wrote:

> On Tue, Jun 1, 2010 at 10:32 PM, Neil Brown <neilb@...e.de> wrote:
> > On Tue, 1 Jun 2010 12:50:01 +0200 (CEST)
> > Thomas Gleixner <tglx@...utronix.de> wrote:
> >
> >> On Tue, 1 Jun 2010, Neil Brown wrote:
> >> >
> >> > I think you have acknowledged that there is a race with suspend - thanks.
> >> > Next step was "can it be closed".
> >> > You seem to suggest that it can, but you describe it as a "work around"
> >> > rather than a "bug fix"...
> >> >
> >> > Do you agree that the race is a "bug", and therefore it is appropriate to
> >> > "fix" it assuming an acceptable fix can be found (which I think it can)?
> >>
> >> If we can fix it, yes we definitely should do and not work around it.
> >>
> >> Thanks,
> >>
> >>       tglx
> >
> > OK.
> > Here is my suggestion.
> >
> > While I think this patch would actually work, and hope the ugly aspects are
> > reasonably balanced by the simplicity, I present it primarily as a base for
> > improvement.
> > The important part is to present how drivers and user-space can co-operate
> > to avoid losing wake-events.  The details of what happens in the kernel are
> > certainly up for discussion (as is everything else really of course).
> >
> > The user-space suspend daemon avoids losing wake-events by using
> > fcntl(F_OWNER) to ensure it gets a signal whenever any important wake-event
> > is ready to be read by user-space.  This may involve:
> >  - the one daemon processing all wake events
> 
> Wake up events are not all processed by one daemon.

Not with your current user-space code, no.  Are you saying that you are not
open to any significant change in the Android user-space code?  That would
make the situation a lot harder to resolve.

> 
> >  - Both the suspend daemon and the main event handling daemon opening any
> >    given device that delivers wake events (this should work with input
> >    events ... unless grabbing is needed)
> 
> Not all wakeup events are broadcast like input events so they cannot
> be read by both daemons. Not that this really matters, since reading
> the event from the suspend daemon does not mean that it has been
> delivered to and processed by the other daemon.

There would still need to be some sort of communication between the the
suspend daemon on any event daemon to ensure that the events had been
processed.  This could be very light weight interaction.  The point though is
that with this patch it becomes possible to avoid races.  Possible is better
than impossible.

> 
> >  - The event handling daemon giving the suspend-daemon's pid as F_OWNER, and
> >    using poll/select to get the events itself.
> 
> I don't like the idea of using signals for this. Without the hack Alan
> Stern suggested, you will temporarily block suspend if the wakeup
> event happened before the suspend daemon thread made it to the kernel,
> but abort suspend if it happened right after.

I'm not sure why that difference matters.  But I'm also not sure that it is
true.
When any wakeup event happen, a signal will be delivered to the suspend
daemon.
This will interrupt a pending suspend request, or a sleep, or whatever else
the daemon is doing.
It can then go back to waiting for a good time to suspend, and then try to
suspend again.


> 
> >
> > When 'mem' is written to /sys/power/state, suspend_prepare waits in an
> > interruptible wait until any wake-event that might have been initiated before
> > the suspend was request, has had a chance to be queued for user-space and
> > trigger kill_fasync.
> 
> And what happens if you are not waiting when this happens?

I'm not sure I understand the question.  Could you explain it please?

Either the initial event happens late enough to abort/resume the suspend, or
the signal happens early enough to abort the suspend, or alert the daemon not
to do a suspend.  Either way we don't get stuck in suspend.


> 
> > Currently this wait is a configurable time after the last wake-event was
> > initiated.  This is hackish, but simple and probably adequate.
> 
> Waiting after a wake event is the same as suspend_block_timeout. This
> is useful when passing events through layers of code that does no
> block suspend, but we should strive to avoid it. Other people had much
> stronger objections to this, which is why it is not included in the
> last suspend blocker patchset.

Absolutely agree.  The idea of of waiting was just a simple way to present
code that actually could work.  There are doubtlessly better ways and I
assume they have been implemented in the suspend-blocker code.
We just need some way to wait for the suspend-block count to reach zero, with
some confidence that this amount of time is limited.

(though to be honest ... the incredible simplicity of waiting a little while
is very compelling.... :-))

> 
> It also does not work for drivers that need to block suspend for more
> than a few seconds. For instance the gpio keypad matrix driver needs
> to block suspend while keys are pressed so it can scan the keypad.

I cannot imagine why it would take multiple seconds to scan a keypad.
Can you explain that?

Do you mean while keys are held pressed?  Maybe you don't get a wake-up event
on key-release?  In that case your user-space daemon could block suspend
while there are any pressed keys....  confused.


Thanks for the review,

NeilBrown

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

Powered by blists - more mailing lists