Re: [PATCH RFC] pm_qos_requirement might sleep

[Andrew Morton <akpm@linux-foundation.org>]

On Thu, 28 Aug 2008 12:44:20 -0700
mark gross <mgross@linux.intel.com> wrote:

> From: John Kacur <jkacur at gmail dot com>

I tend to deobfuscate email addresses when preparing the changelogs.

Not sure why, really, and I feel a bit guilty about doing it (hey, you
can forward the resulting spam to me if you like).  One reason I guess
is to avoid breaking all the emails which my scripts will automatically
send out as the patch goes through its little lifecycle.  Some of which
can sometimes be quite important.

> Patch to make PM_QOS and CPU_IDLE play nicer when ran with the
> RT-Preempt kernel.  CPU_IDLE polls the target_value's of some of the
> pm_qos parameters from the idle loop causing sleeping locking
> warnings.  Changing the target_value to an atomic avoids this issue.
> 
> Signed-off-by: mark gross <mgross@linux.intel.com>
> 
> Thanks,
> 
> --mgross
> 
> 
> Remove the spinlock in pm_qos_requirement by making target_value an atomic type.
> This is necessary for real-time since pm_qos_requirement is called by idle and
> cannot be allowed to sleep.
> Signed-off-by: John Kacur <jkacur at gmail dot com>

As usual when I get two different changelogs, I create a masterpiece
which is better than either of the originals!

> Index: linux-2.6/kernel/pm_qos_params.c
> ===================================================================
> --- linux-2.6.orig/kernel/pm_qos_params.c	2008-08-08 07:52:01.000000000 -0700
> +++ linux-2.6/kernel/pm_qos_params.c	2008-08-28 12:14:55.000000000 -0700
> @@ -43,7 +43,7 @@
>  #include <linux/uaccess.h>
>  
>  /*
> - * locking rule: all changes to target_value or requirements or notifiers lists
> + * locking rule: all changes to requirements or notifiers lists
>   * or pm_qos_object list and pm_qos_objects need to happen with pm_qos_lock
>   * held, taken with _irqsave.  One lock to rule them all
>   */
> @@ -66,7 +66,7 @@
>  	struct miscdevice pm_qos_power_miscdev;
>  	char *name;
>  	s32 default_value;
> -	s32 target_value;
> +	atomic_t target_value;
>  	s32 (*comparitor)(s32, s32);
>  };
>  
> @@ -77,7 +77,7 @@
>  	.notifiers = &cpu_dma_lat_notifier,
>  	.name = "cpu_dma_latency",
>  	.default_value = 2000 * USEC_PER_SEC,
> -	.target_value = 2000 * USEC_PER_SEC,
> +	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
>  	.comparitor = min_compare
>  };
>  
> @@ -87,7 +87,7 @@
>  	.notifiers = &network_lat_notifier,
>  	.name = "network_latency",
>  	.default_value = 2000 * USEC_PER_SEC,
> -	.target_value = 2000 * USEC_PER_SEC,
> +	.target_value = ATOMIC_INIT(2000 * USEC_PER_SEC),
>  	.comparitor = min_compare
>  };
>  
> @@ -99,7 +99,7 @@
>  	.notifiers = &network_throughput_notifier,
>  	.name = "network_throughput",
>  	.default_value = 0,
> -	.target_value = 0,
> +	.target_value = ATOMIC_INIT(0),
>  	.comparitor = max_compare
>  };
>  
> @@ -150,11 +150,11 @@
>  		extreme_value = pm_qos_array[target]->comparitor(
>  				extreme_value, node->value);
>  	}
> -	if (pm_qos_array[target]->target_value != extreme_value) {
> +	if (atomic_read(&pm_qos_array[target]->target_value) != extreme_value) {
>  		call_notifier = 1;
> -		pm_qos_array[target]->target_value = extreme_value;
> +		atomic_set(&pm_qos_array[target]->target_value, extreme_value);
>  		pr_debug(KERN_ERR "new target for qos %d is %d\n", target,
> -			pm_qos_array[target]->target_value);
> +			atomic_read(&pm_qos_array[target]->target_value));
>  	}
>  	spin_unlock_irqrestore(&pm_qos_lock, flags);
>  
> @@ -193,14 +193,7 @@
>   */
>  int pm_qos_requirement(int pm_qos_class)
>  {
> -	int ret_val;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&pm_qos_lock, flags);
> -	ret_val = pm_qos_array[pm_qos_class]->target_value;
> -	spin_unlock_irqrestore(&pm_qos_lock, flags);
> -
> -	return ret_val;
> +	return atomic_read(&pm_qos_array[pm_qos_class]->target_value);
>  }
>  EXPORT_SYMBOL_GPL(pm_qos_requirement);

OK, so I've stared and stared.  This patch doesn't do anything.

I'm suspecting that the rt patch might be doing something silly, like
thinking that it needs to take pm_qos_lock to read a single word from
memory, or something like that.

But this patch is simply not understandable given the amount of
information which you guys have provided.

Help?