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

[Hyman]

在 2021/12/6 16:36, 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.
Ok.
> > +
> > +        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?
> The dirty limit logic only allow plugged vcpu to be enabled throttle, so 
that the "dirtylimit-{cpu-index}" thread don't need to be forked and we 
can save the overhead. So in query logic we just filter the unplugged vcpu.

Another reason is that i thought it could make user confused when we 
return the unplugged vcpu dirtylimit info. Uh, in most time of vm 
lifecycle, hotplugging vcpu may never happen.
> > +        }
> > +    }
> > +
> > +    return head;
> > +}