lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 28 Aug 2008 17:32:01 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	mgross@...ux.intel.com
Cc:	linux-kernel@...r.kernel.org, linux-rt-users@...r.kernel.org,
	peterz@...radead.org, rostedt@...dmis.org, mingo@...e.hu,
	tglx@...utronix.de, arjan@...radead.org, jkacur@...il.com
Subject: Re: [PATCH RFC] pm_qos_requirement might sleep

On Thu, 28 Aug 2008 12:44:20 -0700
mark gross <mgross@...ux.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@...ux.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?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists