Re: [PATCH] softmmu/dirtylimit: Fix usleep early return on signal

[alloc young]

On 2023/9/4 21:27, Yong Huang wrote:
> On Fri, Sep 1, 2023 at 10:19 AM <alloc.young@outlook.com 
> <mailto:alloc.young@outlook.com>> wrote:
>
>     From: alloc <alloc.young@outlook.com <mailto:alloc.young@outlook.com>>
>
>     Timeout functions like usleep can return early on signal, which reduces
>     more dirty pages than expected. In dirtylimit case, dirtyrate meter
>     thread needs to kick all vcpus out to sync. The callchain:
>
>     vcpu_calculate_dirtyrate
>          global_dirty_log_sync
>              memory_global_dirty_log_sync
>                  kvm_log_sync_global
>                      kvm_dirty_ring_flush
>                          kvm_cpu_synchronize_kick_all <---- send vcpu signal
>
>     For long time sleep, use qemu_cond_timedwait_iothread to handle cpu stop
>     event.
>
>
> The Dirty Limit algorithm seeks to keep the vCPU dirty page rate within
> the set limit; since it focuses more emphasis on processing time and
> precision, I feel that improvement should strive for the same result.
> Could you please provide the final test results showing the impact of
> that improvement?

The kvm_cpu_sync in dirty ring flush has to wait all vcpu to exit to run 
sync action, while the vcpu sleep may blocks this. Before this patch, 
the kick can reduce early vcpu return and add more dirty pages. It seems
the kvm_cpu_sync conflicts with vcpu sleep, why not measure dirty rate 
when dirty ring fulls?

>     Signed-off-by: alloc <alloc.young@outlook.com
>     <mailto:alloc.young@outlook.com>>
>     ---
>       softmmu/dirtylimit.c | 19 +++++++++++++++++--
>       1 file changed, 17 insertions(+), 2 deletions(-)
>
>     diff --git a/softmmu/dirtylimit.c b/softmmu/dirtylimit.c
>     index fa959d7743..ee938c636d 100644
>     --- a/softmmu/dirtylimit.c
>     +++ b/softmmu/dirtylimit.c
>     @@ -411,13 +411,28 @@ void dirtylimit_set_all(uint64_t quota,
>
>       void dirtylimit_vcpu_execute(CPUState *cpu)
>       {
>     +    int64_t sleep_us, endtime_us;
>     +
>     +    dirtylimit_state_lock();
>           if (dirtylimit_in_service() &&
>               dirtylimit_vcpu_get_state(cpu->cpu_index)->enabled &&
>               cpu->throttle_us_per_full) {
>               trace_dirtylimit_vcpu_execute(cpu->cpu_index,
>                       cpu->throttle_us_per_full);
>     -        usleep(cpu->throttle_us_per_full);
>     -    }
>     +        sleep_us = cpu->throttle_us_per_full;
>     +        dirtylimit_state_unlock();
>     +        endtime_us = qemu_clock_get_us(QEMU_CLOCK_REALTIME) + sleep_us;
>     +        while (sleep_us > 0 && !cpu->stop) {
>     +            if (sleep_us > SCALE_US) {
>     +                qemu_mutex_lock_iothread();
>     +                qemu_cond_timedwait_iothread(cpu->halt_cond,
>     sleep_us / SCALE_US);
>     +                qemu_mutex_unlock_iothread();
>     +            } else
>     +                g_usleep(sleep_us);
>     +            sleep_us = endtime_us -
>     qemu_clock_get_us(QEMU_CLOCK_REALTIME);
>     +        }
>     +    } else
>     +        dirtylimit_state_unlock();
>       }
>
>       static void dirtylimit_init(void)
>     -- 
>     2.39.3
>
>
>
> --
> Best regards