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

[Hyman Huang]

在 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(黄勇)