From mboxrd@z Thu Jan 1 00:00:00 1970 From: Florian Mickler Subject: Re: [PATCH v3] pm_qos: make update_request callable from interrupt context Date: Wed, 9 Jun 2010 08:54:27 +0200 Message-ID: <20100609085427.3a819e35@schatten.dmk.lab> References: <1275920448.2849.31.camel@mulgrave.site> <1275924869-3811-1-git-send-email-florian@mickler.org> <1275927581.13772.10.camel@mulgrave.site> <20100608041340.GA23473@gvim.org> <1275998764.9953.1.camel@mulgrave.site> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1275998764.9953.1.camel@mulgrave.site> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-pm-bounces@lists.linux-foundation.org Errors-To: linux-pm-bounces@lists.linux-foundation.org To: James Bottomley Cc: pm list , markgross@thegnar.org List-Id: linux-pm@vger.kernel.org On Tue, 08 Jun 2010 08:06:04 -0400 James Bottomley 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.