Skip to contentSkip to conversation

cpu_physical_memory_sync_dirty_bitmap: Fix alignment check

PATCH In review Dr. David Alan Gilbert (git) 3 participants 4 messages Jul 24, 2017 – Jul 25, 2017

lore
Share & tools
lore

Dr. David Alan Gilbert (git)

author
#0 lore raw

PATCH v2 1/1 cpu_physical_memory_sync_dirty_bitmap: Fix alignment check

From: Dr. David Alan Gilbert <[email protected]>

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 <[email protected]>
Reported-by Alex Benneé <[email protected]>
Tested-by Alex Benneé <[email protected]>

--
v2
Move 'page' inside the if (Comment from Paolo)

---
 include/exec/ram_addr.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
include/exec/ram_addr.h +4 -3
index c04f4f67f6..d017639f7e 100644
@@ -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();
Signature · 1 line
2.13.3
#1 lore raw
Quoted 46 lines · Dr. David Alan Gilbert (git)

From: Dr. David Alan Gilbert <[email protected]>

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 <[email protected]>
Reported-by Alex Benneé <[email protected]>
Tested-by Alex Benneé <[email protected]>

--
v2
Move 'page' inside the if (Comment from Paolo)

---
 include/exec/ram_addr.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
include/exec/ram_addr.h +3 -2
index c04f4f67f6..d017639f7e 100644
@@ -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;

include/exec/ram_addr.h uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb, · line 396

Quoted 10 lines
include/exec/ram_addr.h +1 -1
         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

author
#2 lore raw
Quoted 54 lines · * Paolo Bonzini ([email protected]) wrote:
Quoted 53 lines · Dr. David Alan Gilbert (git)

From: Dr. David Alan Gilbert <[email protected]>

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 <[email protected]>
Reported-by Alex Benneé <[email protected]>
Tested-by Alex Benneé <[email protected]>

--
v2
Move 'page' inside the if (Comment from Paolo)

---
 include/exec/ram_addr.h | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)
include/exec/ram_addr.h +4 -3
index c04f4f67f6..d017639f7e 100644
@@ -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);
Quoted 6 lines

include/exec/ram_addr.h uint64_t cpu_physical_memory_sync_dirty_bitmap(RAMBlock *rb, · line 396

Quoted 3 lines
include/exec/ram_addr.h +0 -0
         rcu_read_lock();

Thanks, do you want me to queue this patch?

Yes please.

Dave

Quoted 1 line

Paolo

--
Dr. David Alan Gilbert / [email protected] / Manchester, UK

Reviewed-by on v2 1/1
#3 lore raw
In reply to Dr. David Alan Gilbert (git)
Quoted 14 lines · Dr. David Alan Gilbert (git)

From: Dr. David Alan Gilbert <[email protected]>

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.

Quoted 6 lines
Fixes 084140bd498909
Signed-off-by Dr. David Alan Gilbert <[email protected]>
Reported-by Alex Benneé <[email protected]>
Tested-by Alex Benneé <[email protected]>
Reviewed-by Juan Quintela <[email protected]>