Re: [PATCH v3] pm_qos: make update_request callable from interrupt context

[Florian Mickler <florian@mickler.org>]

On Tue, 08 Jun 2010 08:06:04 -0400
James Bottomley <James.Bottomley@suse.de> wrote:

> On Mon, 2010-06-07 at 21:13 -0700, mark gross wrote: 
> > On Mon, Jun 07, 2010 at 12:19:41PM -0400, James Bottomley wrote:
> > > On Mon, 2010-06-07 at 17:34 +0200, florian@mickler.org wrote:
> > > > We use the spinlocked notifier chain variant (struct
> > > > atomic_notifier_head) and add an __might_sleep() to the chain for
> > > > constraints which have non-atomic notifiers. This way we catch all
> > > > interrupt-context-update-sites at runtime.
> > > 
> > > Actually, I'm afraid we can't really call blocking notifiers through the
> > > atomic chain because we might end up with a contested chain call and a
> > > huge busy wait in the spinlock (especially if one of the notifiers is
> > > sleeping).
> > > 
> > > I think the pm_qos_object still needs the two notifier chains ... it's
> > > just that when set up, one must either fill an atomic or a blocking
> > > chain (leaving the other NULL).  We use the NULL to check to decide what
> > > chain to add notifiers to, and if the blocking chain is null, we refuse
> > > to add blocking notifiers (with a BUG). If the blocking chain is
> > > non-null, we register the might_sleep() notifier (actually, given the
> > > argument mismatch, you'll have to wrapper that).
> > > 
> > > James
> 
> > Can't we just requiere that all notifier callbacks be atomic context
> > safe and not fart around with 2 classes of notifiers?
> 
> Not unless someone rewrites the network notifier: it uses mutexes and is
> clearly assuming user context.  Perhaps they could simply be replaced
> with spinlocks but someone who understands the net code would would have
> to advise on this.
> 
> James
> 

What I thought about is to introduce a "scheduled_notifier_block" for
using sleeping notifiers on atomic_notifier_chain. That notifier block
would not execute the given function but use schedule_work() to
schedule it. I will try to shake up an implementation for that. 

Either way, my preferred approach would be to just use
schedule_work in the network notifier. 
I think that would be safe: Currently there is no synchronization
between pm_qos_update_request caller and the ieee listener. So an
schedule_work would change the timing, but not introduce an race that
wasn't there before. execute_in_process_context might not be a good
idea, because the spinlock will be held in case it's called from user
context.


I will do a patch for scheduling the network notifier and present it to
the relevant people, methinks.

Cheers,
Flo

p.s.: if my assumptions don't hold and it get's shot down by the
network people, I think James' two-queues-one-used solution is
sensible.