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

[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.

[...]

> +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.

Again, I'd rather call the function directly..

[...]

> +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,

-- 
Peter Xu