Re: [Qemu-devel] [PATCH v4 1/5] cpu: Provide vcpu throttling interface

["Jason J. Herne" <jjherne@linux.vnet.ibm.com>]

On 07/23/2015 05:59 AM, Paolo Bonzini wrote:
>
>
> On 16/07/2015 16:21, Jason J. Herne wrote:
>> 1. Using atomic operations to manage throttle_percentage. I'm not sure
>> where atomics are applicable here. If this is still a concern hopefully
>> someone can explain.
>
> I would use atomic_read/atomic_set in cpu_throttle_set,
> cpu_throttle_stop, cpu_throttle_active, cpu_throttle_get_percentage.
> In addition, the function naming seems to be a bit inconsistent: please
> rename cpu_throttle_set to cpu_throttle_set_percentage.
>
> Second, here:
>
>>> +static void cpu_throttle_thread(void *opaque)
>>> +{
>>> + double pct = (double)throttle_percentage/100;
>
> Please use cpu_throttle_get_percentage(), and
>
>>> + double throttle_ratio = pct / (1 - pct);
>>> + long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);
>
> ... move these computations below the if.
>
> I'm also not sure about throttle_ratio, why is it needed?  If pct >= 0.5 you
> end up with throttle_ratio >= 1, i.e. no way for the CPU to do any work.  This
> would definitely cause a problem with callbacks piling up.
>

Throttle ratio is relative to CPU_THROTTLE_TIMESLICE. Take a look at how
throttle_ratio is used in the calculation:

long sleeptime_ms = (long)(throttle_ratio * CPU_THROTTLE_TIMESLICE);

A value of 1 means we sleep the same amount of time that we execute.

>>> + if (!throttle_percentage) {
>>> + return;
>>> + }
>>> +
>>> + qemu_mutex_unlock_iothread();
>>> + g_usleep(sleeptime_ms * 1000); /* Convert ms to us for usleep call */
>>> + qemu_mutex_lock_iothread();
>>> +}
>>> +
>
>> 2. Callback stacking. And it seems like we are convinced that it is not
>> a big issue. Anyone disagree?
>
> I think it's not a big issue to have many timers, but it is a big issue to have many callbacks.  What I suggested is this:
>
>      if (!atomic_xchg(&cpu->throttle_thread_scheduled, 1)) {
>          async_run_on_cpu(cpu, cpu_throttle_thread, NULL);
>      }
>
> and in the callback:
>
>      atomic_set(&cpu->throttle_thread_scheduled, 0);
>      g_usleep(...);
>
> Paolo
>
>

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)