Re: [PATCH] pm_qos: make update_request non blocking

[James Bottomley <James.Bottomley@suse.de>]

On Sat, 2010-06-05 at 22:46 +0200, florian@mickler.org wrote:
> First we use atomic_notifier to get the spinlocked variant.
> Secondly we call the notifiers via schedule_work.
> 
> Signed-off-by: Florian Mickler <florian@mickler.org>
> ---
> 
> Please advise.
> 
>  kernel/pm_qos_params.c |   33 ++++++++++++++++++++++-----------
>  1 files changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/pm_qos_params.c b/kernel/pm_qos_params.c
> index f42d3f7..7335c27 100644
> --- a/kernel/pm_qos_params.c
> +++ b/kernel/pm_qos_params.c
> @@ -40,6 +40,7 @@
>  #include <linux/string.h>
>  #include <linux/platform_device.h>
>  #include <linux/init.h>
> +#include <linux/workqueue.h>
>  
>  #include <linux/uaccess.h>
>  
> @@ -63,7 +64,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 +73,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 +83,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 +94,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 +104,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,6 +135,15 @@ static s32 min_compare(s32 v1, s32 v2)
>  	return min(v1, v2);
>  }
>  
> +static void update_notify(struct work_struct* work){
> +	int pm_qos_class = atomic_long_read(&work->data);
> +	
> +	s32 extreme_value = atomic_read(&pm_qos_array[pm_qos_class]->target_value);
> +	atomic_notifier_call_chain(
> +		pm_qos_array[pm_qos_class]->notifiers,
> +			(unsigned long) extreme_value, NULL);
> +}
> +struct work_struct pm_qos_update_notify_work;
>  
>  static void update_target(int pm_qos_class)
>  {
> @@ -160,10 +169,10 @@ static void update_target(int pm_qos_class)
>  	}
>  	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_long_set(&pm_qos_update_notify_work.data, pm_qos_class);
> +		schedule_work(&pm_qos_update_notify_work);

Actually, you should use execute_in_process_context for this ... that
way any current call from user context won't change semantics (i.e. the
notifier will be executed before the call finishes).

I also suspect we might need an atomic notifier chain to be called
in-line ... things like the dma latency notifier need in-line
notification, I can see that some android ones would at all.  However,
this question can be deferred until an actual use case is seen.

Also, your implementation is racy: you can't have a single
pm_qos_update_notify_work because that means that only one thread can
use it at once ... and that means the work it is scheduling has to be
begun before it can be re-used.  Currently, any thread can blow away the
queued work item, which will likely oops the system.

James