[PATCH] pm_qos: make update_request callable from interrupt context

[PATCH] pm_qos: make update_request callable from interrupt context

[florian]

We introduce new atomic_notifiers and only call the (legacy) blocking
notifiers if there are any registered. A might_sleep() tries to
fence off all misuse.

This should make it possible to call update_request from interrupt-context
as long as only atomic notifiers are registered.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 include/linux/pm_qos_params.h |    3 +
 kernel/pm_qos_params.c        |  114 ++++++++++++++++++++++++++++++++--------
 2 files changed, 94 insertions(+), 23 deletions(-)

diff --git a/include/linux/pm_qos_params.h b/include/linux/pm_qos_params.h
index 8ba440e..60ed542 100644
--- a/include/linux/pm_qos_params.h
+++ b/include/linux/pm_qos_params.h
@@ -22,6 +22,9 @@ void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
 void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req);
 
 int pm_qos_request(int pm_qos_class);
+
 int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier);
 int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
 
+int pm_qos_add_notifier_nonblocking(int pm_qos_class, struct notifier_block *notifier);
+int pm_qos_remove_notifier_nonblocking(int pm_qos_class, struct notifier_block *notifier);
diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index f42d3f7..0a67997 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -63,7 +63,8 @@ static s32 min_compare(s32 v1, s32 v2);
 
 struct pm_qos_object {
 	struct pm_qos_request_list requests;
-	struct blocking_notifier_head *notifiers;
+	struct atomic_notifier_head *notifiers;
+	struct blocking_notifier_head *blocking_notifiers;
 	struct miscdevice pm_qos_power_miscdev;
 	char *name;
 	s32 default_value;
@@ -72,20 +73,24 @@ struct pm_qos_object {
 };
 
 static struct pm_qos_object null_pm_qos;
-static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
+static ATOMIC_NOTIFIER_HEAD(cpu_dma_lat_notifier);
+static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_blocking_notifier);
 static struct pm_qos_object cpu_dma_pm_qos = {
 	.requests = {LIST_HEAD_INIT(cpu_dma_pm_qos.requests.list)},
 	.notifiers = &cpu_dma_lat_notifier,
+	.blocking_notifiers = &cpu_dma_lat_blocking_notifier,
 	.name = "cpu_dma_latency",
 	.default_value = 2000 * USEC_PER_SEC,
 	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
 	.comparitor = min_compare
 };
 
-static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
+static ATOMIC_NOTIFIER_HEAD(network_lat_notifier);
+static BLOCKING_NOTIFIER_HEAD(network_lat_blocking_notifier);
 static struct pm_qos_object network_lat_pm_qos = {
 	.requests = {LIST_HEAD_INIT(network_lat_pm_qos.requests.list)},
 	.notifiers = &network_lat_notifier,
+	.blocking_notifiers = &network_lat_blocking_notifier,
 	.name = "network_latency",
 	.default_value = 2000 * USEC_PER_SEC,
 	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
@@ -93,17 +98,18 @@ static struct pm_qos_object network_lat_pm_qos = {
 };
 
 
-static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
+static ATOMIC_NOTIFIER_HEAD(network_throughput_notifier);
+static BLOCKING_NOTIFIER_HEAD(network_throughput_blocking_notifier);
 static struct pm_qos_object network_throughput_pm_qos = {
 	.requests = {LIST_HEAD_INIT(network_throughput_pm_qos.requests.list)},
 	.notifiers = &network_throughput_notifier,
+	.blocking_notifiers = &network_throughput_blocking_notifier,
 	.name = "network_throughput",
 	.default_value = 0,
 	.target_value = ATOMIC_INIT(0),
 	.comparitor = max_compare
 };
 
-
 static struct pm_qos_object *pm_qos_array[] = {
 	&null_pm_qos,
 	&cpu_dma_pm_qos,
@@ -135,35 +141,45 @@ static s32 min_compare(s32 v1, s32 v2)
 	return min(v1, v2);
 }
 
+/* these notifiers may not be cool to call from interrupt context... */
+static void blocking_update_notify(struct pm_qos_object *obj)
+{
+	s32 extreme_value;
+
+	might_sleep();
+
+	extreme_value = atomic_read(&obj->target_value);
+	blocking_notifier_call_chain(obj->blocking_notifiers,
+			(unsigned long) extreme_value, NULL);
+}
 
 static void update_target(int pm_qos_class)
 {
 	s32 extreme_value;
+	struct pm_qos_object *obj = pm_qos_array[pm_qos_class];
 	struct pm_qos_request_list *node;
 	unsigned long flags;
 	int call_notifier = 0;
 
 	spin_lock_irqsave(&pm_qos_lock, flags);
-	extreme_value = pm_qos_array[pm_qos_class]->default_value;
-	list_for_each_entry(node,
-			&pm_qos_array[pm_qos_class]->requests.list, list) {
-		extreme_value = pm_qos_array[pm_qos_class]->comparitor(
-				extreme_value, node->value);
+	extreme_value = obj->default_value;
+	list_for_each_entry(node, &obj->requests.list, list) {
+		extreme_value = obj->comparitor(extreme_value, node->value);
 	}
-	if (atomic_read(&pm_qos_array[pm_qos_class]->target_value) !=
-			extreme_value) {
+	if (atomic_read(&obj->target_value) != extreme_value) {
 		call_notifier = 1;
-		atomic_set(&pm_qos_array[pm_qos_class]->target_value,
-				extreme_value);
+		atomic_set(&obj->target_value, extreme_value);
 		pr_debug(KERN_ERR "new target for qos %d is %d\n", pm_qos_class,
-			atomic_read(&pm_qos_array[pm_qos_class]->target_value));
+			atomic_read(&obj->target_value));
 	}
 	spin_unlock_irqrestore(&pm_qos_lock, flags);
 
-	if (call_notifier)
-		blocking_notifier_call_chain(
-				pm_qos_array[pm_qos_class]->notifiers,
-					(unsigned long) extreme_value, NULL);
+	if (call_notifier) {
+		atomic_notifier_call_chain(obj->notifiers,
+				(unsigned long) extreme_value, NULL);
+		if (obj->blocking_notifiers->head != NULL)
+			blocking_update_notify(obj);
+	}
 }
 
 static int register_pm_qos_misc(struct pm_qos_object *qos)
@@ -272,6 +288,7 @@ EXPORT_SYMBOL_GPL(pm_qos_update_request);
 
 /**
  * pm_qos_remove_request - modifies an existing qos request
+ *
  * @pm_qos_req: handle to request list element
  *
  * Will remove pm qos request from the list of requests and
@@ -297,11 +314,39 @@ void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req)
 EXPORT_SYMBOL_GPL(pm_qos_remove_request);
 
 /**
+ * pm_qos_add_notifier_nonblocking - sets notification entry for changes to target value
+ *
+ * Code executed by the notifier block may not sleep!
+ *
+ * @pm_qos_class: identifies which qos target changes should be notified.
+ * @notifier: notifier block managed by caller.
+ *
+ * Will register the notifier into a notification chain that gets called
+ * upon changes to the pm_qos_class target value.
+ */
+int pm_qos_add_notifier_nonblocking(int pm_qos_class, struct notifier_block *notifier)
+{
+	int retval;
+
+	retval = atomic_notifier_chain_register(
+			pm_qos_array[pm_qos_class]->notifiers, notifier);
+
+	return retval;
+}
+EXPORT_SYMBOL_GPL(pm_qos_add_notifier_nonblocking);
+
+
+/**
  * pm_qos_add_notifier - sets notification entry for changes to target value
+ *
+ * Notifiers added with this function are executed in process context.
+ * They may also be scheduled for later execution if pm_qos_update_request is
+ * called from interrupt context.
+ *
  * @pm_qos_class: identifies which qos target changes should be notified.
  * @notifier: notifier block managed by caller.
  *
- * will register the notifier into a notification chain that gets called
+ * Will register the notifier into a notification chain that gets called
  * upon changes to the pm_qos_class target value.
  */
 int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier)
@@ -309,18 +354,40 @@ int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier)
 	int retval;
 
 	retval = blocking_notifier_chain_register(
-			pm_qos_array[pm_qos_class]->notifiers, notifier);
+			pm_qos_array[pm_qos_class]->blocking_notifiers, notifier);
 
 	return retval;
 }
 EXPORT_SYMBOL_GPL(pm_qos_add_notifier);
 
 /**
+ * pm_qos_remove_notifier_nonblocking - deletes notification entry from the
+ * nonblocking-notifier-chain.
+ *
+ * @pm_qos_class: identifies which qos target changes are notified.
+ * @notifier: notifier block to be removed.
+ *
+ * Will remove the notifier from the notification chain that gets called
+ * upon changes to the pm_qos_class target value.
+ */
+int pm_qos_remove_notifier_nonblocking(int pm_qos_class, struct notifier_block *notifier)
+{
+	int retval;
+
+	retval = atomic_notifier_chain_unregister(
+			pm_qos_array[pm_qos_class]->notifiers, notifier);
+
+	return retval;
+}
+EXPORT_SYMBOL_GPL(pm_qos_remove_notifier_nonblocking);
+
+/**
  * pm_qos_remove_notifier - deletes notification entry from chain.
+ *
  * @pm_qos_class: identifies which qos target changes are notified.
  * @notifier: notifier block to be removed.
  *
- * will remove the notifier from the notification chain that gets called
+ * Will remove the notifier from the notification chain that gets called
  * upon changes to the pm_qos_class target value.
  */
 int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
@@ -328,7 +395,7 @@ int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
 	int retval;
 
 	retval = blocking_notifier_chain_unregister(
-			pm_qos_array[pm_qos_class]->notifiers, notifier);
+			pm_qos_array[pm_qos_class]->blocking_notifiers, notifier);
 
 	return retval;
 }
@@ -381,6 +448,7 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
 	} else
 		return -EINVAL;
 
+
 	pm_qos_req = (struct pm_qos_request_list *)filp->private_data;
 	pm_qos_update_request(pm_qos_req, value);
 
-- 
1.7.1

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

[James Bottomley]

On Mon, 2010-06-07 at 14:31 +0200, florian@mickler.org wrote:
> We introduce new atomic_notifiers and only call the (legacy) blocking
> notifiers if there are any registered. A might_sleep() tries to
> fence off all misuse.
> 
> This should make it possible to call update_request from interrupt-context
> as long as only atomic notifiers are registered.
> 
> Signed-off-by: Florian Mickler <florian@mickler.org>
> ---
>  include/linux/pm_qos_params.h |    3 +
>  kernel/pm_qos_params.c        |  114 ++++++++++++++++++++++++++++++++--------
>  2 files changed, 94 insertions(+), 23 deletions(-)
> 
> diff --git a/include/linux/pm_qos_params.h b/include/linux/pm_qos_params.h
> index 8ba440e..60ed542 100644
> --- a/include/linux/pm_qos_params.h
> +++ b/include/linux/pm_qos_params.h
> @@ -22,6 +22,9 @@ void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
>  void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req);
>  
>  int pm_qos_request(int pm_qos_class);
> +
>  int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier);
>  int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
>  
> +int pm_qos_add_notifier_nonblocking(int pm_qos_class, struct notifier_block *notifier);
> +int pm_qos_remove_notifier_nonblocking(int pm_qos_class, struct notifier_block *notifier);
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index f42d3f7..0a67997 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -63,7 +63,8 @@ static s32 min_compare(s32 v1, s32 v2);
>  
>  struct pm_qos_object {
>  	struct pm_qos_request_list requests;
> -	struct blocking_notifier_head *notifiers;
> +	struct atomic_notifier_head *notifiers;
> +	struct blocking_notifier_head *blocking_notifiers;
>  	struct miscdevice pm_qos_power_miscdev;
>  	char *name;
>  	s32 default_value;
> @@ -72,20 +73,24 @@ struct pm_qos_object {
>  };
>  
>  static struct pm_qos_object null_pm_qos;
> -static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
> +static ATOMIC_NOTIFIER_HEAD(cpu_dma_lat_notifier);
> +static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_blocking_notifier);

So I think it might be better implemented by having only a single active
notifier head: either blocking or atomic  because all this depends on
where the callsites for the notifiers are, and the person adding the
notifier should know this..  We can add atomic notifiers to the blocking
chain, just not vice versa.  The idea is that if all the add and update
call sites are blocking, you just register the blocking chain and forget
the atomic one.  The only difference between atomic and blocking
notifiers is whether we use a spinlock or a mutex to guard the integrity
of the call chain ... if you know you always have user context at the
callsites, then you can always use the mutex.

Then, for blocking notifiers, I think in init, we can register a single
notifier which just calls __might_sleep() ... that will pick up at
runtime any atomic callsite.

For atomics, you just set up an atomic call chain and leave the blocking
one null.  Then we get a BUG if anyone tries to register a blocking
notifier to an atomic only pm_qos_object.

The implementation looks fine, except:

[...]
>  /**
> + * pm_qos_add_notifier_nonblocking - sets notification entry for changes to target value
> + *
> + * Code executed by the notifier block may not sleep!
> + *
> + * @pm_qos_class: identifies which qos target changes should be notified.
> + * @notifier: notifier block managed by caller.
> + *
> + * Will register the notifier into a notification chain that gets called
> + * upon changes to the pm_qos_class target value.
> + */
> +int pm_qos_add_notifier_nonblocking(int pm_qos_class, struct notifier_block *notifier)

Rightly or wrongly, the notifier people use atomic not nonblocking, so
we should really stick with it to avoid confusion.

James

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

[Alan Stern]

On Mon, 7 Jun 2010, James Bottomley wrote:

> > +int pm_qos_add_notifier_nonblocking(int pm_qos_class, struct notifier_block *notifier)
> 
> Rightly or wrongly, the notifier people use atomic not nonblocking, so
> we should really stick with it to avoid confusion.

I'm afraid that's my fault.  I used "atomic" because it's shorter and
punchier than "nonblocking".  It refers to the context in which the
notifier chain may be called.

Alan Stern

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

[Florian Mickler]

On Mon, 07 Jun 2010 09:10:40 -0400
James Bottomley <James.Bottomley@suse.de> wrote:

> > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > index f42d3f7..0a67997 100644
> > --- a/kernel/pm_qos_params.c
> > +++ b/kernel/pm_qos_params.c
> > @@ -63,7 +63,8 @@ static s32 min_compare(s32 v1, s32 v2);
> >  
> >  struct pm_qos_object {
> >  	struct pm_qos_request_list requests;
> > -	struct blocking_notifier_head *notifiers;
> > +	struct atomic_notifier_head *notifiers;
> > +	struct blocking_notifier_head *blocking_notifiers;
> >  	struct miscdevice pm_qos_power_miscdev;
> >  	char *name;
> >  	s32 default_value;
> > @@ -72,20 +73,24 @@ struct pm_qos_object {
> >  };
> >  
> >  static struct pm_qos_object null_pm_qos;
> > -static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
> > +static ATOMIC_NOTIFIER_HEAD(cpu_dma_lat_notifier);
> > +static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_blocking_notifier);
> 
> So I think it might be better implemented by having only a single active
> notifier head: either blocking or atomic  because all this depends on
> where the callsites for the notifiers are, and the person adding the
> notifier should know this..  

> We can add atomic notifiers to the blocking
> chain, just not vice versa.  The idea is that if all the add and update
> call sites are blocking, you just register the blocking chain and forget
> the atomic one.  

> The only difference between atomic and blocking
> notifiers is whether we use a spinlock or a mutex to guard the integrity
> of the call chain ... if you know you always have user context at the
> callsites, then you can always use the mutex.

> Then, for blocking notifiers, I think in init, we can register a single
> notifier which just calls __might_sleep() ... that will pick up at
> runtime any atomic callsite.

I like that part. Simple and elegant :)

> 
> For atomics, you just set up an atomic call chain and leave the blocking
> one null.  Then we get a BUG if anyone tries to register a blocking
> notifier to an atomic only pm_qos_object.
> 
(Well, we can also just ignore and print a WARN() ... but I got your
point)

But I don't think I understand how you want to set up the call chains.
(I.e. How to decide if all call-sites are from process-context (mutex
allowed)? )

As far as I see, the locking for the notifier-chains is in the head. So
I have to decide before the first AddNotifier what locking I
want (blocking_ or atomic_notifier_head). 

Are you thinking about having it hardcoded alongside the pm_qos_object
instantiation? (I think that would be ok) 

Or are you thinking about some other scheme I don't see?

> The implementation looks fine, except:
> 
> [...]
> >  /**
> > + * pm_qos_add_notifier_nonblocking - sets notification entry for changes to target value
> > + *
> > + * Code executed by the notifier block may not sleep!
> > + *
> > + * @pm_qos_class: identifies which qos target changes should be notified.
> > + * @notifier: notifier block managed by caller.
> > + *
> > + * Will register the notifier into a notification chain that gets called
> > + * upon changes to the pm_qos_class target value.
> > + */
> > +int pm_qos_add_notifier_nonblocking(int pm_qos_class, struct notifier_block *notifier)
> 
> Rightly or wrongly, the notifier people use atomic not nonblocking, so
> we should really stick with it to avoid confusion.

Yes. I think that's better. 

> James
> 

... mulling it over, I think everything you say add's up if I move the
decision to the pm_qos_object instatiation. So I will send out a new
patch with that implemented.

Cheers,
Flo

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

[James Bottomley]

On Mon, 2010-06-07 at 16:10 +0200, Florian Mickler wrote:
> On Mon, 07 Jun 2010 09:10:40 -0400
> James Bottomley <James.Bottomley@suse.de> wrote:
> 
> > > diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> > > index f42d3f7..0a67997 100644
> > > --- a/kernel/pm_qos_params.c
> > > +++ b/kernel/pm_qos_params.c
> > > @@ -63,7 +63,8 @@ static s32 min_compare(s32 v1, s32 v2);
> > >  
> > >  struct pm_qos_object {
> > >  	struct pm_qos_request_list requests;
> > > -	struct blocking_notifier_head *notifiers;
> > > +	struct atomic_notifier_head *notifiers;
> > > +	struct blocking_notifier_head *blocking_notifiers;
> > >  	struct miscdevice pm_qos_power_miscdev;
> > >  	char *name;
> > >  	s32 default_value;
> > > @@ -72,20 +73,24 @@ struct pm_qos_object {
> > >  };
> > >  
> > >  static struct pm_qos_object null_pm_qos;
> > > -static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
> > > +static ATOMIC_NOTIFIER_HEAD(cpu_dma_lat_notifier);
> > > +static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_blocking_notifier);
> > 
> > So I think it might be better implemented by having only a single active
> > notifier head: either blocking or atomic  because all this depends on
> > where the callsites for the notifiers are, and the person adding the
> > notifier should know this..  
> 
> > We can add atomic notifiers to the blocking
> > chain, just not vice versa.  The idea is that if all the add and update
> > call sites are blocking, you just register the blocking chain and forget
> > the atomic one.  
> 
> > The only difference between atomic and blocking
> > notifiers is whether we use a spinlock or a mutex to guard the integrity
> > of the call chain ... if you know you always have user context at the
> > callsites, then you can always use the mutex.
> 
> > Then, for blocking notifiers, I think in init, we can register a single
> > notifier which just calls __might_sleep() ... that will pick up at
> > runtime any atomic callsite.
> 
> I like that part. Simple and elegant :)
> 
> > 
> > For atomics, you just set up an atomic call chain and leave the blocking
> > one null.  Then we get a BUG if anyone tries to register a blocking
> > notifier to an atomic only pm_qos_object.
> > 
> (Well, we can also just ignore and print a WARN() ... but I got your
> point)
> 
> But I don't think I understand how you want to set up the call chains.
> (I.e. How to decide if all call-sites are from process-context (mutex
> allowed)? )

We don't.  Whoever creates the new pm_qos constratint decides this ...
based on where they're putting the add and update calls.  They do this
by either declaring a blocking or an atomic notifier chain for the
pm_qos_object.

> As far as I see, the locking for the notifier-chains is in the head. So
> I have to decide before the first AddNotifier what locking I
> want (blocking_ or atomic_notifier_head). 

Right.  We still have the atomic and blocking pm_qos notifier
registers ... but if you try to register an blocking notifier to a
pm_qos_object that only supports atomic, we bug.  Conversely, if you try
to register an atomic notifier to a pm_qos_object that supports
blocking, we just add it to the blocking call chain ... there's no harm
in that since an atomic notifier is only guaranteeing that it won't
sleep, so it's irrelevant to it whether we have user context or not.

> Are you thinking about having it hardcoded alongside the pm_qos_object
> instantiation? (I think that would be ok) 
> 
> Or are you thinking about some other scheme I don't see?

James

[PATCH v2] pm_qos: make update_request callable from interrupt context

[florian]

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.

Signed-off-by: Florian Mickler <florian@mickler.org>
---
 include/linux/pm_qos_params.h |    2 +-
 kernel/pm_qos_params.c        |   54 +++++++++++++++++++++-------------------
 2 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/include/linux/pm_qos_params.h b/include/linux/pm_qos_params.h
index 8ba440e..2959aac 100644
--- a/include/linux/pm_qos_params.h
+++ b/include/linux/pm_qos_params.h
@@ -22,6 +22,6 @@ void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
 void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req);
 
 int pm_qos_request(int pm_qos_class);
+
 int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier);
 int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
-
diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index f42d3f7..da7e85e 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -63,7 +63,7 @@ static s32 min_compare(s32 v1, s32 v2);
 
 struct pm_qos_object {
 	struct pm_qos_request_list requests;
-	struct blocking_notifier_head *notifiers;
+	struct atomic_notifier_head *notifiers;
 	struct miscdevice pm_qos_power_miscdev;
 	char *name;
 	s32 default_value;
@@ -72,7 +72,7 @@ struct pm_qos_object {
 };
 
 static struct pm_qos_object null_pm_qos;
-static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
+static ATOMIC_NOTIFIER_HEAD(cpu_dma_lat_notifier);
 static struct pm_qos_object cpu_dma_pm_qos = {
 	.requests = {LIST_HEAD_INIT(cpu_dma_pm_qos.requests.list)},
 	.notifiers = &cpu_dma_lat_notifier,
@@ -82,7 +82,7 @@ static struct pm_qos_object cpu_dma_pm_qos = {
 	.comparitor = min_compare
 };
 
-static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
+static ATOMIC_NOTIFIER_HEAD(network_lat_notifier);
 static struct pm_qos_object network_lat_pm_qos = {
 	.requests = {LIST_HEAD_INIT(network_lat_pm_qos.requests.list)},
 	.notifiers = &network_lat_notifier,
@@ -93,7 +93,7 @@ static struct pm_qos_object network_lat_pm_qos = {
 };
 
 
-static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
+static ATOMIC_NOTIFIER_HEAD(network_throughput_notifier);
 static struct pm_qos_object network_throughput_pm_qos = {
 	.requests = {LIST_HEAD_INIT(network_throughput_pm_qos.requests.list)},
 	.notifiers = &network_throughput_notifier,
@@ -103,7 +103,6 @@ static struct pm_qos_object network_throughput_pm_qos = {
 	.comparitor = max_compare
 };
 
-
 static struct pm_qos_object *pm_qos_array[] = {
 	&null_pm_qos,
 	&cpu_dma_pm_qos,
@@ -135,35 +134,30 @@ static s32 min_compare(s32 v1, s32 v2)
 	return min(v1, v2);
 }
 
-
 static void update_target(int pm_qos_class)
 {
 	s32 extreme_value;
+	struct pm_qos_object *obj = pm_qos_array[pm_qos_class];
 	struct pm_qos_request_list *node;
 	unsigned long flags;
 	int call_notifier = 0;
 
 	spin_lock_irqsave(&pm_qos_lock, flags);
-	extreme_value = pm_qos_array[pm_qos_class]->default_value;
-	list_for_each_entry(node,
-			&pm_qos_array[pm_qos_class]->requests.list, list) {
-		extreme_value = pm_qos_array[pm_qos_class]->comparitor(
-				extreme_value, node->value);
+	extreme_value = obj->default_value;
+	list_for_each_entry(node, &obj->requests.list, list) {
+		extreme_value = obj->comparitor(extreme_value, node->value);
 	}
-	if (atomic_read(&pm_qos_array[pm_qos_class]->target_value) !=
-			extreme_value) {
+	if (atomic_read(&obj->target_value) != extreme_value) {
 		call_notifier = 1;
-		atomic_set(&pm_qos_array[pm_qos_class]->target_value,
-				extreme_value);
+		atomic_set(&obj->target_value, extreme_value);
 		pr_debug(KERN_ERR "new target for qos %d is %d\n", pm_qos_class,
-			atomic_read(&pm_qos_array[pm_qos_class]->target_value));
+			atomic_read(&obj->target_value));
 	}
 	spin_unlock_irqrestore(&pm_qos_lock, flags);
 
-	if (call_notifier)
-		blocking_notifier_call_chain(
-				pm_qos_array[pm_qos_class]->notifiers,
-					(unsigned long) extreme_value, NULL);
+	if (call_notifier) 
+		atomic_notifier_call_chain(obj->notifiers,
+				(unsigned long) extreme_value, NULL);
 }
 
 static int register_pm_qos_misc(struct pm_qos_object *qos)
@@ -272,6 +266,7 @@ EXPORT_SYMBOL_GPL(pm_qos_update_request);
 
 /**
  * pm_qos_remove_request - modifies an existing qos request
+ *
  * @pm_qos_req: handle to request list element
  *
  * Will remove pm qos request from the list of requests and
@@ -298,18 +293,23 @@ EXPORT_SYMBOL_GPL(pm_qos_remove_request);
 
 /**
  * pm_qos_add_notifier - sets notification entry for changes to target value
+ *
+ * Notifiers added with this function are executed in process context.
+ * They may also be scheduled for later execution if pm_qos_update_request is
+ * called from interrupt context.
+ *
  * @pm_qos_class: identifies which qos target changes should be notified.
  * @notifier: notifier block managed by caller.
  *
- * will register the notifier into a notification chain that gets called
+ * Will register the notifier into a notification chain that gets called
  * upon changes to the pm_qos_class target value.
  */
 int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier)
 {
 	int retval;
 
-	retval = blocking_notifier_chain_register(
-			pm_qos_array[pm_qos_class]->notifiers, notifier);
+	retval = atomic_notifier_chain_register(
+		pm_qos_array[pm_qos_class]->notifiers, notifier);
 
 	return retval;
 }
@@ -317,18 +317,19 @@ EXPORT_SYMBOL_GPL(pm_qos_add_notifier);
 
 /**
  * pm_qos_remove_notifier - deletes notification entry from chain.
+ *
  * @pm_qos_class: identifies which qos target changes are notified.
  * @notifier: notifier block to be removed.
  *
- * will remove the notifier from the notification chain that gets called
+ * Will remove the notifier from the notification chain that gets called
  * upon changes to the pm_qos_class target value.
  */
 int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
 {
 	int retval;
 
-	retval = blocking_notifier_chain_unregister(
-			pm_qos_array[pm_qos_class]->notifiers, notifier);
+	retval = atomic_notifier_chain_unregister(
+		pm_qos_array[pm_qos_class]->notifiers, notifier);
 
 	return retval;
 }
@@ -381,6 +382,7 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
 	} else
 		return -EINVAL;
 
+
 	pm_qos_req = (struct pm_qos_request_list *)filp->private_data;
 	pm_qos_update_request(pm_qos_req, value);
 
-- 
1.7.1

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

[florian]

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.

Signed-off-by: Florian Mickler <florian@mickler.org>
---

sorry, forgot to commit the changes of my last pass.

btw, that = __might_sleep() gives an "incompatible pointer" warning. 

should we care? it boots ok.

include/linux/pm_qos_params.h |    3 +-
 kernel/pm_qos_params.c        |   61 +++++++++++++++++++++++-----------------
 2 files changed, 36 insertions(+), 28 deletions(-)

diff --git a/include/linux/pm_qos_params.h b/include/linux/pm_qos_params.h
index 8ba440e..c21ab20 100644
--- a/include/linux/pm_qos_params.h
+++ b/include/linux/pm_qos_params.h
@@ -20,8 +20,7 @@ struct pm_qos_request_list *pm_qos_add_request(int pm_qos_class, s32 value);
 void pm_qos_update_request(struct pm_qos_request_list *pm_qos_req,
 		s32 new_value);
 void pm_qos_remove_request(struct pm_qos_request_list *pm_qos_req);
-
 int pm_qos_request(int pm_qos_class);
+
 int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier);
 int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier);
-
diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
index f42d3f7..5ae86c8 100644
--- a/kernel/pm_qos_params.c
+++ b/kernel/pm_qos_params.c
@@ -63,7 +63,7 @@ static s32 min_compare(s32 v1, s32 v2);
 
 struct pm_qos_object {
 	struct pm_qos_request_list requests;
-	struct blocking_notifier_head *notifiers;
+	struct atomic_notifier_head *notifiers;
 	struct miscdevice pm_qos_power_miscdev;
 	char *name;
 	s32 default_value;
@@ -72,7 +72,7 @@ struct pm_qos_object {
 };
 
 static struct pm_qos_object null_pm_qos;
-static BLOCKING_NOTIFIER_HEAD(cpu_dma_lat_notifier);
+static ATOMIC_NOTIFIER_HEAD(cpu_dma_lat_notifier);
 static struct pm_qos_object cpu_dma_pm_qos = {
 	.requests = {LIST_HEAD_INIT(cpu_dma_pm_qos.requests.list)},
 	.notifiers = &cpu_dma_lat_notifier,
@@ -82,7 +82,7 @@ static struct pm_qos_object cpu_dma_pm_qos = {
 	.comparitor = min_compare
 };
 
-static BLOCKING_NOTIFIER_HEAD(network_lat_notifier);
+static ATOMIC_NOTIFIER_HEAD(network_lat_notifier);
 static struct pm_qos_object network_lat_pm_qos = {
 	.requests = {LIST_HEAD_INIT(network_lat_pm_qos.requests.list)},
 	.notifiers = &network_lat_notifier,
@@ -93,7 +93,7 @@ static struct pm_qos_object network_lat_pm_qos = {
 };
 
 
-static BLOCKING_NOTIFIER_HEAD(network_throughput_notifier);
+static ATOMIC_NOTIFIER_HEAD(network_throughput_notifier);
 static struct pm_qos_object network_throughput_pm_qos = {
 	.requests = {LIST_HEAD_INIT(network_throughput_pm_qos.requests.list)},
 	.notifiers = &network_throughput_notifier,
@@ -103,7 +103,6 @@ static struct pm_qos_object network_throughput_pm_qos = {
 	.comparitor = max_compare
 };
 
-
 static struct pm_qos_object *pm_qos_array[] = {
 	&null_pm_qos,
 	&cpu_dma_pm_qos,
@@ -135,35 +134,30 @@ static s32 min_compare(s32 v1, s32 v2)
 	return min(v1, v2);
 }
 
-
 static void update_target(int pm_qos_class)
 {
 	s32 extreme_value;
+	struct pm_qos_object *obj = pm_qos_array[pm_qos_class];
 	struct pm_qos_request_list *node;
 	unsigned long flags;
 	int call_notifier = 0;
 
 	spin_lock_irqsave(&pm_qos_lock, flags);
-	extreme_value = pm_qos_array[pm_qos_class]->default_value;
-	list_for_each_entry(node,
-			&pm_qos_array[pm_qos_class]->requests.list, list) {
-		extreme_value = pm_qos_array[pm_qos_class]->comparitor(
-				extreme_value, node->value);
+	extreme_value = obj->default_value;
+	list_for_each_entry(node, &obj->requests.list, list) {
+		extreme_value = obj->comparitor(extreme_value, node->value);
 	}
-	if (atomic_read(&pm_qos_array[pm_qos_class]->target_value) !=
-			extreme_value) {
+	if (atomic_read(&obj->target_value) != extreme_value) {
 		call_notifier = 1;
-		atomic_set(&pm_qos_array[pm_qos_class]->target_value,
-				extreme_value);
+		atomic_set(&obj->target_value, extreme_value);
 		pr_debug(KERN_ERR "new target for qos %d is %d\n", pm_qos_class,
-			atomic_read(&pm_qos_array[pm_qos_class]->target_value));
+			atomic_read(&obj->target_value));
 	}
 	spin_unlock_irqrestore(&pm_qos_lock, flags);
 
-	if (call_notifier)
-		blocking_notifier_call_chain(
-				pm_qos_array[pm_qos_class]->notifiers,
-					(unsigned long) extreme_value, NULL);
+	if (call_notifier) 
+		atomic_notifier_call_chain(obj->notifiers,
+				(unsigned long) extreme_value, NULL);
 }
 
 static int register_pm_qos_misc(struct pm_qos_object *qos)
@@ -272,6 +266,7 @@ EXPORT_SYMBOL_GPL(pm_qos_update_request);
 
 /**
  * pm_qos_remove_request - modifies an existing qos request
+ *
  * @pm_qos_req: handle to request list element
  *
  * Will remove pm qos request from the list of requests and
@@ -298,18 +293,23 @@ EXPORT_SYMBOL_GPL(pm_qos_remove_request);
 
 /**
  * pm_qos_add_notifier - sets notification entry for changes to target value
+ *
+ * Note: There may be update sites which are called from interrupt context
+ * for a given constraint. Check pm_qos_power_init to check if the constraint
+ * in question does already have a might_sleep() added to the notifier chain.
+ *
  * @pm_qos_class: identifies which qos target changes should be notified.
  * @notifier: notifier block managed by caller.
  *
- * will register the notifier into a notification chain that gets called
+ * Will register the notifier into a notification chain that gets called
  * upon changes to the pm_qos_class target value.
  */
 int pm_qos_add_notifier(int pm_qos_class, struct notifier_block *notifier)
 {
 	int retval;
 
-	retval = blocking_notifier_chain_register(
-			pm_qos_array[pm_qos_class]->notifiers, notifier);
+	retval = atomic_notifier_chain_register(
+		pm_qos_array[pm_qos_class]->notifiers, notifier);
 
 	return retval;
 }
@@ -317,18 +317,19 @@ EXPORT_SYMBOL_GPL(pm_qos_add_notifier);
 
 /**
  * pm_qos_remove_notifier - deletes notification entry from chain.
+ *
  * @pm_qos_class: identifies which qos target changes are notified.
  * @notifier: notifier block to be removed.
  *
- * will remove the notifier from the notification chain that gets called
+ * Will remove the notifier from the notification chain that gets called
  * upon changes to the pm_qos_class target value.
  */
 int pm_qos_remove_notifier(int pm_qos_class, struct notifier_block *notifier)
 {
 	int retval;
 
-	retval = blocking_notifier_chain_unregister(
-			pm_qos_array[pm_qos_class]->notifiers, notifier);
+	retval = atomic_notifier_chain_unregister(
+		pm_qos_array[pm_qos_class]->notifiers, notifier);
 
 	return retval;
 }
@@ -381,6 +382,7 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
 	} else
 		return -EINVAL;
 
+
 	pm_qos_req = (struct pm_qos_request_list *)filp->private_data;
 	pm_qos_update_request(pm_qos_req, value);
 
@@ -388,21 +390,28 @@ static ssize_t pm_qos_power_write(struct file *filp, const char __user *buf,
 }
 
 
+static struct notifier_block nb_might_sleep = {
+	.notifier_call = __might_sleep,
+};
+
 static int __init pm_qos_power_init(void)
 {
 	int ret = 0;
 
 	ret = register_pm_qos_misc(&cpu_dma_pm_qos);
+	pm_qos_add_notifier(PM_QOS_CPU_DMA_LATENCY, &nb_might_sleep);
 	if (ret < 0) {
 		printk(KERN_ERR "pm_qos_param: cpu_dma_latency setup failed\n");
 		return ret;
 	}
 	ret = register_pm_qos_misc(&network_lat_pm_qos);
+	pm_qos_add_notifier(PM_QOS_NETWORK_LATENCY,&nb_might_sleep);
 	if (ret < 0) {
 		printk(KERN_ERR "pm_qos_param: network_latency setup failed\n");
 		return ret;
 	}
 	ret = register_pm_qos_misc(&network_throughput_pm_qos);
+	pm_qos_add_notifier(PM_QOS_NETWORK_THROUGHPUT,&nb_might_sleep);
 	if (ret < 0)
 		printk(KERN_ERR
 			"pm_qos_param: network_throughput setup failed\n");
-- 
1.7.1

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

[James Bottomley]

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

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

[mark gross]

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?

--mgross

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

[Florian Mickler]

On Mon, 7 Jun 2010 21:13:41 -0700
mark gross <640e9920@gmail.com> 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).

Argh. True. The spinlock is held while calling the notifiers. 

> > 
> > 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?
> 
> --mgross

I think that would be conceptual right. 

Under the assumption, that the qos-infrastructure is used by the
drivers to guarantee their functioning, we can not allow a race to
occur. 

( Individual listeners of course are free to spawn or schedule work if
they think it is safe to do so and have all races possible considered. )

I'll audit the current listeners this evening and respin this patch
without the might_sleep()'s and adapted comments.
What do you think?

Cheers,
Flo

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

[James Bottomley]

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

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

[Florian Mickler]

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. 

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

[Florian Mickler]

On Wed, 9 Jun 2010 08:54:27 +0200
Florian Mickler <florian@mickler.org> 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).
	
Actually, I just checked and __atomic_notifier_call_chain()
uses an rcu_read_lock() to protect the chain from removal of
notifiers while going through it. 
	
So I (now again) think this patch (with queuing the  might_sleep() for
the network notifier) might be enough.

Cheers,
Flo

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

[Florian Mickler]

On Wed, 9 Jun 2010 09:13:30 +0200
Florian Mickler <florian@mickler.org> wrote:

> On Wed, 9 Jun 2010 08:54:27 +0200
> Florian Mickler <florian@mickler.org> 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).
> 	
> Actually, I just checked and __atomic_notifier_call_chain()
> uses an rcu_read_lock() to protect the chain from removal of
> notifiers while going through it. 
> 	
> So I (now again) think this patch (with queuing the  might_sleep() for
> the network notifier) might be enough.
> 
> Cheers,
> Flo

Oh, wow. I changed my mind again after reading up on RCU :|.

 * It is illegal to block while in an RCU read-side critical section.

(from include/linux/rcupdate.h)

Cheers,
Flo

p.s.: so, back to my original plan.