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

[Peter Xu]

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")

?

Thanks,

-- 
Peter Xu