| Message ID | 20211207115016.73195-8-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Postcopy cleanup on ram disgard | expand |
* 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(); > > return 0; > } > diff --git a/migration/trace-events b/migration/trace-events > index d63a5915f5..1aa6937dc1 100644 > --- a/migration/trace-events > +++ b/migration/trace-events > @@ -14,7 +14,7 @@ loadvm_handle_cmd_packaged_main(int ret) "%d" > loadvm_handle_cmd_packaged_received(int ret) "%d" > loadvm_handle_recv_bitmap(char *s) "%s" > loadvm_postcopy_handle_advise(void) "" > -loadvm_postcopy_handle_listen(void) "" > +loadvm_postcopy_handle_listen(int i) "%d" > loadvm_postcopy_handle_run(void) "" > loadvm_postcopy_handle_run_cpu_sync(void) "" > loadvm_postcopy_handle_run_vmstart(void) "" > -- > 2.32.0 >
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 (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 >
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); 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); return 0; } diff --git a/migration/trace-events b/migration/trace-events index d63a5915f5..1aa6937dc1 100644 --- a/migration/trace-events +++ b/migration/trace-events @@ -14,7 +14,7 @@ loadvm_handle_cmd_packaged_main(int ret) "%d" loadvm_handle_cmd_packaged_received(int ret) "%d" loadvm_handle_recv_bitmap(char *s) "%s" loadvm_postcopy_handle_advise(void) "" -loadvm_postcopy_handle_listen(void) "" +loadvm_postcopy_handle_listen(int i) "%d" loadvm_postcopy_handle_run(void) "" loadvm_postcopy_handle_run_cpu_sync(void) "" loadvm_postcopy_handle_run_vmstart(void) ""
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(-)