Re: [Qemu-devel] [PATCH 2/3] migration: Wake rate limiting for urgent requests

[Peter Xu]

On Thu, Jun 14, 2018 at 09:50:35AM +0100, Dr. David Alan Gilbert wrote:
> * Peter Xu (address@hidden) wrote:
> > On Wed, Jun 13, 2018 at 11:26:41AM +0100, Dr. David Alan Gilbert (git) 
> > wrote:
> > 
> > [...]
> > 
> > > @@ -2932,10 +2943,24 @@ static void *migration_thread(void *opaque)
> > >  
> > >          migration_update_counters(s, current_time);
> > >  
> > > +        urgent = false;
> > >          if (qemu_file_rate_limit(s->to_dst_file)) {
> > > -            /* usleep expects microseconds */
> > > -            g_usleep((s->iteration_start_time + BUFFER_DELAY -
> > > -                      current_time) * 1000);
> > > +            /* Wait for a delay to do rate limiting OR
> > > +             * something urgent to post the semaphore.
> > > +             */
> > > +            int ms = s->iteration_start_time + BUFFER_DELAY - 
> > > current_time;
> > > +            trace_migration_thread_ratelimit_pre(ms);
> > > +            if (qemu_sem_timedwait(&s->rate_limit_sem, ms) == 0) {
> > > +                /* We were worken 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);
> > 
> > Is it possible that we just avoid eating that in the next patch?  Then
> > we only provide a helper to "trigger an urgent request" but we only
> > consume the point here?
> 
> I think it's harder;
> This code is generic in migration.c where as the next patch is all
> specific in ram.c; so we'd have to push a flag all the way down.
> Also, the code later is very simple - every request it adds/removes
> it posts/waits the semaphore - it's nice to keep that simple.

So what I meant above was to remove this post() meanwhile remove the
wait() in unqueue_page() in the next page.  Then it'll be a
single-direction message telling that "we have some urgent work to do"
without any ACK.  That'll possibly work but we might need to run the
loop even if we don't really have postcopy pages to send (e.g., when 2
continuous postcopy requests come, we'll handle the two pages in the
first loop, but we'll run the whole iteration twice while in the 2nd
loop we'll see that the postcopy queue is empty).

The ideal solution I can think of is to use a semaphore that only with
max value equals to 1.  I suppose that can be emulated by an eventfd
but I think it's fine now with current approach; we just need to make
sure the post() and wait() are "very" paired up well when calling the
new APIs.

As a summary:

Reviewed-by: Peter Xu <address@hidden>

(One important reason for the r-b is that I see Dave's graph when
 testing the series; it's very beautiful!)

Thanks,

-- 
Peter Xu