Hi Mathieu,
Thanks for taking the time to provide your input. More responses below.
Mathieu Desnoyers wrote:
* Saravana Kannan (skannan@xxxxxxxxxxxxxx) wrote:
[...]
Seems a bit more complicated than what I had in mind. This is touching
the scheduler I think we can get away without having to. Also, there is
no simple implementation for the "slowpath" that can guarantee the delay
without starting over the loop and hoping not to get interrupted or just
giving up and doing a massively inaccurate delay (like msleep, etc).
Not necessarily. Another way to do it: we could keep the udelay loop counter in
the task struct. When ondemand changes frequency, and upon migration, this
counter would be adapted to the current cpu frequency.
This will take us back to the scalability problem because we now have to
go through every process running on a CPU to update their udelay loop
counters whenever the CPU freq changes.
I was thinking of something along the lines of this:
udelay()
{
if (!is_atomic())
see hardirq.h:
/*
* Are we running in atomic context? WARNING: this macro cannot
* always detect atomic context; in particular, it cannot know about
* held spinlocks in non-preemptible kernels. Thus it should not be
* used in the general case to determine whether sleeping is possible.
* Do not use in_atomic() in driver code.
*/
#define in_atomic() ((preempt_count() & ~PREEMPT_ACTIVE) != PREEMPT_INATOMIC_BASE)
Sorry, your scheme is broken on !PREEMPT kernels.
If it's a !PREEMPT kernel, we don't have to worry about the CPUfreq
changing on us. CPU freq is changed in a deferrable work queue context.
down_read(&freq_sem);
/* else
do nothing since cpufreq can't interrupt you.
*/
This comment seems broken. in_atomic() can return true because preemption is
disabled, thus letting cpufreq interrupts coming in.
As mentioned earlier, cpufreq change can't happen when udelay is running
in !PREEMPT kernel (which is where in_atomic() won't work). Btw, I
actually wasn't referring to the real in_atomic() macro (I remembered it
having limitations). But now that you mentioned the limitation, it might
not be a problem after all.
call usual code since cpufreq is not going to preempt you.
if (!is_atomic())
up_read(&freq_sem);
}
__cpufreq_driver_target(...)
{
down_write(&freq_sem);
cpufreq_driver->target(...);
up_write(&freq_sem);
}
In the implementation of the cpufreq driver, they just need to make sure
they always increase the LPJ _before_ increasing the freq and decrease
the LPJ _after_ decreasing the freq. This is make sure that when an
interrupt handler preempts the cpufreq driver code (since atomic
contexts aren't looking at the r/w semaphore) the LPJ value will be good
enough to satisfy the _at least_ guarantee of udelay().
For the CPU switching issue, I think the solution I proposed is quite
simple and should work.
You mean this ?
udelay(us)
{
set cpu affinity to current CPU;
Do the usual udelay code;
restore cpu affinity status;
}
Things like lock scalability and performance degradations comes to my mind. We
can expect some drivers to make very heavy use of udelay(). This should not
bring a 4096-core box to its knees. sched_setaffinity() is very far from being
lightweight, as it locks cpu hotplug (that's a global mutex protecting a
refcount), allocates memory, manipulates cpumasks, etc...
Hmm... set affinity does seem more complicated than what I expected.
Does my better explained solution look palatable?
Nope, not on a multiprocessor system.
Yes, set affinity seems to be a problem.
Didn't get to work on this for the past few days. Let me think more
about this before I get back. In the mean time, if you can come up with
a relatively simple solution without scalability issues, I would be glad
to drop my existing solution.
Thanks again for the input.
-Saravana
--
To unsubscribe from this list: send the line "unsubscribe cpufreq" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
