Re: [PATCH] migration: Rate limit inside host pages

[Peter Xu <peterx@redhat.com>]

On Thu, Dec 05, 2019 at 10:29:18AM +0000, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> When using hugepages, rate limiting is necessary within each huge
> page, since a 1G huge page can take a significant time to send, so
> you end up with bursty behaviour.
> 
> Fixes: 4c011c37ecb3 ("postcopy: Send whole huge pages")
> Reported-by: Lin Ma <LMa@suse.com>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  migration/migration.c  | 57 ++++++++++++++++++++++++------------------
>  migration/migration.h  |  1 +
>  migration/ram.c        |  2 ++
>  migration/trace-events |  4 +--
>  4 files changed, 37 insertions(+), 27 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 354ad072fa..27500d09a9 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -3224,6 +3224,37 @@ void migration_consume_urgent_request(void)
>      qemu_sem_wait(&migrate_get_current()->rate_limit_sem);
>  }
>  
> +/* Returns true if the rate limiting was broken by an urgent request */
> +bool migration_rate_limit(void)
> +{
> +    int64_t now = qemu_clock_get_ms(QEMU_CLOCK_REALTIME);
> +    MigrationState *s = migrate_get_current();
> +
> +    bool urgent = false;
> +    migration_update_counters(s, now);
> +    if (qemu_file_rate_limit(s->to_dst_file)) {
> +        /*
> +         * Wait for a delay to do rate limiting OR
> +         * something urgent to post the semaphore.
> +         */
> +        int ms = s->iteration_start_time + BUFFER_DELAY - now;
> +        trace_migration_rate_limit_pre(ms);
> +        if (qemu_sem_timedwait(&s->rate_limit_sem, ms) == 0) {
> +            /*
> +             * We were woken by one or more urgent things but
> +             * the timedwait will have consumed one of them.
> +             * The service routine for the urgent wake will dec
> +             * the semaphore itself for each item it consumes,
> +             * so add this one we just eat back.
> +             */
> +            qemu_sem_post(&s->rate_limit_sem);

I remembered I've commented around this when it was first introduced
on whether we can avoid this post().  IMHO we can if with something
like an eventfd, so when we queue the page we write the eventfd to 1,
here we poll() on the eventfd with the same timeout, then clear it
after the poll no matter what.  When unqueue, we can probably simply
do nothing.  I'm not sure about Windows or other OS, though..

Anyway this patch is not changing that part but to fix huge page
issue, so that's another story for sure.

Reviewed-by: Peter Xu <peterx@redhat.com>

Thanks,

-- 
Peter Xu