Re: [PATCH v14 6/7] softmmu/dirtylimit: Implement virtual CPU throttle

[Hyman Huang <huangy81@chinatelecom.cn>]

在 2022/2/14 16:20, Peter Xu 写道:
> On Fri, Feb 11, 2022 at 12:17:40AM +0800, huangy81@chinatelecom.cn wrote:
>> @@ -2964,8 +2971,13 @@ int kvm_cpu_exec(CPUState *cpu)
>>                */
>>               trace_kvm_dirty_ring_full(cpu->cpu_index);
>>               qemu_mutex_lock_iothread();
>> -            kvm_dirty_ring_reap(kvm_state, NULL);
>> +            if (dirtylimit_in_service()) {
>> +                kvm_dirty_ring_reap(kvm_state, cpu);
>> +            } else {
>> +                kvm_dirty_ring_reap(kvm_state, NULL);
>> +            }
> 
> Could you add some comment here on why the cpu pointer is conditionally passed
> into the reaping routine?  Even if we know it now, it's not immediately obvious
> to all the readers.
Sure.
> 
> [...]
> 
>> +struct {
>> +    VcpuDirtyLimitState *states;
>> +    /* Max cpus number configured by user */
>> +    int max_cpus;
>> +    /* Number of vcpu under dirtylimit */
>> +    int limited_nvcpu;
>> +    /* Function to implement throttle set up */
>> +    DirtyLimitFunc setup;
> 
> "setup" normally is used only at startup of something, but not per interval.
> Perhaps "process" or "adjust"?  Same across other "setup" namings across the
> patch.
Ok, 'adjust' is fine.
> 
> Again, I'd rather call the function directly..
Um, maybe using the function pointer is more extensible.

[...]
static void *vcpu_dirty_rate_stat_thread(void *opaque)
{
     rcu_register_thread();

     /* start log sync */
     global_dirty_log_change(GLOBAL_DIRTY_LIMIT, true);

     while (qatomic_read(&vcpu_dirty_rate_stat->running)) {
         vcpu_dirty_rate_stat_collect();
         if (dirtylimit_in_service() &&
             dirtylimit_state->setup) {
             dirtylimit_state->setup();
         }
     }

     /* stop log sync */
     global_dirty_log_change(GLOBAL_DIRTY_LIMIT, false);

     rcu_unregister_thread();
     return NULL;
}
[...]

Function pointer makes the 'dirtyrate-stat' logic and 'dirtylimit' logic 
kind of decoupled.

But i'm ok if you insist because it's just about how to call the 
'dirtylimit' and doesn't affect the whole logic.

> 
> [...]
> 
>> +static void dirtylimit_adjust_throttle(CPUState *cpu)
>> +{
>> +    uint64_t quota = 0;
>> +    uint64_t current = 0;
>> +    int cpu_index = cpu->cpu_index;
>> +
>> +    quota = dirtylimit_vcpu_get_state(cpu_index)->quota;
>> +    current = vcpu_dirty_rate_get(cpu_index);
>> +
>> +    if (current == 0) {
>> +        cpu->throttle_us_per_full = 0;
>> +        goto end;
> 
> Can be dropped?
> 
>> +    } else if (dirtylimit_done(quota, current)) {
>> +        goto end;
> 
> Same here.  Dropping it wholely and:
> 
>         } else if (!dirtylimit_done(quota, current)) {
>             dirtylimit_set_throttle(cpu, quota, current);
>         }
> 
> Would work?
> 
>> +    } else {
>> +        dirtylimit_set_throttle(cpu, quota, current);
>> +    }
>> +end:
> 
> Can be dropped?
> 
>> +    trace_dirtylimit_adjust_throttle(cpu_index,
>> +                                     quota, current,
>> +                                     cpu->throttle_us_per_full);
>> +    return;
>> +}
>> +
>> +void dirtylimit_setup(void)
>> +{
>> +    CPUState *cpu;
>> +
>> +    if (!qatomic_read(&dirtylimit_quit)) {
>> +        dirtylimit_state_lock();
>> +
>> +        if (!dirtylimit_in_service()) {
>> +            dirtylimit_state_unlock();
> 
> Need to return?
> 
>> +        }
>> +
>> +        CPU_FOREACH(cpu) {
>> +            if (!dirtylimit_vcpu_get_state(cpu->cpu_index)->enabled) {
>> +                continue;
>> +            }
>> +            dirtylimit_adjust_throttle(cpu);
>> +        }
>> +        dirtylimit_state_unlock();
>> +    }
>> +}
> 
> [...]
> 
>> +void dirtylimit_set_vcpu(int cpu_index,
>> +                         uint64_t quota,
>> +                         bool enable)
>> +{
>> +    dirtylimit_vcpu_set_quota(cpu_index, quota, enable);
>> +    trace_dirtylimit_set_vcpu(cpu_index, quota);
>> +}
> 
> This helper is not "help"ful..  How about wrapping the trace into
> dirtylimit_vcpu_set_quota, then drop it?
> 
> Thanks,
> 

-- 
Best regard

Hyman Huang(黄勇)