Re: [Qemu-devel] [RFC 1/3] exec: Adjust notdirty tracing

[David Hildenbrand]

On 18.09.19 07:26, Richard Henderson wrote:
> The memory_region_tb_read tracepoint is unreachable, since notdirty
> is supposed to apply only to reads.  The memory_region_tb_write
> tracepoint is mis-named, because notdirty is not only used for TB
> invalidation.  It is also used for e.g. VGA RAM updates.
> 
> Replace memory_region_tb_write with memory_notdirty_write, and
> place it in memory_notdirty_write_prepare where it can catch all
> of the instances.  Add memory_notdirty_dirty to log when we no
> longer intercept writes to a page.
> 
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  exec.c       | 3 +++
>  memory.c     | 4 ----
>  trace-events | 4 ++--
>  3 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/exec.c b/exec.c
> index 8b998974f8..9babe57615 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2755,6 +2755,8 @@ void memory_notdirty_write_prepare(NotDirtyInfo *ndi,
>      ndi->size = size;
>      ndi->pages = NULL;
>  
> +    trace_memory_notdirty_write(mem_vaddr, ram_addr, size);
> +
>      assert(tcg_enabled());
>      if (!cpu_physical_memory_get_dirty_flag(ram_addr, DIRTY_MEMORY_CODE)) {
>          ndi->pages = page_collection_lock(ram_addr, ram_addr + size);
> @@ -2779,6 +2781,7 @@ void memory_notdirty_write_complete(NotDirtyInfo *ndi)
>      /* we remove the notdirty callback only if the code has been
>         flushed */
>      if (!cpu_physical_memory_is_clean(ndi->ram_addr)) {
> +        trace_memory_notdirty_dirty(ndi->mem_vaddr);
>          tlb_set_dirty(ndi->cpu, ndi->mem_vaddr);
>      }
>  }
> diff --git a/memory.c b/memory.c
> index b9dd6b94ca..57c44c97db 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -438,7 +438,6 @@ static MemTxResult  
> memory_region_read_accessor(MemoryRegion *mr,
>          /* Accesses to code which has previously been translated into a TB 
> show
>           * up in the MMIO path, as accesses to the io_mem_notdirty
>           * MemoryRegion. */
> -        trace_memory_region_tb_read(get_cpu_index(), addr, tmp, size);
>      } else if (TRACE_MEMORY_REGION_OPS_READ_ENABLED) {
>          hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
>          trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, 
> size);
> @@ -465,7 +464,6 @@ static MemTxResult 
> memory_region_read_with_attrs_accessor(MemoryRegion *mr,
>          /* Accesses to code which has previously been translated into a TB 
> show
>           * up in the MMIO path, as accesses to the io_mem_notdirty
>           * MemoryRegion. */
> -        trace_memory_region_tb_read(get_cpu_index(), addr, tmp, size);
>      } else if (TRACE_MEMORY_REGION_OPS_READ_ENABLED) {
>          hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
>          trace_memory_region_ops_read(get_cpu_index(), mr, abs_addr, tmp, 
> size);
> @@ -490,7 +488,6 @@ static MemTxResult 
> memory_region_write_accessor(MemoryRegion *mr,
>          /* Accesses to code which has previously been translated into a TB 
> show
>           * up in the MMIO path, as accesses to the io_mem_notdirty
>           * MemoryRegion. */
> -        trace_memory_region_tb_write(get_cpu_index(), addr, tmp, size);
>      } else if (TRACE_MEMORY_REGION_OPS_WRITE_ENABLED) {
>          hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
>          trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, 
> size);
> @@ -515,7 +512,6 @@ static MemTxResult 
> memory_region_write_with_attrs_accessor(MemoryRegion *mr,
>          /* Accesses to code which has previously been translated into a TB 
> show
>           * up in the MMIO path, as accesses to the io_mem_notdirty
>           * MemoryRegion. */
> -        trace_memory_region_tb_write(get_cpu_index(), addr, tmp, size);
>      } else if (TRACE_MEMORY_REGION_OPS_WRITE_ENABLED) {
>          hwaddr abs_addr = memory_region_to_absolute_addr(mr, addr);
>          trace_memory_region_ops_write(get_cpu_index(), mr, abs_addr, tmp, 
> size);
> diff --git a/trace-events b/trace-events
> index 823a4ae64e..5c9a1631e7 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -52,14 +52,14 @@ dma_map_wait(void *dbs) "dbs=%p"
>  find_ram_offset(uint64_t size, uint64_t offset) "size: 0x%" PRIx64 " @ 0x%" 
> PRIx64
>  find_ram_offset_loop(uint64_t size, uint64_t candidate, uint64_t offset, 
> uint64_t next, uint64_t mingap) "trying size: 0x%" PRIx64 " @ 0x%" PRIx64 ", 
> offset: 0x%" PRIx64" next: 0x%" PRIx64 " mingap: 0x%" PRIx64
>  ram_block_discard_range(const char *rbname, void *hva, size_t length, bool 
> need_madvise, bool need_fallocate, int ret) "%s@%p + 0x%zx: madvise: %d 
> fallocate: %d ret: %d"
> +memory_notdirty_write(uint64_t vaddr, uint64_t ram_addr, unsigned size) 
> "0x%" PRIx64 " ram_addr 0x%" PRIx64 " size %u"
> +memory_notdirty_dirty(uint64_t vaddr) "0x%" PRIx64

My only suggestion would be to give slightly better names like

memory_notdirty_write_access()
memory_notdirty_set_dirty()

-- 

Thanks,

David / dhildenb