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(+)

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

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

-- 
Thanks,

David / dhildenb

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,

-- 
Peter Xu

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: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!


-- 
Thanks,

David / dhildenb

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