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

Groups > linux.kernel > #1312267 > unrolled thread

Re: [RFC PATCH 18/19] cpufreq: remove transition_lock

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

  Re: [RFC PATCH 18/19] cpufreq: remove transition_lock Peter Zijlstra <peterz@infradead.org> - 2016-01-19 15:10 +0100
    Re: [RFC PATCH 18/19] cpufreq: remove transition_lock Juri Lelli <juri.lelli@arm.com> - 2016-01-19 15:50 +0100
      Re: [RFC PATCH 18/19] cpufreq: remove transition_lock Peter Zijlstra <peterz@infradead.org> - 2016-01-19 16:40 +0100
        Re: [RFC PATCH 18/19] cpufreq: remove transition_lock Juri Lelli <juri.lelli@arm.com> - 2016-01-19 17:10 +0100
          Re: [RFC PATCH 18/19] cpufreq: remove transition_lock Peter Zijlstra <peterz@infradead.org> - 2016-01-19 20:20 +0100
            Re: [RFC PATCH 18/19] cpufreq: remove transition_lock Peter Zijlstra <peterz@infradead.org> - 2016-01-19 20:30 +0100
              Re: [RFC PATCH 18/19] cpufreq: remove transition_lock "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-01-19 23:00 +0100
                Re: [RFC PATCH 18/19] cpufreq: remove transition_lock Peter Zijlstra <peterz@infradead.org> - 2016-01-20 18:10 +0100
                  Re: [RFC PATCH 18/19] cpufreq: remove transition_lock "Rafael J. Wysocki" <rafael@kernel.org> - 2016-01-20 23:20 +0100
                    Re: [RFC PATCH 18/19] cpufreq: remove transition_lock Peter Zijlstra <peterz@infradead.org> - 2016-01-20 23:40 +0100
                      Re: [RFC PATCH 18/19] cpufreq: remove transition_lock "Rafael J. Wysocki" <rafael@kernel.org> - 2016-01-21 00:40 +0100
              Re: [RFC PATCH 18/19] cpufreq: remove transition_lock Juri Lelli <juri.lelli@arm.com> - 2016-01-20 14:00 +0100

#1312267 — Re: [RFC PATCH 18/19] cpufreq: remove transition_lock

On Wed, Jan 13, 2016 at 10:21:31AM -0800, Michael Turquette wrote:
> RCU is absolutely not a magic bullet or elixir that lets us kick off
> DVFS transitions from the schedule() context. The frequency transitions
> are write-side operations, as we invariably touch struct cpufreq_policy.
> This means that the read-side stuff can live in the schedule() context,
> but write-side needs to be kicked out to a thread.

Why? If the state is per-cpu and acquired by RCU, updates should be no
problem at all.

If you need inter-cpu state, then things get to be a little tricky
though, but you can actually nest a raw_spinlock_t in there if you
absolutely have to.

[toc] | [next] | [standalone]

#1312306

On 19/01/16 15:00, Peter Zijlstra wrote:
> On Wed, Jan 13, 2016 at 10:21:31AM -0800, Michael Turquette wrote:
> > RCU is absolutely not a magic bullet or elixir that lets us kick off
> > DVFS transitions from the schedule() context. The frequency transitions
> > are write-side operations, as we invariably touch struct cpufreq_policy.
> > This means that the read-side stuff can live in the schedule() context,
> > but write-side needs to be kicked out to a thread.
> 
> Why? If the state is per-cpu and acquired by RCU, updates should be no
> problem at all.
> 
> If you need inter-cpu state, then things get to be a little tricky
> though, but you can actually nest a raw_spinlock_t in there if you
> absolutely have to.
> 

We have at least two problems. First one is that state is per frequency
domain (struct cpufreq_policy) and this usually spans more than one cpu.
Second one is that we might need to sleep while servicing the frequency
transition, both because platform needs to sleep and because some paths
of cpufreq core use sleeping locks (yes, that might be changed as well I
guess).  A solution based on spinlocks only might not be usable on
platforms that needs to sleep, also.

Another thing that I was thinking of actually is that since struct
cpufreq_policy is updated a lot (more or less at every frequency
transition), is it actually suitable for RCU?

Best,

- Juri

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

#1312334

On Tue, Jan 19, 2016 at 02:42:33PM +0000, Juri Lelli wrote:
> On 19/01/16 15:00, Peter Zijlstra wrote:
> > On Wed, Jan 13, 2016 at 10:21:31AM -0800, Michael Turquette wrote:
> > > RCU is absolutely not a magic bullet or elixir that lets us kick off
> > > DVFS transitions from the schedule() context. The frequency transitions
> > > are write-side operations, as we invariably touch struct cpufreq_policy.
> > > This means that the read-side stuff can live in the schedule() context,
> > > but write-side needs to be kicked out to a thread.
> > 
> > Why? If the state is per-cpu and acquired by RCU, updates should be no
> > problem at all.
> > 
> > If you need inter-cpu state, then things get to be a little tricky
> > though, but you can actually nest a raw_spinlock_t in there if you
> > absolutely have to.
> > 
> 
> We have at least two problems. First one is that state is per frequency
> domain (struct cpufreq_policy) and this usually spans more than one cpu.
> Second one is that we might need to sleep while servicing the frequency
> transition, both because platform needs to sleep and because some paths
> of cpufreq core use sleeping locks (yes, that might be changed as well I
> guess).  A solution based on spinlocks only might not be usable on
> platforms that needs to sleep, also.

Sure, if you need to actually sleep to poke the hardware you've lost and
you do indeed need the kthread thingy.

> Another thing that I was thinking of actually is that since struct
> cpufreq_policy is updated a lot (more or less at every frequency
> transition), is it actually suitable for RCU?

That entirely depends on how 'hard' it is to 'replace/change' the
cpufreq policy.

Typically I envision that to be very hard and require mutexes and the
like, in which case RCU can provide a cheap lookup and existence.

So on 'sane' hardware with per logical cpu hints you can get away
without any locks.

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

#1312360

On 19/01/16 16:30, Peter Zijlstra wrote:
> On Tue, Jan 19, 2016 at 02:42:33PM +0000, Juri Lelli wrote:
> > On 19/01/16 15:00, Peter Zijlstra wrote:
> > > On Wed, Jan 13, 2016 at 10:21:31AM -0800, Michael Turquette wrote:
> > > > RCU is absolutely not a magic bullet or elixir that lets us kick off
> > > > DVFS transitions from the schedule() context. The frequency transitions
> > > > are write-side operations, as we invariably touch struct cpufreq_policy.
> > > > This means that the read-side stuff can live in the schedule() context,
> > > > but write-side needs to be kicked out to a thread.
> > > 
> > > Why? If the state is per-cpu and acquired by RCU, updates should be no
> > > problem at all.
> > > 
> > > If you need inter-cpu state, then things get to be a little tricky
> > > though, but you can actually nest a raw_spinlock_t in there if you
> > > absolutely have to.
> > > 
> > 
> > We have at least two problems. First one is that state is per frequency
> > domain (struct cpufreq_policy) and this usually spans more than one cpu.
> > Second one is that we might need to sleep while servicing the frequency
> > transition, both because platform needs to sleep and because some paths
> > of cpufreq core use sleeping locks (yes, that might be changed as well I
> > guess).  A solution based on spinlocks only might not be usable on
> > platforms that needs to sleep, also.
> 
> Sure, if you need to actually sleep to poke the hardware you've lost and
> you do indeed need the kthread thingy.
> 

Yeah, also cpufreq relies on blocking notifiers (to name one thing). So,
it seems to me quite some things needs to be changed to make it fully
non sleeping.

> > Another thing that I was thinking of actually is that since struct
> > cpufreq_policy is updated a lot (more or less at every frequency
> > transition), is it actually suitable for RCU?
> 
> That entirely depends on how 'hard' it is to 'replace/change' the
> cpufreq policy.
> 
> Typically I envision that to be very hard and require mutexes and the
> like, in which case RCU can provide a cheap lookup and existence.
> 

Right, read path is fast, but write path still requires some sort of
locking (malloc, copy and update). So, I'm wondering if this still pays
off for a structure that gets written a lot.

> So on 'sane' hardware with per logical cpu hints you can get away
> without any locks.
> 

But maybe you are saying that there are ways we can make that work :).

Thanks,

- Juri

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

#1312459

On Tue, Jan 19, 2016 at 04:01:55PM +0000, Juri Lelli wrote:
> Right, read path is fast, but write path still requires some sort of
> locking (malloc, copy and update). So, I'm wondering if this still pays
> off for a structure that gets written a lot.

No, not at all.

struct cpufreq_driver *driver;

void sched_util_change(unsigned int util)
{
	struct my_per_cpu_data *foo;

	rcu_read_lock();
	foo = __this_cpu_ptr(rcu_dereference(driver)->data);
	if (foo) {
		if (abs(util - foo->last_util) > 10) {
			foo->last_util = util;
			foo->set_util(util);
		}
	}
	rcu_read_unlock();
}


struct cpufreq_driver *cpufreq_flip_driver(struct cpufreq_driver *new_driver)
{
	struct cpufreq_driver *old_driver;

	mutex_lock(&cpufreq_driver_lock);
	old_driver = driver;
	rcu_assign_driver(driver, new_driver);
	if (old_driver)
		synchronize_rcu();
	mutex_unlock(&cpufreq_driver_lock);

	return old_driver;
}

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

#1312464

On Tue, Jan 19, 2016 at 08:17:34PM +0100, Peter Zijlstra wrote:
> On Tue, Jan 19, 2016 at 04:01:55PM +0000, Juri Lelli wrote:
> > Right, read path is fast, but write path still requires some sort of
> > locking (malloc, copy and update). So, I'm wondering if this still pays
> > off for a structure that gets written a lot.
> 
> No, not at all.
> 
> struct cpufreq_driver *driver;
> 
> void sched_util_change(unsigned int util)
> {
> 	struct my_per_cpu_data *foo;
> 
> 	rcu_read_lock();

That should obviously be:

	d = rcu_dereference(driver);
	if (d) {
		foo = __this_cpu_ptr(d->data);

> 		if (abs(util - foo->last_util) > 10) {
> 			foo->last_util = util;
> 			foo->set_util(util);
> 		}
> 	}
> 	rcu_read_unlock();
> }
> 
> 
> struct cpufreq_driver *cpufreq_flip_driver(struct cpufreq_driver *new_driver)
> {
> 	struct cpufreq_driver *old_driver;
> 
> 	mutex_lock(&cpufreq_driver_lock);
> 	old_driver = driver;
> 	rcu_assign_driver(driver, new_driver);
> 	if (old_driver)
> 		synchronize_rcu();
> 	mutex_unlock(&cpufreq_driver_lock);
> 
> 	return old_driver;
> }
> 
> 
> 

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

#1312541

On Tuesday, January 19, 2016 08:21:11 PM Peter Zijlstra wrote:
> On Tue, Jan 19, 2016 at 08:17:34PM +0100, Peter Zijlstra wrote:
> > On Tue, Jan 19, 2016 at 04:01:55PM +0000, Juri Lelli wrote:
> > > Right, read path is fast, but write path still requires some sort of
> > > locking (malloc, copy and update). So, I'm wondering if this still pays
> > > off for a structure that gets written a lot.
> > 
> > No, not at all.
> > 

This is very similar to what I was thinking about, plus-minus a couple of
things.

> > struct cpufreq_driver *driver;
> > 
> > void sched_util_change(unsigned int util)
> > {
> > 	struct my_per_cpu_data *foo;
> > 
> > 	rcu_read_lock();
> 
> That should obviously be:
> 
> 	d = rcu_dereference(driver);
> 	if (d) {
> 		foo = __this_cpu_ptr(d->data);

If we do this, it would be convenient to define ->set_util() to take
foo as an arg too, in addition to util.

And is there any particular reason why d->data has to be per-cpu?

> 
> > 		if (abs(util - foo->last_util) > 10) {

Even if the utilization doesn't change, it still may be too high or too low,
so we may want to call foo->set_util() in that case too, at least once a
while.

> > 			foo->last_util = util;
> > 			foo->set_util(util);
> > 		}
> > 	}
> > 	rcu_read_unlock();
> > }
> > 
> > 
> > struct cpufreq_driver *cpufreq_flip_driver(struct cpufreq_driver *new_driver)
> > {
> > 	struct cpufreq_driver *old_driver;
> > 
> > 	mutex_lock(&cpufreq_driver_lock);
> > 	old_driver = driver;
> > 	rcu_assign_driver(driver, new_driver);
> > 	if (old_driver)
> > 		synchronize_rcu();
> > 	mutex_unlock(&cpufreq_driver_lock);
> > 
> > 	return old_driver;
> > }

We never need to do this, because we never replace one driver with another in
one go.  We need to go from a valid driver pointer to NULL and the other way
around only.

This means there may be other pointers around that may be accessed safely
from foo->set_util() above if there's a rule that they must be set before
the driver pointer and the data structures they point to must stay around
until the syncronize_rcu() returns.

Thanks,
Rafael

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

#1313380

On Tue, Jan 19, 2016 at 10:52:22PM +0100, Rafael J. Wysocki wrote:
> This is very similar to what I was thinking about, plus-minus a couple of
> things.
> 
> > > struct cpufreq_driver *driver;
> > > 
> > > void sched_util_change(unsigned int util)
> > > {
> > > 	struct my_per_cpu_data *foo;
> > > 
> > > 	rcu_read_lock();
> > 
> > That should obviously be:
> > 
> > 	d = rcu_dereference(driver);
> > 	if (d) {
> > 		foo = __this_cpu_ptr(d->data);
> 
> If we do this, it would be convenient to define ->set_util() to take
> foo as an arg too, in addition to util.
> 
> And is there any particular reason why d->data has to be per-cpu?

Seems sensible, at best it actually is per cpu data, at worst this per
cpu pointer points to the same data for multiple cpus (the freq domain).

> > 
> > > 		if (abs(util - foo->last_util) > 10) {
> 
> Even if the utilization doesn't change, it still may be too high or too low,
> so we may want to call foo->set_util() in that case too, at least once a
> while.
> 
> > > 			foo->last_util = util;

Ah, the whole point of this was that ^^^ store.

Modifying the data structure doesn't need a new alloc / copy etc.. We
only use RCU to guarantee the data exists, once we have the data, the
data itself can be modified however.

Here its strictly per-cpu data, so modifying it can be unserialized
since CPUs themselves are sequentially consistent.

If you have a freq domain with multiple CPUs in, you'll have to go stick
a lock in.

> > > 			foo->set_util(util);
> > > 		}
> > > 	}
> > > 	rcu_read_unlock();
> > > }
> > > 
> > > 
> > > struct cpufreq_driver *cpufreq_flip_driver(struct cpufreq_driver *new_driver)
> > > {
> > > 	struct cpufreq_driver *old_driver;
> > > 
> > > 	mutex_lock(&cpufreq_driver_lock);
> > > 	old_driver = driver;
> > > 	rcu_assign_driver(driver, new_driver);
> > > 	if (old_driver)
> > > 		synchronize_rcu();
> > > 	mutex_unlock(&cpufreq_driver_lock);
> > > 
> > > 	return old_driver;
> > > }
> 
> We never need to do this, because we never replace one driver with another in
> one go.  We need to go from a valid driver pointer to NULL and the other way
> around only.

The above can do those transitions :-)

> This means there may be other pointers around that may be accessed safely
> from foo->set_util() above if there's a rule that they must be set before
> the driver pointer and the data structures they point to must stay around
> until the syncronize_rcu() returns.

I would dangle _everything_ off the one driver pointer, that's much
easier.

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

#1313612

On Wed, Jan 20, 2016 at 6:04 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Jan 19, 2016 at 10:52:22PM +0100, Rafael J. Wysocki wrote:
>> This is very similar to what I was thinking about, plus-minus a couple of
>> things.
>>
>> > > struct cpufreq_driver *driver;
>> > >
>> > > void sched_util_change(unsigned int util)
>> > > {
>> > >   struct my_per_cpu_data *foo;
>> > >
>> > >   rcu_read_lock();
>> >
>> > That should obviously be:
>> >
>> >     d = rcu_dereference(driver);
>> >     if (d) {
>> >             foo = __this_cpu_ptr(d->data);
>>
>> If we do this, it would be convenient to define ->set_util() to take
>> foo as an arg too, in addition to util.
>>
>> And is there any particular reason why d->data has to be per-cpu?
>
> Seems sensible, at best it actually is per cpu data, at worst this per
> cpu pointer points to the same data for multiple cpus (the freq domain).
>
>> >
>> > >           if (abs(util - foo->last_util) > 10) {
>>
>> Even if the utilization doesn't change, it still may be too high or too low,
>> so we may want to call foo->set_util() in that case too, at least once a
>> while.
>>
>> > >                   foo->last_util = util;
>
> Ah, the whole point of this was that ^^^ store.

OK, I see.

> Modifying the data structure doesn't need a new alloc / copy etc.. We
> only use RCU to guarantee the data exists, once we have the data, the
> data itself can be modified however.
>
> Here its strictly per-cpu data, so modifying it can be unserialized
> since CPUs themselves are sequentially consistent.

Right.  So that's why you want it to be per-cpu really.

> If you have a freq domain with multiple CPUs in, you'll have to go stick
> a lock in.

Right.

>> > >                   foo->set_util(util);
>> > >           }
>> > >   }
>> > >   rcu_read_unlock();
>> > > }
>> > >
>> > >
>> > > struct cpufreq_driver *cpufreq_flip_driver(struct cpufreq_driver *new_driver)
>> > > {
>> > >   struct cpufreq_driver *old_driver;
>> > >
>> > >   mutex_lock(&cpufreq_driver_lock);
>> > >   old_driver = driver;
>> > >   rcu_assign_driver(driver, new_driver);
>> > >   if (old_driver)
>> > >           synchronize_rcu();
>> > >   mutex_unlock(&cpufreq_driver_lock);
>> > >
>> > >   return old_driver;
>> > > }
>>
>> We never need to do this, because we never replace one driver with another in
>> one go.  We need to go from a valid driver pointer to NULL and the other way
>> around only.
>
> The above can do those transitions :-)

Yes, it can, but the real thing will probably be more complicated than
the code above and then the difference may actually matter.

>> This means there may be other pointers around that may be accessed safely
>> from foo->set_util() above if there's a rule that they must be set before
>> the driver pointer and the data structures they point to must stay around
>> until the syncronize_rcu() returns.
>
> I would dangle _everything_ off the one driver pointer, that's much
> easier.

I'm not sure how much easier it is in practice.

Even if everything dangles out of the driver pointer, data structures
pointed to by those things need not be allocated all in one go by the
same entity.  Some of them are allocated by drivers, some of them by
the core, at different times.  The ordering between those allocations
and populating the pointers is what matters, not how all that is laid
out in memory.

Thanks,
Rafael

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

#1313635

On Wed, Jan 20, 2016 at 11:12:45PM +0100, Rafael J. Wysocki wrote:
> > I would dangle _everything_ off the one driver pointer, that's much
> > easier.
> 
> I'm not sure how much easier it is in practice.
> 
> Even if everything dangles out of the driver pointer, data structures
> pointed to by those things need not be allocated all in one go by the
> same entity.  Some of them are allocated by drivers, some of them by
> the core, at different times.

Yes, I've noticed, some of that is really bonkers.

> The ordering between those allocations
> and populating the pointers is what matters, not how all that is laid
> out in memory.

I'm thinking getting that ordering right is easier/more natural, if its
all contained in one object. But this could be subjective.

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

#1313706

On Wed, Jan 20, 2016 at 11:38 PM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Jan 20, 2016 at 11:12:45PM +0100, Rafael J. Wysocki wrote:
>> > I would dangle _everything_ off the one driver pointer, that's much
>> > easier.
>>
>> I'm not sure how much easier it is in practice.
>>
>> Even if everything dangles out of the driver pointer, data structures
>> pointed to by those things need not be allocated all in one go by the
>> same entity.  Some of them are allocated by drivers, some of them by
>> the core, at different times.
>
> Yes, I've noticed, some of that is really bonkers.
>
>> The ordering between those allocations
>> and populating the pointers is what matters, not how all that is laid
>> out in memory.
>
> I'm thinking getting that ordering right is easier/more natural, if its
> all contained in one object. But this could be subjective.

I'm trying to look at this from the perspective of making changes.

It should be possible to change the ordering of how the data
structures are populated and pointers set without changing the
existing memory layout of them, which may allow us to minimize the
amount of changes to cpufreq drivers for old hardware (and therefore
generally difficult to test), for example.

Also, this way each individual change may be more limited in scope and
therefore less error prone IMO.

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

#1313202

On 19/01/16 20:21, Peter Zijlstra wrote:
> On Tue, Jan 19, 2016 at 08:17:34PM +0100, Peter Zijlstra wrote:
> > On Tue, Jan 19, 2016 at 04:01:55PM +0000, Juri Lelli wrote:
> > > Right, read path is fast, but write path still requires some sort of
> > > locking (malloc, copy and update). So, I'm wondering if this still pays
> > > off for a structure that gets written a lot.
> > 
> > No, not at all.
> > 
> > struct cpufreq_driver *driver;
> > 
> > void sched_util_change(unsigned int util)
> > {
> > 	struct my_per_cpu_data *foo;
> > 
> > 	rcu_read_lock();
> 
> That should obviously be:
> 
> 	d = rcu_dereference(driver);
> 	if (d) {
> 		foo = __this_cpu_ptr(d->data);
> 
> > 		if (abs(util - foo->last_util) > 10) {
> > 			foo->last_util = util;
> > 			foo->set_util(util);
> > 		}
> > 	}
> > 	rcu_read_unlock();
> > }
> > 
> > 
> > struct cpufreq_driver *cpufreq_flip_driver(struct cpufreq_driver *new_driver)
> > {
> > 	struct cpufreq_driver *old_driver;
> > 
> > 	mutex_lock(&cpufreq_driver_lock);
> > 	old_driver = driver;
> > 	rcu_assign_driver(driver, new_driver);
> > 	if (old_driver)
> > 		synchronize_rcu();
> > 	mutex_unlock(&cpufreq_driver_lock);
> > 
> > 	return old_driver;
> > }
> > 
> > 
> > 
> 

Right, this addresses the driver side (modulo what Rafael pointed out
about setting driver pointer to NULL and then to point to the new
driver); and for this part I think RCU works well. I'm not concerned
about the driver side :).

Now, assuming that we move cpufreq_cpu_data inside cpufreq_driver (IIUC
this is your d->data), we will have per_cpu pointers pointing to the
different policies. Inside these policy data structures we have
information regarding current frequency, maximum allowed frequency, cpus
covered by this policy, and a few more. IIUC this is your foo thing.
Since the structure pointed to by foo will be shared amongs several
cpus, we need some way to guarantee mutual exclusion and such. I think
we were thinking to use RCU for this bit as well and that is what I'm
concerned about, as curr frequency will change at every frequency
transition.

Maybe you are also implying that we need to change cpufreq_cpu_data as
well. I need to think more about that.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web