[PATCH 2/8] system/memory: add memory_region_finalize tracepoint

Alex Bennée posted 8 patches 1 month ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, "Alex Bennée" <alex.bennee@linaro.org>, Akihiko Odaki <odaki@rsg.ci.i.u-tokyo.ac.jp>, Dmitry Osipenko <dmitry.osipenko@collabora.com>, Paolo Bonzini <pbonzini@redhat.com>, Peter Xu <peterx@redhat.com>, David Hildenbrand <david@redhat.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Eric Blake <eblake@redhat.com>, Markus Armbruster <armbru@redhat.com>, "Marc-André Lureau" <marcandre.lureau@redhat.com>
[PATCH 2/8] system/memory: add memory_region_finalize tracepoint
Posted by Alex Bennée 1 month ago
This only traces named memory regions as it is otherwise quite noisy
every time the address map changes.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 system/memory.c     | 5 +++++
 system/trace-events | 1 +
 2 files changed, 6 insertions(+)

diff --git a/system/memory.c b/system/memory.c
index 8b84661ae36..fd7c3192ed4 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -1821,6 +1821,11 @@ static void memory_region_finalize(Object *obj)
      * memory_region_set_enabled instead could trigger a transaction and
      * cause an infinite loop.
      */
+
+    if (mr->name) {
+        trace_memory_region_finalize(mr, mr->name);
+    }
+
     mr->enabled = false;
     memory_region_transaction_begin();
     if (mr->container) {
diff --git a/system/trace-events b/system/trace-events
index 82856e44f2e..a8ef2326e14 100644
--- a/system/trace-events
+++ b/system/trace-events
@@ -23,6 +23,7 @@ memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t v
 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)"
+memory_region_finalize(void *mr, const char *name) "mr %p, %s"
 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.47.3


Re: [PATCH 2/8] system/memory: add memory_region_finalize tracepoint
Posted by Akihiko Odaki 1 month ago
On 2025/10/14 20:12, Alex Bennée wrote:
> This only traces named memory regions as it is otherwise quite noisy
> every time the address map changes.

Checking for name does not seem effective to reduce noises. I used 
Coccinelle and found there are only three instances of unnamed memory 
regions. The three instances are:

- memory_region_init_alias() in xen_gnttab_realize()
- memory_region_init_io() in subpage_init()
- memory_region_init_io() in io_mem_init()

The command line I used is as follows:
spatch --macro-file scripts/cocci-macro-file.h --sp-file 
./scripts/coccinelle/a.cocci --keep-comments --in-place --use-gitgrep 
--dir .

Below is the content of scripts/coccinelle/a.cocci:

@filter@
expression a, b, c, d;
position p;
@@

(
  memory_region_init@p(a, b, NULL, ...);
|
  memory_region_init_io@p(a, b, c, d, NULL, ...);
|
  memory_region_init_ram_nomigrate@p(a, b, NULL, ...);
|
  memory_region_init_ram_flags_nomigrate@p(a, b, NULL, ...);
|
  memory_region_init_resizeable_ram@p(a, b, NULL, ...);
|
  memory_region_init_ram_from_file@p(a, b, NULL, ...);
|
  memory_region_init_ram_from_fd@p(a, b, NULL, ...);
|
  memory_region_init_ram_ptr@p(a, b, NULL, ...);
|
  memory_region_init_ram_device_ptr@p(a, b, NULL, ...);
|
  memory_region_init_alias@p(a, b, NULL, ...);
|
  memory_region_init_rom_nomigrate@p(a, b, NULL, ...);
|
  memory_region_init_rom_device_nomigrate@p(a, b, c, d, NULL, ...);
|
  memory_region_init_iommu@p(a, b, c, d, NULL, ...);
|
  memory_region_init_ram@p(a, b, NULL, ...);
|
  memory_region_init_ram_guest_memfd@p(a, b, NULL, ...);
|
  memory_region_init_rom@p(a, b, NULL, ...);
|
  memory_region_init_rom_device@p(a, b, c, d, NULL, ...);
)

@script:python@
p << filter.p;
@@

cocci.print_main("found", p)


> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>   system/memory.c     | 5 +++++
>   system/trace-events | 1 +
>   2 files changed, 6 insertions(+)
> 
> diff --git a/system/memory.c b/system/memory.c
> index 8b84661ae36..fd7c3192ed4 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -1821,6 +1821,11 @@ static void memory_region_finalize(Object *obj)
>        * memory_region_set_enabled instead could trigger a transaction and
>        * cause an infinite loop.
>        */
> +
> +    if (mr->name) {
> +        trace_memory_region_finalize(mr, mr->name);
> +    }
> +
>       mr->enabled = false;
>       memory_region_transaction_begin();
>       if (mr->container) {
> diff --git a/system/trace-events b/system/trace-events
> index 82856e44f2e..a8ef2326e14 100644
> --- a/system/trace-events
> +++ b/system/trace-events
> @@ -23,6 +23,7 @@ memory_region_subpage_write(int cpu_index, void *mr, uint64_t offset, uint64_t v
>   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)"
> +memory_region_finalize(void *mr, const char *name) "mr %p, %s"
>   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)"