Re: [PATCH v9 3/3] cpus-common: implement dirty page limit on vCPU

[Peter Xu]

On Fri, Dec 03, 2021 at 09:39:47AM +0800, huangy81@chinatelecom.cn wrote:
> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> 
> Implement dirtyrate calculation periodically basing on
> dirty-ring and throttle vCPU until it reachs the quota
> dirty page rate given by user.
> 
> Introduce qmp commands "vcpu-dirty-limit", "query-vcpu-dirty-limit"
> to enable, disable, query dirty page limit for virtual CPU.
> 
> Meanwhile, introduce corresponding hmp commands "vcpu_dirty_limit",
> "info vcpu_dirty_limit" so developers can play with them easier.

Thanks.  Even if I start to use qmp-shell more recently but still in some case
where only hmp is specified this could still be handy.

> +void qmp_vcpu_dirty_limit(int64_t cpu_index,
> +                          bool enable,
> +                          uint64_t dirty_rate,
> +                          Error **errp)
> +{
> +    if (!kvm_enabled() || !kvm_dirty_ring_enabled()) {
> +        error_setg(errp, "dirty page limit feature requires KVM with"
> +                   " accelerator property 'dirty-ring-size' set'");
> +        return;
> +    }
> +
> +    if (!dirtylimit_is_vcpu_index_valid(cpu_index)) {
> +        error_setg(errp, "cpu index out of range");
> +        return;
> +    }
> +
> +    if (enable) {
> +        dirtylimit_calc();
> +        dirtylimit_vcpu(cpu_index, dirty_rate);
> +    } else {
> +        if (!dirtylimit_enabled(cpu_index)) {
> +            error_setg(errp, "dirty page limit for CPU %ld not set",
> +                       cpu_index);
> +            return;
> +        }

We don't need to fail the user for enable=off even if vcpu is not throttled,
imho.

> +
> +        if (!dirtylimit_cancel_vcpu(cpu_index)) {
> +            dirtylimit_calc_quit();
> +        }
> +    }
> +}

[...]

> +struct DirtyLimitInfoList *qmp_query_vcpu_dirty_limit(bool has_cpu_index,
> +                                                      int64_t cpu_index,
> +                                                      Error **errp)
> +{
> +    DirtyLimitInfo *info = NULL;
> +    DirtyLimitInfoList *head = NULL, **tail = &head;
> +
> +    if (has_cpu_index &&
> +        (!dirtylimit_is_vcpu_index_valid(cpu_index))) {
> +        error_setg(errp, "cpu index out of range");
> +        return NULL;
> +    }
> +
> +    if (has_cpu_index) {
> +        info = dirtylimit_query_vcpu(cpu_index);
> +        QAPI_LIST_APPEND(tail, info);
> +    } else {
> +        CPUState *cpu;
> +        CPU_FOREACH(cpu) {
> +            if (!cpu->unplug) {
> +                info = dirtylimit_query_vcpu(cpu->cpu_index);
> +                QAPI_LIST_APPEND(tail, info);
> +            }

There're special handling for unplug in a few places.  Could you explain why?
E.g. if the vcpu is unplugged then dirty rate is zero, then it seems fine to
even keep it there?

> +        }
> +    }
> +
> +    return head;
> +}

-- 
Peter Xu