1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [PATCH v2] cpu_physical_memory_sync_dirty_bitmap: Fix alignment check

Dr. David Alan Gilbert (git) posted 1 patch 8 years, 3 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20170724165125.29887-1-dgilbert@redhat.com
Test FreeBSD passed
Test checkpatch passed
Test s390x passed
include/exec/ram_addr.h | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
[Qemu-devel] [PATCH v2] cpu_physical_memory_sync_dirty_bitmap: Fix alignment check
Posted by Dr. David Alan Gilbert (git) 8 years, 3 months ago 
From: Dr. David Alan Gilbert <dgilbert@redhat.com>

This code has an optimised, word aligned version, and a boring
unaligned version.  Recently 084140bd498909 fixed a missing offset
addition from the core of both versions.  However, the offset isn't
necessarily aligned and thus the choice between the two versions
needs fixing up to also include the offset.

Symptom:
  A few stuck unsent pages during migration; not normally noticed
unless under very low bandwidth in which case the migration may get
stuck never ending and never performing a 2nd sync; noticed by
a hanging postcopy-test on a very heavily loaded system.

Fixes: 084140bd498909

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reported-by: Alex Benneé <alex.benee@linaro.org>
Tested-by: Alex Benneé <alex.benee@linaro.org>

--
v2
  Move 'page' inside the if (Comment from Paolo)
---
 include/exec/ram_addr.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
index c04f4f67f6..d017639f7e 100644
--- a/include/exec/ram_addr.h
+++ b/include/exec/ram_addr.h
@@ -377,19 +377,20 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
                                                uint64_t *real_dirty_pages)
 {
     ram_addr_t addr;
-    unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
+    unsigned long word = BIT_WORD((start + rb->offset) >> TARGET_PAGE_BITS);
     uint64_t num_dirty = 0;
     unsigned long *dest = rb->bmap;
 
     /* start address is aligned at the start of a word? */
-    if (((page * BITS_PER_LONG) << TARGET_PAGE_BITS) == start) {
+    if (((word * BITS_PER_LONG) << TARGET_PAGE_BITS) ==
+         (start + rb->offset)) {
         int k;
         int nr = BITS_TO_LONGS(length >> TARGET_PAGE_BITS);
         unsigned long * const *src;
-        unsigned long word = BIT_WORD((start + rb->offset) >> TARGET_PAGE_BITS);
         unsigned long idx = (word * BITS_PER_LONG) / DIRTY_MEMORY_BLOCK_SIZE;
         unsigned long offset = BIT_WORD((word * BITS_PER_LONG) %
                                         DIRTY_MEMORY_BLOCK_SIZE);
+        unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
 
         rcu_read_lock();
 
-- 
2.13.3
Re: [Qemu-devel] [PATCH v2] cpu_physical_memory_sync_dirty_bitmap: Fix alignment check
Posted by Paolo Bonzini 8 years, 3 months ago 
On 24/07/2017 18:51, Dr. David Alan Gilbert (git) wrote:
> From: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> This code has an optimised, word aligned version, and a boring
> unaligned version.  Recently 084140bd498909 fixed a missing offset
> addition from the core of both versions.  However, the offset isn't
> necessarily aligned and thus the choice between the two versions
> needs fixing up to also include the offset.
> 
> Symptom:
>   A few stuck unsent pages during migration; not normally noticed
> unless under very low bandwidth in which case the migration may get
> stuck never ending and never performing a 2nd sync; noticed by
> a hanging postcopy-test on a very heavily loaded system.
> 
> Fixes: 084140bd498909
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reported-by: Alex Benneé <alex.benee@linaro.org>
> Tested-by: Alex Benneé <alex.benee@linaro.org>
> 
> --
> v2
>   Move 'page' inside the if (Comment from Paolo)
> ---
>  include/exec/ram_addr.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> index c04f4f67f6..d017639f7e 100644
> --- a/include/exec/ram_addr.h
> +++ b/include/exec/ram_addr.h
> @@ -377,19 +377,20 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
>                                                 uint64_t *real_dirty_pages)
>  {
>      ram_addr_t addr;
> -    unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
> +    unsigned long word = BIT_WORD((start + rb->offset) >> TARGET_PAGE_BITS);
>      uint64_t num_dirty = 0;
>      unsigned long *dest = rb->bmap;
>  
>      /* start address is aligned at the start of a word? */
> -    if (((page * BITS_PER_LONG) << TARGET_PAGE_BITS) == start) {
> +    if (((word * BITS_PER_LONG) << TARGET_PAGE_BITS) ==
> +         (start + rb->offset)) {
>          int k;
>          int nr = BITS_TO_LONGS(length >> TARGET_PAGE_BITS);
>          unsigned long * const *src;
> -        unsigned long word = BIT_WORD((start + rb->offset) >> TARGET_PAGE_BITS);
>          unsigned long idx = (word * BITS_PER_LONG) / DIRTY_MEMORY_BLOCK_SIZE;
>          unsigned long offset = BIT_WORD((word * BITS_PER_LONG) %
>                                          DIRTY_MEMORY_BLOCK_SIZE);
> +        unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
>  
>          rcu_read_lock();
>  
> 


Thanks, do you want me to queue this patch?

Paolo
Re: [Qemu-devel] [PATCH v2] cpu_physical_memory_sync_dirty_bitmap: Fix alignment check
Posted by Dr. David Alan Gilbert 8 years, 3 months ago 
* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 24/07/2017 18:51, Dr. David Alan Gilbert (git) wrote:
> > From: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > This code has an optimised, word aligned version, and a boring
> > unaligned version.  Recently 084140bd498909 fixed a missing offset
> > addition from the core of both versions.  However, the offset isn't
> > necessarily aligned and thus the choice between the two versions
> > needs fixing up to also include the offset.
> > 
> > Symptom:
> >   A few stuck unsent pages during migration; not normally noticed
> > unless under very low bandwidth in which case the migration may get
> > stuck never ending and never performing a 2nd sync; noticed by
> > a hanging postcopy-test on a very heavily loaded system.
> > 
> > Fixes: 084140bd498909
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Reported-by: Alex Benneé <alex.benee@linaro.org>
> > Tested-by: Alex Benneé <alex.benee@linaro.org>
> > 
> > --
> > v2
> >   Move 'page' inside the if (Comment from Paolo)
> > ---
> >  include/exec/ram_addr.h | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h
> > index c04f4f67f6..d017639f7e 100644
> > --- a/include/exec/ram_addr.h
> > +++ b/include/exec/ram_addr.h
> > @@ -377,19 +377,20 @@ uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb,
> >                                                 uint64_t *real_dirty_pages)
> >  {
> >      ram_addr_t addr;
> > -    unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
> > +    unsigned long word = BIT_WORD((start + rb->offset) >> TARGET_PAGE_BITS);
> >      uint64_t num_dirty = 0;
> >      unsigned long *dest = rb->bmap;
> >  
> >      /* start address is aligned at the start of a word? */
> > -    if (((page * BITS_PER_LONG) << TARGET_PAGE_BITS) == start) {
> > +    if (((word * BITS_PER_LONG) << TARGET_PAGE_BITS) ==
> > +         (start + rb->offset)) {
> >          int k;
> >          int nr = BITS_TO_LONGS(length >> TARGET_PAGE_BITS);
> >          unsigned long * const *src;
> > -        unsigned long word = BIT_WORD((start + rb->offset) >> TARGET_PAGE_BITS);
> >          unsigned long idx = (word * BITS_PER_LONG) / DIRTY_MEMORY_BLOCK_SIZE;
> >          unsigned long offset = BIT_WORD((word * BITS_PER_LONG) %
> >                                          DIRTY_MEMORY_BLOCK_SIZE);
> > +        unsigned long page = BIT_WORD(start >> TARGET_PAGE_BITS);
> >  
> >          rcu_read_lock();
> >  
> > 
> 
> 
> Thanks, do you want me to queue this patch?

Yes please.

Dave

> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Re: [Qemu-devel] [PATCH v2] cpu_physical_memory_sync_dirty_bitmap: Fix alignment check
Posted by Juan Quintela 8 years, 3 months ago 
"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> wrote:
> From: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> This code has an optimised, word aligned version, and a boring
> unaligned version.  Recently 084140bd498909 fixed a missing offset
> addition from the core of both versions.  However, the offset isn't
> necessarily aligned and thus the choice between the two versions
> needs fixing up to also include the offset.
>
> Symptom:
>   A few stuck unsent pages during migration; not normally noticed
> unless under very low bandwidth in which case the migration may get
> stuck never ending and never performing a 2nd sync; noticed by
> a hanging postcopy-test on a very heavily loaded system.
>
> Fixes: 084140bd498909
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Reported-by: Alex Benneé <alex.benee@linaro.org>
> Tested-by: Alex Benneé <alex.benee@linaro.org>
>

Reviewed-by: Juan Quintela <quintela@redhat.com>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.