Re: [PATCH RFC] pm_qos_requirement might sleep

["John Kacur" <jkacur@gmail.com>]

On Fri, Aug 29, 2008 at 2:32 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> 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.
>

Hi Andrew

The purpose of the patch is to remove the spin_lock around the read in
the function pm_qos_requirement - since spinlocks can sleep in -rt and
this function is called from idle. I did propose a patch, that simply
removed the spinlock, since Peter Zilstra and others (yourself) point
out, the lock isn't needed to read a single word from memory. Although
the patch worked fine, Arjan objected to it, because it was not
symmetric. A reader of the code might wonder why it was locked in
other functions but not here. He suggested changing it to an atomic
type might be less confusing. The alternative which I like better
actually is to simply remove the lock in pm_qos_requirement and add a
comment for readers of the code.

John