Re: [PATCH 7/7] migration: Finer grained tracepoints for POSTCOPY_LISTEN

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* Peter Xu (peterx@redhat.com) wrote:
> On Wed, Dec 08, 2021 at 07:46:20PM +0000, Dr. David Alan Gilbert wrote:
> > * Peter Xu (peterx@redhat.com) wrote:
> > > The enablement of postcopy listening has a few steps, add a few tracepoints to
> > > be there ready for some basic measurements for them.
> > > 
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >  migration/savevm.c     | 5 ++++-
> > >  migration/trace-events | 2 +-
> > >  2 files changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/migration/savevm.c b/migration/savevm.c
> > > index 17b8e25e00..5b3f31eab2 100644
> > > --- a/migration/savevm.c
> > > +++ b/migration/savevm.c
> > > @@ -1946,7 +1946,7 @@ static void *postcopy_ram_listen_thread(void *opaque)
> > >  static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> > >  {
> > >      PostcopyState ps = postcopy_state_set(POSTCOPY_INCOMING_LISTENING);
> > > -    trace_loadvm_postcopy_handle_listen();
> > > +    trace_loadvm_postcopy_handle_listen(1);
> > 
> > I think we tend just to split this into separate traces in many places;
> > or if we're using the same one then we should use a string
> > 
> > I'd make this:
> >   trace_loadvm_postcopy_handle_listen_entry();
> > 
> > for example.
> > 
> > >      Error *local_err = NULL;
> > >  
> > >      if (ps != POSTCOPY_INCOMING_ADVISE && ps != POSTCOPY_INCOMING_DISCARD) {
> > > @@ -1962,6 +1962,7 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> > >              postcopy_ram_prepare_discard(mis);
> > >          }
> > >      }
> > > +    trace_loadvm_postcopy_handle_listen(2);
> > >  
> > >      /*
> > >       * Sensitise RAM - can now generate requests for blocks that don't exist
> > > @@ -1974,6 +1975,7 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> > >              return -1;
> > >          }
> > >      }
> > > +    trace_loadvm_postcopy_handle_listen(3);
> > >  
> > >      if (postcopy_notify(POSTCOPY_NOTIFY_INBOUND_LISTEN, &local_err)) {
> > >          error_report_err(local_err);
> > > @@ -1988,6 +1990,7 @@ static int loadvm_postcopy_handle_listen(MigrationIncomingState *mis)
> > >                         QEMU_THREAD_DETACHED);
> > >      qemu_sem_wait(&mis->listen_thread_sem);
> > >      qemu_sem_destroy(&mis->listen_thread_sem);
> > > +    trace_loadvm_postcopy_handle_listen(4);
> > 
> >   trace_loadvm_postcopy_handle_listen_entry_end();
> 
> I see, I can use it.  It's just that I want to trap more than entry/exit.
> 
> For the "4 steps" here I split it into four procedures, the 2 steps inside are
> majorly to trap either uffd registration time or external uffd handling of
> other processes.
> 
> One example:
> 
> We may want to know how slow is postcopy_notify(POSTCOPY_NOTIFY_INBOUND_LISTEN)
> when there're some external process attached.  I wanted to be prepared for that
> so when there's need to evaluate slowness of this procedure with vhost-user
> enabled we have some tracepoints without replacing the binaries.
> 
> It's easy to use strings too if that's better looking than numbers.  How about:
> 
>   trace_loadvm_postcopy_handle_listen("entry")
>   trace_loadvm_postcopy_handle_listen("uffd-reg")
>   trace_loadvm_postcopy_handle_listen("external")
>   trace_loadvm_postcopy_handle_listen("exit")
> 

Yes, that's fine; but it would also be fine to create 4 separate traces.

Dave

> ?
> 
> Thanks,
> 
> -- 
> Peter Xu
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK