[PATCH v3 0/2] hw/vfio: Improve vfio_get_dirty_bitmap() tracepoint

[PATCH v3 0/2] hw/vfio: Improve vfio_get_dirty_bitmap() tracepoint

[Joao Martins]

Hey,

This tiny series changes the tracepoint to include the number of
dirty pages via the vfio_get_dirty_bitmap. I find it useful for
observability in general to understand the number of dirty pages in an
IOVA range. With dirty tracking supported by device or IOMMU it's specially
relevant data to include in the tracepoint.

First patch changes the return value to be the number of dirty pages in
the helper function setting dirty bits and the second patch expands the
VFIO tracepoint to include the dirty pages.

Thanks,
	Joao

Changes since v2[2]:
* Add a comment explaining the difference of retval between
cpu_physical_memory_set_dirty_lebitmap() and
cpu_physical_memory_sync_dirty_bitmap() (Peter Xu)
* Add Peter's Reviewed-by;
* Rename dirty variable into dirty_pages (Philippe Mathieu-Daude)

Changes since v1[1]:
* Make the nr of dirty pages a retval similar to sync variant of helper in
  patch 1 (Cedric)
* Stash number of bits set in bitmap quad in a variable and reuse in
  GLOBAL_DIRTY_RATE in patch 1
* Drop init to 0 given that we always initialize the @dirty used in the
  tracepoint

[1] https://lore.kernel.org/qemu-devel/20230523151217.46427-1-joao.m.martins@oracle.com/
[2] https://lore.kernel.org/qemu-devel/20230525114321.71066-1-joao.m.martins@oracle.com/

Joao Martins (2):
  exec/ram_addr: return nr of dirty pages in
    cpu_physical_memory_set_dirty_lebitmap()
  hw/vfio: Add nr of dirty pages to vfio_get_dirty_bitmap tracepoint

 hw/vfio/common.c        |  7 ++++---
 hw/vfio/trace-events    |  2 +-
 include/exec/ram_addr.h | 28 ++++++++++++++++++++++------
 3 files changed, 27 insertions(+), 10 deletions(-)

-- 
2.39.3

[PATCH v3 1/2] exec/ram_addr: return nr of dirty pages in cpu_physical_memory_set_dirty_lebitmap()

[Joao Martins]

In preparation for including the number of dirty pages in the
vfio_get_dirty_bitmap() tracepoint, return the number of dirty pages in
cpu_physical_memory_set_dirty_lebitmap() similar to
cpu_physical_memory_sync_dirty_bitmap().

To avoid counting twice when GLOBAL_DIRTY_RATE is enabled, stash the
number of bits set per bitmap quad in a variable (@nbits) and reuse it
there.

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 include/exec/ram_addr.h | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index 90a82692904f..9f2e3893f562 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -334,14 +334,23 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
 }
 
 #if !defined(_WIN32)
-static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
-                                                          ram_addr_t start,
-                                                          ram_addr_t pages)
+
+/*
+ * Contrary to cpu_physical_memory_sync_dirty_bitmap() this function returns
+ * the number of dirty pages in @bitmap passed as argument. On the other hand,
+ * cpu_physical_memory_sync_dirty_bitmap() returns newly dirtied pages that
+ * weren't set in the global migration bitmap.
+ */
+static inline
+uint64_t cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
+                                                ram_addr_t start,
+                                                ram_addr_t pages)
 {
     unsigned long i, j;
-    unsigned long page_number, c;
+    unsigned long page_number, c, nbits;
     hwaddr addr;
     ram_addr_t ram_addr;
+    uint64_t num_dirty = 0;
     unsigned long len = (pages + HOST_LONG_BITS - 1) / HOST_LONG_BITS;
     unsigned long hpratio = qemu_real_host_page_size() / TARGET_PAGE_SIZE;
     unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
@@ -369,6 +378,7 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
                 if (bitmap[k]) {
                     unsigned long temp = leul_to_cpu(bitmap[k]);
 
+                    nbits = ctpopl(temp);
                     qatomic_or(&blocks[DIRTY_MEMORY_VGA][idx][offset], temp);
 
                     if (global_dirty_tracking) {
@@ -377,10 +387,12 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
                                 temp);
                         if (unlikely(
                             global_dirty_tracking & GLOBAL_DIRTY_DIRTY_RATE)) {
-                            total_dirty_pages += ctpopl(temp);
+                            total_dirty_pages += nbits;
                         }
                     }
 
+                    num_dirty += nbits;
+
                     if (tcg_enabled()) {
                         qatomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset],
                                    temp);
@@ -409,9 +421,11 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
         for (i = 0; i < len; i++) {
             if (bitmap[i] != 0) {
                 c = leul_to_cpu(bitmap[i]);
+                nbits = ctpopl(c);
                 if (unlikely(global_dirty_tracking & GLOBAL_DIRTY_DIRTY_RATE)) {
-                    total_dirty_pages += ctpopl(c);
+                    total_dirty_pages += nbits;
                 }
+                num_dirty += nbits;
                 do {
                     j = ctzl(c);
                     c &= ~(1ul << j);
@@ -424,6 +438,8 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
             }
         }
     }
+
+    return num_dirty;
 }
 #endif /* not _WIN32 */
 
-- 
2.39.3

[PATCH v3 2/2] hw/vfio: Add nr of dirty pages to vfio_get_dirty_bitmap tracepoint

[Joao Martins]

Include the number of dirty pages on the vfio_get_dirty_bitmap tracepoint.
These are fetched from the newly added return value in
cpu_physical_memory_set_lebitmap().

Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
---
 hw/vfio/common.c     | 7 ++++---
 hw/vfio/trace-events | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 78358ede2764..fa8fd949b1cf 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1747,6 +1747,7 @@ static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
 {
     bool all_device_dirty_tracking =
         vfio_devices_all_device_dirty_tracking(container);
+    uint64_t dirty_pages;
     VFIOBitmap vbmap;
     int ret;
 
@@ -1772,11 +1773,11 @@ static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
         goto out;
     }
 
-    cpu_physical_memory_set_dirty_lebitmap(vbmap.bitmap, ram_addr,
-                                           vbmap.pages);
+    dirty_pages = cpu_physical_memory_set_dirty_lebitmap(vbmap.bitmap, ram_addr,
+                                                         vbmap.pages);
 
     trace_vfio_get_dirty_bitmap(container->fd, iova, size, vbmap.size,
-                                ram_addr);
+                                ram_addr, dirty_pages);
 out:
     g_free(vbmap.bitmap);
 
diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
index 646e42fd27f9..cfb60c354de3 100644
--- a/hw/vfio/trace-events
+++ b/hw/vfio/trace-events
@@ -120,7 +120,7 @@ vfio_region_sparse_mmap_header(const char *name, int index, int nr_areas) "Devic
 vfio_region_sparse_mmap_entry(int i, unsigned long start, unsigned long end) "sparse entry %d [0x%lx - 0x%lx]"
 vfio_get_dev_region(const char *name, int index, uint32_t type, uint32_t subtype) "%s index %d, %08x/%08x"
 vfio_dma_unmap_overflow_workaround(void) ""
-vfio_get_dirty_bitmap(int fd, uint64_t iova, uint64_t size, uint64_t bitmap_size, uint64_t start) "container fd=%d, iova=0x%"PRIx64" size= 0x%"PRIx64" bitmap_size=0x%"PRIx64" start=0x%"PRIx64
+vfio_get_dirty_bitmap(int fd, uint64_t iova, uint64_t size, uint64_t bitmap_size, uint64_t start, uint64_t dirty_pages) "container fd=%d, iova=0x%"PRIx64" size= 0x%"PRIx64" bitmap_size=0x%"PRIx64" start=0x%"PRIx64" dirty_pages=%"PRIu64
 vfio_iommu_map_dirty_notify(uint64_t iova_start, uint64_t iova_end) "iommu dirty @ 0x%"PRIx64" - 0x%"PRIx64
 
 # platform.c
-- 
2.39.3

Re: [PATCH v3 2/2] hw/vfio: Add nr of dirty pages to vfio_get_dirty_bitmap tracepoint

[Cédric Le Goater]

On 5/29/23 14:11, Joao Martins wrote:
> Include the number of dirty pages on the vfio_get_dirty_bitmap tracepoint.
> These are fetched from the newly added return value in
> cpu_physical_memory_set_lebitmap().
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/vfio/common.c     | 7 ++++---
>   hw/vfio/trace-events | 2 +-
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 78358ede2764..fa8fd949b1cf 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1747,6 +1747,7 @@ static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
>   {
>       bool all_device_dirty_tracking =
>           vfio_devices_all_device_dirty_tracking(container);
> +    uint64_t dirty_pages;
>       VFIOBitmap vbmap;
>       int ret;
>   
> @@ -1772,11 +1773,11 @@ static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
>           goto out;
>       }
>   
> -    cpu_physical_memory_set_dirty_lebitmap(vbmap.bitmap, ram_addr,
> -                                           vbmap.pages);
> +    dirty_pages = cpu_physical_memory_set_dirty_lebitmap(vbmap.bitmap, ram_addr,
> +                                                         vbmap.pages);
>   
>       trace_vfio_get_dirty_bitmap(container->fd, iova, size, vbmap.size,
> -                                ram_addr);
> +                                ram_addr, dirty_pages);
>   out:
>       g_free(vbmap.bitmap);
>   
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index 646e42fd27f9..cfb60c354de3 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -120,7 +120,7 @@ vfio_region_sparse_mmap_header(const char *name, int index, int nr_areas) "Devic
>   vfio_region_sparse_mmap_entry(int i, unsigned long start, unsigned long end) "sparse entry %d [0x%lx - 0x%lx]"
>   vfio_get_dev_region(const char *name, int index, uint32_t type, uint32_t subtype) "%s index %d, %08x/%08x"
>   vfio_dma_unmap_overflow_workaround(void) ""
> -vfio_get_dirty_bitmap(int fd, uint64_t iova, uint64_t size, uint64_t bitmap_size, uint64_t start) "container fd=%d, iova=0x%"PRIx64" size= 0x%"PRIx64" bitmap_size=0x%"PRIx64" start=0x%"PRIx64
> +vfio_get_dirty_bitmap(int fd, uint64_t iova, uint64_t size, uint64_t bitmap_size, uint64_t start, uint64_t dirty_pages) "container fd=%d, iova=0x%"PRIx64" size= 0x%"PRIx64" bitmap_size=0x%"PRIx64" start=0x%"PRIx64" dirty_pages=%"PRIu64
>   vfio_iommu_map_dirty_notify(uint64_t iova_start, uint64_t iova_end) "iommu dirty @ 0x%"PRIx64" - 0x%"PRIx64
>   
>   # platform.c

Re: [PATCH v3 1/2] exec/ram_addr: return nr of dirty pages in cpu_physical_memory_set_dirty_lebitmap()

[Avihai Horon]

On 29/05/2023 15:11, Joao Martins wrote:
> External email: Use caution opening links or attachments
>
Nit, s/nr/number in the subject.

> In preparation for including the number of dirty pages in the
> vfio_get_dirty_bitmap() tracepoint, return the number of dirty pages in
> cpu_physical_memory_set_dirty_lebitmap() similar to
> cpu_physical_memory_sync_dirty_bitmap().
>
> To avoid counting twice when GLOBAL_DIRTY_RATE is enabled, stash the
> number of bits set per bitmap quad in a variable (@nbits) and reuse it
> there.
>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> ---
>   include/exec/ram_addr.h | 28 ++++++++++++++++++++++------
>   1 file changed, 22 insertions(+), 6 deletions(-)
>
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index 90a82692904f..9f2e3893f562 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -334,14 +334,23 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start,
>   }
>
>   #if !defined(_WIN32)
> -static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
> -                                                          ram_addr_t start,
> -                                                          ram_addr_t pages)
> +
> +/*
> + * Contrary to cpu_physical_memory_sync_dirty_bitmap() this function returns
> + * the number of dirty pages in @bitmap passed as argument. On the other hand,
> + * cpu_physical_memory_sync_dirty_bitmap() returns newly dirtied pages that
> + * weren't set in the global migration bitmap.
> + */
> +static inline
> +uint64_t cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
> +                                                ram_addr_t start,
> +                                                ram_addr_t pages)
>   {
>       unsigned long i, j;
> -    unsigned long page_number, c;
> +    unsigned long page_number, c, nbits;
>       hwaddr addr;
>       ram_addr_t ram_addr;
> +    uint64_t num_dirty = 0;
>       unsigned long len = (pages + HOST_LONG_BITS - 1) / HOST_LONG_BITS;
>       unsigned long hpratio = qemu_real_host_page_size() / TARGET_PAGE_SIZE;
>       unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
> @@ -369,6 +378,7 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>                   if (bitmap[k]) {
>                       unsigned long temp = leul_to_cpu(bitmap[k]);
>
> +                    nbits = ctpopl(temp);
>                       qatomic_or(&blocks[DIRTY_MEMORY_VGA][idx][offset], temp);
>
>                       if (global_dirty_tracking) {
> @@ -377,10 +387,12 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>                                   temp);
>                           if (unlikely(
>                               global_dirty_tracking & GLOBAL_DIRTY_DIRTY_RATE)) {
> -                            total_dirty_pages += ctpopl(temp);
> +                            total_dirty_pages += nbits;
>                           }
>                       }
>
> +                    num_dirty += nbits;
> +
>                       if (tcg_enabled()) {
>                           qatomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset],
>                                      temp);
> @@ -409,9 +421,11 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>           for (i = 0; i < len; i++) {
>               if (bitmap[i] != 0) {
>                   c = leul_to_cpu(bitmap[i]);
> +                nbits = ctpopl(c);
>                   if (unlikely(global_dirty_tracking & GLOBAL_DIRTY_DIRTY_RATE)) {
> -                    total_dirty_pages += ctpopl(c);
> +                    total_dirty_pages += nbits;
>                   }
> +                num_dirty += nbits;
>                   do {
>                       j = ctzl(c);
>                       c &= ~(1ul << j);
> @@ -424,6 +438,8 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>               }
>           }
>       }
> +
> +    return num_dirty;
>   }
>   #endif /* not _WIN32 */
>
> --
> 2.39.3
>

Re: [PATCH v3 2/2] hw/vfio: Add nr of dirty pages to vfio_get_dirty_bitmap tracepoint

[Avihai Horon]

On 29/05/2023 15:11, Joao Martins wrote:
> External email: Use caution opening links or attachments
>
Just a nit, maybe subject should be "vfio/common: Add number of dirty 
pages to vfio_get_dirty_bitmap tracepoint".

> Include the number of dirty pages on the vfio_get_dirty_bitmap tracepoint.
> These are fetched from the newly added return value in
> cpu_physical_memory_set_lebitmap().

s/cpu_physical_memory_set_lebitmap()/cpu_physical_memory_set_dirty_lebitmap()

>
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> ---
>   hw/vfio/common.c     | 7 ++++---
>   hw/vfio/trace-events | 2 +-
>   2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
> index 78358ede2764..fa8fd949b1cf 100644
> --- a/hw/vfio/common.c
> +++ b/hw/vfio/common.c
> @@ -1747,6 +1747,7 @@ static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
>   {
>       bool all_device_dirty_tracking =
>           vfio_devices_all_device_dirty_tracking(container);
> +    uint64_t dirty_pages;
>       VFIOBitmap vbmap;
>       int ret;
>
> @@ -1772,11 +1773,11 @@ static int vfio_get_dirty_bitmap(VFIOContainer *container, uint64_t iova,
>           goto out;
>       }
>
> -    cpu_physical_memory_set_dirty_lebitmap(vbmap.bitmap, ram_addr,
> -                                           vbmap.pages);
> +    dirty_pages = cpu_physical_memory_set_dirty_lebitmap(vbmap.bitmap, ram_addr,
> +                                                         vbmap.pages);
>
>       trace_vfio_get_dirty_bitmap(container->fd, iova, size, vbmap.size,
> -                                ram_addr);
> +                                ram_addr, dirty_pages);
>   out:
>       g_free(vbmap.bitmap);
>
> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
> index 646e42fd27f9..cfb60c354de3 100644
> --- a/hw/vfio/trace-events
> +++ b/hw/vfio/trace-events
> @@ -120,7 +120,7 @@ vfio_region_sparse_mmap_header(const char *name, int index, int nr_areas) "Devic
>   vfio_region_sparse_mmap_entry(int i, unsigned long start, unsigned long end) "sparse entry %d [0x%lx - 0x%lx]"
>   vfio_get_dev_region(const char *name, int index, uint32_t type, uint32_t subtype) "%s index %d, %08x/%08x"
>   vfio_dma_unmap_overflow_workaround(void) ""
> -vfio_get_dirty_bitmap(int fd, uint64_t iova, uint64_t size, uint64_t bitmap_size, uint64_t start) "container fd=%d, iova=0x%"PRIx64" size= 0x%"PRIx64" bitmap_size=0x%"PRIx64" start=0x%"PRIx64
> +vfio_get_dirty_bitmap(int fd, uint64_t iova, uint64_t size, uint64_t bitmap_size, uint64_t start, uint64_t dirty_pages) "container fd=%d, iova=0x%"PRIx64" size= 0x%"PRIx64" bitmap_size=0x%"PRIx64" start=0x%"PRIx64" dirty_pages=%"PRIu64
>   vfio_iommu_map_dirty_notify(uint64_t iova_start, uint64_t iova_end) "iommu dirty @ 0x%"PRIx64" - 0x%"PRIx64
>
>   # platform.c
> --
> 2.39.3
>

Re: [PATCH v3 1/2] exec/ram_addr: return nr of dirty pages in cpu_physical_memory_set_dirty_lebitmap()

[Joao Martins]

On 30/05/2023 09:37, Avihai Horon wrote:
> 
> On 29/05/2023 15:11, Joao Martins wrote:
>> External email: Use caution opening links or attachments
>>
> Nit, s/nr/number in the subject.
> 

Fixed (this was instinctly just trying to fit 80 columns)

>> In preparation for including the number of dirty pages in the
>> vfio_get_dirty_bitmap() tracepoint, return the number of dirty pages in
>> cpu_physical_memory_set_dirty_lebitmap() similar to
>> cpu_physical_memory_sync_dirty_bitmap().
>>
>> To avoid counting twice when GLOBAL_DIRTY_RATE is enabled, stash the
>> number of bits set per bitmap quad in a variable (@nbits) and reuse it
>> there.
>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> Reviewed-by: Peter Xu <peterx@redhat.com>
>> ---
>>   include/exec/ram_addr.h | 28 ++++++++++++++++++++++------
>>   1 file changed, 22 insertions(+), 6 deletions(-)
>>
>> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
>> index 90a82692904f..9f2e3893f562 100644
>> --- a/include/exec/ram_addr.h
>> +++ b/include/exec/ram_addr.h
>> @@ -334,14 +334,23 @@ static inline void
>> cpu_physical_memory_set_dirty_range(ram_addr_t start,
>>   }
>>
>>   #if !defined(_WIN32)
>> -static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>> -                                                          ram_addr_t start,
>> -                                                          ram_addr_t pages)
>> +
>> +/*
>> + * Contrary to cpu_physical_memory_sync_dirty_bitmap() this function returns
>> + * the number of dirty pages in @bitmap passed as argument. On the other hand,
>> + * cpu_physical_memory_sync_dirty_bitmap() returns newly dirtied pages that
>> + * weren't set in the global migration bitmap.
>> + */
>> +static inline
>> +uint64_t cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>> +                                                ram_addr_t start,
>> +                                                ram_addr_t pages)
>>   {
>>       unsigned long i, j;
>> -    unsigned long page_number, c;
>> +    unsigned long page_number, c, nbits;
>>       hwaddr addr;
>>       ram_addr_t ram_addr;
>> +    uint64_t num_dirty = 0;
>>       unsigned long len = (pages + HOST_LONG_BITS - 1) / HOST_LONG_BITS;
>>       unsigned long hpratio = qemu_real_host_page_size() / TARGET_PAGE_SIZE;
>>       unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
>> @@ -369,6 +378,7 @@ static inline void
>> cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>>                   if (bitmap[k]) {
>>                       unsigned long temp = leul_to_cpu(bitmap[k]);
>>
>> +                    nbits = ctpopl(temp);
>>                       qatomic_or(&blocks[DIRTY_MEMORY_VGA][idx][offset], temp);
>>
>>                       if (global_dirty_tracking) {
>> @@ -377,10 +387,12 @@ static inline void
>> cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>>                                   temp);
>>                           if (unlikely(
>>                               global_dirty_tracking & GLOBAL_DIRTY_DIRTY_RATE)) {
>> -                            total_dirty_pages += ctpopl(temp);
>> +                            total_dirty_pages += nbits;
>>                           }
>>                       }
>>
>> +                    num_dirty += nbits;
>> +
>>                       if (tcg_enabled()) {
>>                           qatomic_or(&blocks[DIRTY_MEMORY_CODE][idx][offset],
>>                                      temp);
>> @@ -409,9 +421,11 @@ static inline void
>> cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>>           for (i = 0; i < len; i++) {
>>               if (bitmap[i] != 0) {
>>                   c = leul_to_cpu(bitmap[i]);
>> +                nbits = ctpopl(c);
>>                   if (unlikely(global_dirty_tracking &
>> GLOBAL_DIRTY_DIRTY_RATE)) {
>> -                    total_dirty_pages += ctpopl(c);
>> +                    total_dirty_pages += nbits;
>>                   }
>> +                num_dirty += nbits;
>>                   do {
>>                       j = ctzl(c);
>>                       c &= ~(1ul << j);
>> @@ -424,6 +438,8 @@ static inline void
>> cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap,
>>               }
>>           }
>>       }
>> +
>> +    return num_dirty;
>>   }
>>   #endif /* not _WIN32 */
>>
>> -- 
>> 2.39.3
>>

Re: [PATCH v3 2/2] hw/vfio: Add nr of dirty pages to vfio_get_dirty_bitmap tracepoint

[Joao Martins]

On 30/05/2023 09:39, Avihai Horon wrote:
> 
> On 29/05/2023 15:11, Joao Martins wrote:
>> External email: Use caution opening links or attachments
>>
> Just a nit, maybe subject should be "vfio/common: Add number of dirty pages to
> vfio_get_dirty_bitmap tracepoint".
> 

Fixed

>> Include the number of dirty pages on the vfio_get_dirty_bitmap tracepoint.
>> These are fetched from the newly added return value in
>> cpu_physical_memory_set_lebitmap().
> 
> s/cpu_physical_memory_set_lebitmap()/cpu_physical_memory_set_dirty_lebitmap()
>
Fixed

>>
>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
>> ---
>>   hw/vfio/common.c     | 7 ++++---
>>   hw/vfio/trace-events | 2 +-
>>   2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/vfio/common.c b/hw/vfio/common.c
>> index 78358ede2764..fa8fd949b1cf 100644
>> --- a/hw/vfio/common.c
>> +++ b/hw/vfio/common.c
>> @@ -1747,6 +1747,7 @@ static int vfio_get_dirty_bitmap(VFIOContainer
>> *container, uint64_t iova,
>>   {
>>       bool all_device_dirty_tracking =
>>           vfio_devices_all_device_dirty_tracking(container);
>> +    uint64_t dirty_pages;
>>       VFIOBitmap vbmap;
>>       int ret;
>>
>> @@ -1772,11 +1773,11 @@ static int vfio_get_dirty_bitmap(VFIOContainer
>> *container, uint64_t iova,
>>           goto out;
>>       }
>>
>> -    cpu_physical_memory_set_dirty_lebitmap(vbmap.bitmap, ram_addr,
>> -                                           vbmap.pages);
>> +    dirty_pages = cpu_physical_memory_set_dirty_lebitmap(vbmap.bitmap, ram_addr,
>> +                                                         vbmap.pages);
>>
>>       trace_vfio_get_dirty_bitmap(container->fd, iova, size, vbmap.size,
>> -                                ram_addr);
>> +                                ram_addr, dirty_pages);
>>   out:
>>       g_free(vbmap.bitmap);
>>
>> diff --git a/hw/vfio/trace-events b/hw/vfio/trace-events
>> index 646e42fd27f9..cfb60c354de3 100644
>> --- a/hw/vfio/trace-events
>> +++ b/hw/vfio/trace-events
>> @@ -120,7 +120,7 @@ vfio_region_sparse_mmap_header(const char *name, int
>> index, int nr_areas) "Devic
>>   vfio_region_sparse_mmap_entry(int i, unsigned long start, unsigned long end)
>> "sparse entry %d [0x%lx - 0x%lx]"
>>   vfio_get_dev_region(const char *name, int index, uint32_t type, uint32_t
>> subtype) "%s index %d, %08x/%08x"
>>   vfio_dma_unmap_overflow_workaround(void) ""
>> -vfio_get_dirty_bitmap(int fd, uint64_t iova, uint64_t size, uint64_t
>> bitmap_size, uint64_t start) "container fd=%d, iova=0x%"PRIx64" size=
>> 0x%"PRIx64" bitmap_size=0x%"PRIx64" start=0x%"PRIx64
>> +vfio_get_dirty_bitmap(int fd, uint64_t iova, uint64_t size, uint64_t
>> bitmap_size, uint64_t start, uint64_t dirty_pages) "container fd=%d,
>> iova=0x%"PRIx64" size= 0x%"PRIx64" bitmap_size=0x%"PRIx64" start=0x%"PRIx64"
>> dirty_pages=%"PRIu64
>>   vfio_iommu_map_dirty_notify(uint64_t iova_start, uint64_t iova_end) "iommu
>> dirty @ 0x%"PRIx64" - 0x%"PRIx64
>>
>>   # platform.c
>> -- 
>> 2.39.3
>>

Re: [PATCH v3 2/2] hw/vfio: Add nr of dirty pages to vfio_get_dirty_bitmap tracepoint

[Philippe Mathieu-Daudé]

On 30/5/23 10:48, Joao Martins wrote:
> 
> 
> On 30/05/2023 09:39, Avihai Horon wrote:
>>
>> On 29/05/2023 15:11, Joao Martins wrote:
>>> External email: Use caution opening links or attachments
>>>
>> Just a nit, maybe subject should be "vfio/common: Add number of dirty pages to
>> vfio_get_dirty_bitmap tracepoint".
>>
> 
> Fixed
> 
>>> Include the number of dirty pages on the vfio_get_dirty_bitmap tracepoint.
>>> These are fetched from the newly added return value in
>>> cpu_physical_memory_set_lebitmap().
>>
>> s/cpu_physical_memory_set_lebitmap()/cpu_physical_memory_set_dirty_lebitmap()
>>
> Fixed

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 1/2] exec/ram_addr: return nr of dirty pages in cpu_physical_memory_set_dirty_lebitmap()

[Philippe Mathieu-Daudé]

On 29/5/23 14:11, Joao Martins wrote:
> In preparation for including the number of dirty pages in the
> vfio_get_dirty_bitmap() tracepoint, return the number of dirty pages in
> cpu_physical_memory_set_dirty_lebitmap() similar to
> cpu_physical_memory_sync_dirty_bitmap().
> 
> To avoid counting twice when GLOBAL_DIRTY_RATE is enabled, stash the
> number of bits set per bitmap quad in a variable (@nbits) and reuse it
> there.
> 
> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> Reviewed-by: Peter Xu <peterx@redhat.com>
> ---
>   include/exec/ram_addr.h | 28 ++++++++++++++++++++++------
>   1 file changed, 22 insertions(+), 6 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>