| Message ID | 20230525114321.71066-2-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/vfio: Improve vfio_get_dirty_bitmap() tracepoint | expand |
On Thu, May 25, 2023 at 12:43:20PM +0100, 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(). The patch itself looks good to me, but it's slightly different from sync version because that was only for MIGRATION bitmap, meanwhile it counts newly dirtied ones (so exclude already dirtied ones even if re-dirtied in the MIGRATION bitmap), while this one counts any dirty bits in *bitmap. Shall we perhaps state it somewhere explicitly? A comment for retval might be suitable above the function? Thanks, > > 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> > --- > include/exec/ram_addr.h | 21 +++++++++++++++------ > 1 file changed, 15 insertions(+), 6 deletions(-) > > diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h > index f4fb6a211175..8b8f271d0731 100644 > --- a/include/exec/ram_addr.h > +++ b/include/exec/ram_addr.h > @@ -333,14 +333,16 @@ 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) > +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); > @@ -368,6 +370,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) { > @@ -376,10 +379,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); > @@ -408,9 +413,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); > @@ -423,6 +430,8 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap, > } > } > } > + > + return num_dirty; > } > #endif /* not _WIN32 */ > > -- > 2.31.1 >
On 25/05/2023 14:16, Peter Xu wrote: > On Thu, May 25, 2023 at 12:43:20PM +0100, 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(). > > The patch itself looks good to me, but it's slightly different from sync > version because that was only for MIGRATION bitmap, meanwhile it counts > newly dirtied ones (so exclude already dirtied ones even if re-dirtied in > the MIGRATION bitmap), while this one counts any dirty bits in *bitmap. > Good callout. > Shall we perhaps state it somewhere explicitly? A comment for retval might > be suitable above the function? > Yeap, Something like this? diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h index 8b8f271d0731..deaf746421da 100644 --- a/include/exec/ram_addr.h +++ b/include/exec/ram_addr.h @@ -333,6 +333,13 @@ static inline void cpu_physical_memory_set_dirty_range(ram_addr_t start, } #if !defined(_WIN32) + +/* + * 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,
On Thu, May 25, 2023 at 02:47:26PM +0100, Joao Martins wrote: > Yeap, Something like this? > > diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h > index 8b8f271d0731..deaf746421da 100644 > --- a/include/exec/ram_addr.h > +++ b/include/exec/ram_addr.h > @@ -333,6 +333,13 @@ static inline void > cpu_physical_memory_set_dirty_range(ram_addr_t start, > } > > #if !defined(_WIN32) > + > +/* > + * 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, > Good enough to me. :) With that, feel free to add: Reviewed-by: Peter Xu <peterx@redhat.com>
On 25/05/2023 14:54, Peter Xu wrote: > On Thu, May 25, 2023 at 02:47:26PM +0100, Joao Martins wrote: >> Yeap, Something like this? >> >> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h >> index 8b8f271d0731..deaf746421da 100644 >> --- a/include/exec/ram_addr.h >> +++ b/include/exec/ram_addr.h >> @@ -333,6 +333,13 @@ static inline void >> cpu_physical_memory_set_dirty_range(ram_addr_t start, >> } >> >> #if !defined(_WIN32) >> + >> +/* >> + * 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, >> > > Good enough to me. :) With that, feel free to add: > > Reviewed-by: Peter Xu <peterx@redhat.com> > Thank you!
diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h index f4fb6a211175..8b8f271d0731 100644 --- a/include/exec/ram_addr.h +++ b/include/exec/ram_addr.h @@ -333,14 +333,16 @@ 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) +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); @@ -368,6 +370,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) { @@ -376,10 +379,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); @@ -408,9 +413,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); @@ -423,6 +430,8 @@ static inline void cpu_physical_memory_set_dirty_lebitmap(unsigned long *bitmap, } } } + + return num_dirty; } #endif /* not _WIN32 */
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> --- include/exec/ram_addr.h | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)