| Message ID | 20210817013706.30986-1-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | memory: Add tracepoints for log_sync | expand |
On 17.08.21 03:37, Peter Xu wrote: > Trace at memory_region_sync_dirty_bitmap() for log_sync() or global_log_sync() > on memory regions. One trace line should suffice when it finishes, so as to > estimate the time used for each log sync process. I wonder if a start/finish would be even nicer. At least it wouldn't really result in significantly more code changes :)
On Tue, Aug 17, 2021 at 09:25:56AM +0200, David Hildenbrand wrote: > On 17.08.21 03:37, Peter Xu wrote: > > Trace at memory_region_sync_dirty_bitmap() for log_sync() or global_log_sync() > > on memory regions. One trace line should suffice when it finishes, so as to > > estimate the time used for each log sync process. > > I wonder if a start/finish would be even nicer. At least it wouldn't really > result in significantly more code changes :) Note that the "name"s I added is not only for not using start/end, it's about knowing which memory listener is slow. Start/end won't achieve that if we don't have a name for them. So far I just wanted to identify majorly kvm, vhost and kvm-smram, however it'll always be good when some log_sync is missed when tracing. I'm also wondering whether kvm-smram needs a whole bitmap as I don't know what RAM would be touched within system manager mode (as I thought it should only touch a very limited range and should be defined somewhere), but that's off-topic. If we want to make it start/end pair, I can do that too. But the 1st patch will still be wanted. Thanks,
On 17.08.21 18:05, Peter Xu wrote: > On Tue, Aug 17, 2021 at 09:25:56AM +0200, David Hildenbrand wrote: >> On 17.08.21 03:37, Peter Xu wrote: >>> Trace at memory_region_sync_dirty_bitmap() for log_sync() or global_log_sync() >>> on memory regions. One trace line should suffice when it finishes, so as to >>> estimate the time used for each log sync process. >> >> I wonder if a start/finish would be even nicer. At least it wouldn't really >> result in significantly more code changes :) > > Note that the "name"s I added is not only for not using start/end, it's about > knowing which memory listener is slow. Start/end won't achieve that if we > don't have a name for them. So far I just wanted to identify majorly kvm, > vhost and kvm-smram, however it'll always be good when some log_sync is missed > when tracing. > > I'm also wondering whether kvm-smram needs a whole bitmap as I don't know what > RAM would be touched within system manager mode (as I thought it should only > touch a very limited range and should be defined somewhere), but that's > off-topic. > > If we want to make it start/end pair, I can do that too. But the 1st patch > will still be wanted. Yeah, absolutely, not complaining about the name, it will be valuable to have!
On 17/08/21 18:05, Peter Xu wrote: > > I'm also wondering whether kvm-smram needs a whole bitmap as I don't know what > RAM would be touched within system manager mode (as I thought it should only > touch a very limited range and should be defined somewhere), but that's > off-topic. The kvm-smram dirty bitmap will include all memory touched while the SMM address space is in effect, so not just SMRAM. The two KVM dirty bitmaps end up in just one QEMU dirty bitmap (the one with id DIRTY_MEMORY_MIGRATION) but they are separate at the kernel level. Paolo
On 17/08/21 18:07, David Hildenbrand wrote: >> >> >> If we want to make it start/end pair, I can do that too. But the 1st >> patch >> will still be wanted. > > Yeah, absolutely, not complaining about the name, it will be valuable to > have! You could have start/end tracepoints for memory_region_sync_dirty_bitmap on top of this one that you are adding, so I queued the patches. Paolo
diff --git a/softmmu/memory.c b/softmmu/memory.c index bfedaf9c4d..f0c5817b97 100644 --- a/softmmu/memory.c +++ b/softmmu/memory.c @@ -2149,6 +2149,7 @@ static void memory_region_sync_dirty_bitmap(MemoryRegion *mr) } } flatview_unref(view); + trace_memory_region_sync_dirty(mr ? mr->name : "(all)", listener->name, 0); } else if (listener->log_sync_global) { /* * No matter whether MR is specified, what we can do here @@ -2156,6 +2157,7 @@ static void memory_region_sync_dirty_bitmap(MemoryRegion *mr) * sync in a finer granularity. */ listener->log_sync_global(listener); + trace_memory_region_sync_dirty(mr ? mr->name : "(all)", listener->name, 1); } } } diff --git a/softmmu/trace-events b/softmmu/trace-events index 7b278590a0..bf1469990e 100644 --- a/softmmu/trace-events +++ b/softmmu/trace-events @@ -15,6 +15,7 @@ memory_region_subpage_read(int cpu_index, void *mr, uint64_t offset, uint64_t va memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t value, unsigned size) "cpu %d mr %p offset 0x%"PRIx64" value 0x%"PRIx64" size %u" memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u" memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 0x%"PRIx64" size %u" +memory_region_sync_dirty(const char *mr, const char *listener, int global) "mr '%s' listener '%s' synced (global=%d)" flatview_new(void *view, void *root) "%p (root %p)" flatview_destroy(void *view, void *root) "%p (root %p)" flatview_destroy_rcu(void *view, void *root) "%p (root %p)"
Trace at memory_region_sync_dirty_bitmap() for log_sync() or global_log_sync() on memory regions. One trace line should suffice when it finishes, so as to estimate the time used for each log sync process. Signed-off-by: Peter Xu <peterx@redhat.com> --- softmmu/memory.c | 2 ++ softmmu/trace-events | 1 + 2 files changed, 3 insertions(+)