Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1306559 > unrolled thread

[RFC PATCH 04/19] cpufreq: bring data structures close to their locks

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC PATCH 04/19] cpufreq: bring data structures close to their locks Juri Lelli <juri.lelli@arm.com> - 2016-01-11 18:50 +0100
    Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their  locks Peter Zijlstra <peterz@infradead.org> - 2016-01-11 23:10 +0100
      Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their locks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-12 00:10 +0100
        Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their  locks Peter Zijlstra <peterz@infradead.org> - 2016-01-12 09:30 +0100
          Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their  locks Juri Lelli <juri.lelli@arm.com> - 2016-01-12 11:50 +0100
          Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their locks "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-12 17:50 +0100
    Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their  locks Peter Zijlstra <peterz@infradead.org> - 2016-01-11 23:10 +0100
      Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their  locks Viresh Kumar <viresh.kumar@linaro.org> - 2016-01-12 10:40 +0100
        Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their  locks Juri Lelli <juri.lelli@arm.com> - 2016-01-12 12:30 +0100
          Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their  locks Peter Zijlstra <peterz@infradead.org> - 2016-01-12 13:00 +0100
            Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their  locks Juri Lelli <juri.lelli@arm.com> - 2016-01-12 13:40 +0100
              Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their  locks Juri Lelli <juri.lelli@arm.com> - 2016-01-12 16:30 +0100
                Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their  locks Peter Zijlstra <peterz@infradead.org> - 2016-01-12 17:00 +0100

#1306559 — [RFC PATCH 04/19] cpufreq: bring data structures close to their locks

Currently it is not easy to figure out which lock/mutex protects which data
structure. Clean things up by moving data structures and their locks/mutexs
closer; also, change comments to document relations further.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Juri Lelli <juri.lelli@arm.com>
---
 drivers/cpufreq/cpufreq.c | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 2e41356..00a00cd 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -31,7 +31,25 @@
 #include <linux/tick.h>
 #include <trace/events/power.h>
 
+/**
+ * Iterate over governors
+ *
+ * cpufreq_governor_list is protected by cpufreq_governor_mutex.
+ */
+static LIST_HEAD(cpufreq_governor_list);
+static DEFINE_MUTEX(cpufreq_governor_mutex);
+#define for_each_governor(__governor)				\
+	list_for_each_entry(__governor, &cpufreq_governor_list, governor_list)
+
+/**
+ * The "cpufreq driver" - the arch- or hardware-dependent low
+ * level driver of CPUFreq support, and its spinlock (cpufreq_driver_lock).
+ * This lock also protects cpufreq_cpu_data array and cpufreq_policy_list.
+ */
+static struct cpufreq_driver *cpufreq_driver;
+static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
 static LIST_HEAD(cpufreq_policy_list);
+static DEFINE_RWLOCK(cpufreq_driver_lock);
 
 static inline bool policy_is_inactive(struct cpufreq_policy *policy)
 {
@@ -86,21 +104,6 @@ static struct cpufreq_policy *first_policy(bool active)
 #define for_each_inactive_policy(__policy)		\
 	for_each_suitable_policy(__policy, false)
 
-/* Iterate over governors */
-static LIST_HEAD(cpufreq_governor_list);
-#define for_each_governor(__governor)				\
-	list_for_each_entry(__governor, &cpufreq_governor_list, governor_list)
-
-/**
- * The "cpufreq driver" - the arch- or hardware-dependent low
- * level driver of CPUFreq support, and its spinlock. This lock
- * also protects the cpufreq_cpu_data array.
- */
-static struct cpufreq_driver *cpufreq_driver;
-static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
-static DEFINE_RWLOCK(cpufreq_driver_lock);
-static DEFINE_MUTEX(cpufreq_governor_mutex);
-
 /* Flag to suspend/resume CPUFreq governors */
 static bool cpufreq_suspended;
 
-- 
2.2.2

[toc] | [next] | [standalone]

#1306748 — Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their locks

On Mon, Jan 11, 2016 at 05:35:45PM +0000, Juri Lelli wrote:
> +/**
> + * The "cpufreq driver" - the arch- or hardware-dependent low
> + * level driver of CPUFreq support, and its spinlock (cpufreq_driver_lock).
> + * This lock also protects cpufreq_cpu_data array and cpufreq_policy_list.
> + */
> +static struct cpufreq_driver *cpufreq_driver;
> +static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
>  static LIST_HEAD(cpufreq_policy_list);
> +static DEFINE_RWLOCK(cpufreq_driver_lock);

Part of my suggestion was to fold the per-cpu data of cpufreq_cpu_data
into struct cpufreq_driver.

That way each cpufreq_driver will have its own copy and there'd be only
the one global pointer to swizzle. Something very well suited to RCU.

[toc] | [prev] | [next] | [standalone]

#1306810

On Monday, January 11, 2016 11:05:28 PM Peter Zijlstra wrote:
> On Mon, Jan 11, 2016 at 05:35:45PM +0000, Juri Lelli wrote:
> > +/**
> > + * The "cpufreq driver" - the arch- or hardware-dependent low
> > + * level driver of CPUFreq support, and its spinlock (cpufreq_driver_lock).
> > + * This lock also protects cpufreq_cpu_data array and cpufreq_policy_list.
> > + */
> > +static struct cpufreq_driver *cpufreq_driver;
> > +static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
> >  static LIST_HEAD(cpufreq_policy_list);
> > +static DEFINE_RWLOCK(cpufreq_driver_lock);
> 
> Part of my suggestion was to fold the per-cpu data of cpufreq_cpu_data
> into struct cpufreq_driver.
> 
> That way each cpufreq_driver will have its own copy and there'd be only
> the one global pointer to swizzle. Something very well suited to RCU.

Well, I'm not really sure reworking all that is necessary.

What we need is to be able to call something analogous to dbs_timer_handler()
from the scheduler and a driver callback from there (if present).  For that,
it should be sufficient to have a pointer to that callback (that may be set
upon driver registration) protected by RCU (or should that be sched RCU
rather?) if I'm not missing anything.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1307092 — Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their locks

On Tue, Jan 12, 2016 at 12:03:39AM +0100, Rafael J. Wysocki wrote:
> On Monday, January 11, 2016 11:05:28 PM Peter Zijlstra wrote:
> > On Mon, Jan 11, 2016 at 05:35:45PM +0000, Juri Lelli wrote:
> > > +/**
> > > + * The "cpufreq driver" - the arch- or hardware-dependent low
> > > + * level driver of CPUFreq support, and its spinlock (cpufreq_driver_lock).
> > > + * This lock also protects cpufreq_cpu_data array and cpufreq_policy_list.
> > > + */
> > > +static struct cpufreq_driver *cpufreq_driver;
> > > +static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
> > >  static LIST_HEAD(cpufreq_policy_list);
> > > +static DEFINE_RWLOCK(cpufreq_driver_lock);
> > 
> > Part of my suggestion was to fold the per-cpu data of cpufreq_cpu_data
> > into struct cpufreq_driver.
> > 
> > That way each cpufreq_driver will have its own copy and there'd be only
> > the one global pointer to swizzle. Something very well suited to RCU.
> 
> Well, I'm not really sure reworking all that is necessary.
> 
> What we need is to be able to call something analogous to dbs_timer_handler()
> from the scheduler and a driver callback from there (if present).  For that,
> it should be sufficient to have a pointer to that callback (that may be set
> upon driver registration) protected by RCU (or should that be sched RCU
> rather?) if I'm not missing anything.

But such a callback will invariably want to use the per-cpu state. And
now you have two pointers, one for the driver and one for the per-cpu
state. Keeping that in sync is a pain.

Moving the per-cpu data into the driver solves that trivially.

[toc] | [prev] | [next] | [standalone]

#1307243 — Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their locks

Hi,

On 12/01/16 09:27, Peter Zijlstra wrote:
> On Tue, Jan 12, 2016 at 12:03:39AM +0100, Rafael J. Wysocki wrote:
> > On Monday, January 11, 2016 11:05:28 PM Peter Zijlstra wrote:
> > > On Mon, Jan 11, 2016 at 05:35:45PM +0000, Juri Lelli wrote:
> > > > +/**
> > > > + * The "cpufreq driver" - the arch- or hardware-dependent low
> > > > + * level driver of CPUFreq support, and its spinlock (cpufreq_driver_lock).
> > > > + * This lock also protects cpufreq_cpu_data array and cpufreq_policy_list.
> > > > + */
> > > > +static struct cpufreq_driver *cpufreq_driver;
> > > > +static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
> > > >  static LIST_HEAD(cpufreq_policy_list);
> > > > +static DEFINE_RWLOCK(cpufreq_driver_lock);
> > > 
> > > Part of my suggestion was to fold the per-cpu data of cpufreq_cpu_data
> > > into struct cpufreq_driver.
> > > 
> > > That way each cpufreq_driver will have its own copy and there'd be only
> > > the one global pointer to swizzle. Something very well suited to RCU.
> > 
> > Well, I'm not really sure reworking all that is necessary.
> > 
> > What we need is to be able to call something analogous to dbs_timer_handler()
> > from the scheduler and a driver callback from there (if present).  For that,
> > it should be sufficient to have a pointer to that callback (that may be set
> > upon driver registration) protected by RCU (or should that be sched RCU
> > rather?) if I'm not missing anything.
> 
> But such a callback will invariably want to use the per-cpu state. And
> now you have two pointers, one for the driver and one for the per-cpu
> state. Keeping that in sync is a pain.
> 
> Moving the per-cpu data into the driver solves that trivially.
> 

Oh, I think I now finally get your suggestion completely (I hope :-));
and it makes sense to me. On top of this series I have patches that
implement RCU logic. What I tried to do is to protect all cpufreq.c
stuff with a single mutex (plus RCU logic) and single policies with
another mutex (plus RCU logic). What you are saying should make things
easier to get right. I have to go back and try that.

My idea was to try to build some confidence that this first set is right
and then post the second part implementing RCU logic. 

Thanks,

- Juri

[toc] | [prev] | [next] | [standalone]

#1307622

On Tuesday, January 12, 2016 09:27:18 AM Peter Zijlstra wrote:
> On Tue, Jan 12, 2016 at 12:03:39AM +0100, Rafael J. Wysocki wrote:
> > On Monday, January 11, 2016 11:05:28 PM Peter Zijlstra wrote:
> > > On Mon, Jan 11, 2016 at 05:35:45PM +0000, Juri Lelli wrote:
> > > > +/**
> > > > + * The "cpufreq driver" - the arch- or hardware-dependent low
> > > > + * level driver of CPUFreq support, and its spinlock (cpufreq_driver_lock).
> > > > + * This lock also protects cpufreq_cpu_data array and cpufreq_policy_list.
> > > > + */
> > > > +static struct cpufreq_driver *cpufreq_driver;
> > > > +static DEFINE_PER_CPU(struct cpufreq_policy *, cpufreq_cpu_data);
> > > >  static LIST_HEAD(cpufreq_policy_list);
> > > > +static DEFINE_RWLOCK(cpufreq_driver_lock);
> > > 
> > > Part of my suggestion was to fold the per-cpu data of cpufreq_cpu_data
> > > into struct cpufreq_driver.
> > > 
> > > That way each cpufreq_driver will have its own copy and there'd be only
> > > the one global pointer to swizzle. Something very well suited to RCU.
> > 
> > Well, I'm not really sure reworking all that is necessary.
> > 
> > What we need is to be able to call something analogous to dbs_timer_handler()
> > from the scheduler and a driver callback from there (if present).  For that,
> > it should be sufficient to have a pointer to that callback (that may be set
> > upon driver registration) protected by RCU (or should that be sched RCU
> > rather?) if I'm not missing anything.
> 
> But such a callback will invariably want to use the per-cpu state.

Which likely is the driver's own per-cpu state, not the policy object itself.

> And now you have two pointers, one for the driver and one for the per-cpu
> state. Keeping that in sync is a pain.

Well, I basically need to guarantee that all of the pointers involved are set
and the data structures are valid when the driver pointer is set.

> Moving the per-cpu data into the driver solves that trivially.

It doesn't really address the case when the driver has its own per-cpu state
as I said above.

Thanks,
Rafael

[toc] | [prev] | [next] | [standalone]

#1306755 — Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their locks

On Mon, Jan 11, 2016 at 05:35:45PM +0000, Juri Lelli wrote:
> +/**
> + * Iterate over governors
> + *
> + * cpufreq_governor_list is protected by cpufreq_governor_mutex.
> + */
> +static LIST_HEAD(cpufreq_governor_list);
> +static DEFINE_MUTEX(cpufreq_governor_mutex);
> +#define for_each_governor(__governor)				\
> +	list_for_each_entry(__governor, &cpufreq_governor_list, governor_list)

So you could stuff the lockdep_assert_held() you later add intididually
into the for_each_governor macro, impossible to forget that way.

[toc] | [prev] | [next] | [standalone]

#1307151 — Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their locks

On 11-01-16, 23:07, Peter Zijlstra wrote:
> On Mon, Jan 11, 2016 at 05:35:45PM +0000, Juri Lelli wrote:
> > +/**
> > + * Iterate over governors
> > + *
> > + * cpufreq_governor_list is protected by cpufreq_governor_mutex.
> > + */
> > +static LIST_HEAD(cpufreq_governor_list);
> > +static DEFINE_MUTEX(cpufreq_governor_mutex);
> > +#define for_each_governor(__governor)				\
> > +	list_for_each_entry(__governor, &cpufreq_governor_list, governor_list)
> 
> So you could stuff the lockdep_assert_held() you later add intididually
> into the for_each_governor macro, impossible to forget that way.

How exactly? I couldn't see how it can be done in a neat and clean
way.

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1307286 — Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their locks

Hi,

On 12/01/16 14:57, Viresh Kumar wrote:
> On 11-01-16, 23:07, Peter Zijlstra wrote:
> > On Mon, Jan 11, 2016 at 05:35:45PM +0000, Juri Lelli wrote:
> > > +/**
> > > + * Iterate over governors
> > > + *
> > > + * cpufreq_governor_list is protected by cpufreq_governor_mutex.
> > > + */
> > > +static LIST_HEAD(cpufreq_governor_list);
> > > +static DEFINE_MUTEX(cpufreq_governor_mutex);
> > > +#define for_each_governor(__governor)				\
> > > +	list_for_each_entry(__governor, &cpufreq_governor_list, governor_list)
> > 
> > So you could stuff the lockdep_assert_held() you later add intididually
> > into the for_each_governor macro, impossible to forget that way.
> 
> How exactly? I couldn't see how it can be done in a neat and clean
> way.
> 

I tried to see if something like for_each_domain() can be done, but here
we use list_for_each_entry() macro. Peter, do you mean something like
the following?

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 78b1e2f..1a847a6 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -39,6 +39,7 @@
 static LIST_HEAD(cpufreq_governor_list);
 static DEFINE_MUTEX(cpufreq_governor_mutex);
 #define for_each_governor(__governor)				\
+	lockdep_assert_held(&cpufreq_governor_mutex);		\
 	list_for_each_entry(__governor, &cpufreq_governor_list, governor_list)
 
 /**
@@ -508,7 +509,6 @@ static struct cpufreq_governor *find_governor(const char *str_governor)
 {
 	struct cpufreq_governor *t;
 
-	lockdep_assert_held(&cpufreq_governor_mutex);
 	for_each_governor(t)
 		if (!strncasecmp(str_governor, t->name, CPUFREQ_NAME_LEN))
 			return t;

Since for_each_governor() is not used in if conditions that should be
fine?

Thanks,

- Juri

[toc] | [prev] | [next] | [standalone]

#1307325 — Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their locks

On Tue, Jan 12, 2016 at 11:21:25AM +0000, Juri Lelli wrote:
> I tried to see if something like for_each_domain() can be done, but here
> we use list_for_each_entry() macro. Peter, do you mean something like
> the following?
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 78b1e2f..1a847a6 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -39,6 +39,7 @@
>  static LIST_HEAD(cpufreq_governor_list);
>  static DEFINE_MUTEX(cpufreq_governor_mutex);
>  #define for_each_governor(__governor)				\
> +	lockdep_assert_held(&cpufreq_governor_mutex);		\
>  	list_for_each_entry(__governor, &cpufreq_governor_list, governor_list)

That fails for things like:

	if (blah)
		for_each_governor(...) {
		}

which looks like valid C -- even though our Coding Style says the if
should have { } on.

I was thinking of either open coding the for statement and adding it to
the first statement like:

	#define for_each_governor(__g) \
		for (_g = list_first_entry(&cpufreq_governor_list, typeof(*_g), governor_list, lockdep_assert_held(), \
		     ..... )

Or use something like this:

  lkml.kernel.org/r/20150422154212.GE3007@worktop.Skamania.guest

	#define for_each_governor(_g) \
		list_for_each_entry(_g, &cpufreq_governor_list, governor_list)
			if (lockdep_assert_held(..), false)
				;
			else

Which should preserve C syntax rules.

[toc] | [prev] | [next] | [standalone]

#1307362 — Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their locks

On 12/01/16 12:58, Peter Zijlstra wrote:
> On Tue, Jan 12, 2016 at 11:21:25AM +0000, Juri Lelli wrote:
> > I tried to see if something like for_each_domain() can be done, but here
> > we use list_for_each_entry() macro. Peter, do you mean something like
> > the following?
> > 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 78b1e2f..1a847a6 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -39,6 +39,7 @@
> >  static LIST_HEAD(cpufreq_governor_list);
> >  static DEFINE_MUTEX(cpufreq_governor_mutex);
> >  #define for_each_governor(__governor)				\
> > +	lockdep_assert_held(&cpufreq_governor_mutex);		\
> >  	list_for_each_entry(__governor, &cpufreq_governor_list, governor_list)
> 
> That fails for things like:
> 
> 	if (blah)
> 		for_each_governor(...) {
> 		}
> 
> which looks like valid C -- even though our Coding Style says the if
> should have { } on.
> 
> I was thinking of either open coding the for statement and adding it to
> the first statement like:
> 
> 	#define for_each_governor(__g) \
> 		for (_g = list_first_entry(&cpufreq_governor_list, typeof(*_g), governor_list, lockdep_assert_held(), \
> 		     ..... )
> 
> Or use something like this:
> 
>   lkml.kernel.org/r/20150422154212.GE3007@worktop.Skamania.guest
> 
> 	#define for_each_governor(_g) \
> 		list_for_each_entry(_g, &cpufreq_governor_list, governor_list)
> 			if (lockdep_assert_held(..), false)
> 				;
> 			else
> 
> Which should preserve C syntax rules.
> 

Oh, nice this! I'll try it.

Thanks,

- Juri

[toc] | [prev] | [next] | [standalone]

#1307548 — Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their locks

On 12/01/16 12:36, Juri Lelli wrote:
> On 12/01/16 12:58, Peter Zijlstra wrote:
> > On Tue, Jan 12, 2016 at 11:21:25AM +0000, Juri Lelli wrote:
> > > I tried to see if something like for_each_domain() can be done, but here
> > > we use list_for_each_entry() macro. Peter, do you mean something like
> > > the following?
> > > 
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index 78b1e2f..1a847a6 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -39,6 +39,7 @@
> > >  static LIST_HEAD(cpufreq_governor_list);
> > >  static DEFINE_MUTEX(cpufreq_governor_mutex);
> > >  #define for_each_governor(__governor)				\
> > > +	lockdep_assert_held(&cpufreq_governor_mutex);		\
> > >  	list_for_each_entry(__governor, &cpufreq_governor_list, governor_list)
> > 
> > That fails for things like:
> > 
> > 	if (blah)
> > 		for_each_governor(...) {
> > 		}
> > 
> > which looks like valid C -- even though our Coding Style says the if
> > should have { } on.
> > 
> > I was thinking of either open coding the for statement and adding it to
> > the first statement like:
> > 
> > 	#define for_each_governor(__g) \
> > 		for (_g = list_first_entry(&cpufreq_governor_list, typeof(*_g), governor_list, lockdep_assert_held(), \
> > 		     ..... )
> > 
> > Or use something like this:
> > 
> >   lkml.kernel.org/r/20150422154212.GE3007@worktop.Skamania.guest
> > 
> > 	#define for_each_governor(_g) \
> > 		list_for_each_entry(_g, &cpufreq_governor_list, governor_list)
> > 			if (lockdep_assert_held(..), false)
> > 				;
> > 			else
> > 
> > Which should preserve C syntax rules.
> > 
> 
> Oh, nice this! I'll try it.
> 

This second approach doesn't really play well with lockdep_assert_held
definition, right?

However, it seems I could make this work with

 #ifdef CONFIG_LOCKDEP
 #define for_each_governor(__gov)					    \
 	for (__gov = list_first_entry(&cpufreq_governor_list, 		    \
 				      typeof(*__gov), 			    \
 				      governor_list),			    \
 				WARN_ON(debug_locks &&			    \
 				!lockdep_is_held(&cpufreq_governor_mutex)); \
 	     &__gov->governor_list != (&cpufreq_governor_list);		    \
 	     __gov = list_next_entry(__gov, governor_list))
 #else /* !CONFIG_LOCKDEP */
 #define for_each_governor(__gov)					    \
 	list_for_each_entry(__gov, &cpufreq_governor_list, governor_list)
 #endif /* CONFIG_LOCKDEP */

Thanks,

- Juri

[toc] | [prev] | [next] | [standalone]

#1307578 — Re: [RFC PATCH 04/19] cpufreq: bring data structures close to their locks

On Tue, Jan 12, 2016 at 03:26:01PM +0000, Juri Lelli wrote:
> > > 	#define for_each_governor(_g) \
> > > 		list_for_each_entry(_g, &cpufreq_governor_list, governor_list)
> > > 			if (lockdep_assert_held(..), false)
> > > 				;
> > > 			else
> > > 
> > > Which should preserve C syntax rules.
> > > 
> > 
> > Oh, nice this! I'll try it.
> > 
> 
> This second approach doesn't really play well with lockdep_assert_held
> definition, right?

Right, the below however makes it work, except:

../kernel/sched/core.c: In function ‘scheduler_ipi’:
../kernel/sched/core.c:1831:32: warning: left-hand operand of comma expression has no effect [-Wunused-value]
  if (lockdep_assert_held(&lock), false)

Which is of course correct and very much on purpose :/

---

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index c57e424d914b..caf7a89643d8 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -362,9 +362,9 @@ extern void lock_unpin_lock(struct lockdep_map *lock);
 
 #define lockdep_depth(tsk)	(debug_locks ? (tsk)->lockdep_depth : 0)
 
-#define lockdep_assert_held(l)	do {				\
+#define lockdep_assert_held(l)	({				\
 		WARN_ON(debug_locks && !lockdep_is_held(l));	\
-	} while (0)
+		(void)l; })
 
 #define lockdep_assert_held_once(l)	do {				\
 		WARN_ON_ONCE(debug_locks && !lockdep_is_held(l));	\
@@ -422,7 +422,7 @@ struct lock_class_key { };
 
 #define lockdep_depth(tsk)	(0)
 
-#define lockdep_assert_held(l)			do { (void)(l); } while (0)
+#define lockdep_assert_held(l)			({ (void)l; })
 #define lockdep_assert_held_once(l)		do { (void)(l); } while (0)
 
 #define lockdep_recursing(tsk)			(0)
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 77d97a6fc715..f6f36217133d 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1817,6 +1817,8 @@ void sched_ttwu_pending(void)
 	raw_spin_unlock_irqrestore(&rq->lock, flags);
 }
 
+raw_spinlock_t lock;
+
 void scheduler_ipi(void)
 {
 	/*
@@ -1826,6 +1828,9 @@ void scheduler_ipi(void)
 	 */
 	preempt_fold_need_resched();
 
+	if (lockdep_assert_held(&lock), false)
+		;
+
 	if (llist_empty(&this_rq()->wake_list) && !got_nohz_idle_kick())
 		return;
 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web