Re: CPUfreq - udelay() interaction issues

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

[Index of Archives]     [Linux Kernel Devel]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Forum]     [Linux SCSI]

  Powered by Linux