Fixups for non-migratable RAMBlocks

PATCH In review Dr. David Alan Gilbert (git) 3 participants 10 messages Jun 5, 2018 – Jun 15, 2018 1 message not in the archive

lore

Share & tools

lore

Dr. David Alan Gilbert (git)

author

2018-06-05 16:25 UTC #0 lore raw

cover Fixups for non-migratable RAMBlocks

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

Hi,
Cédric's b895de50 got merged at about the same
time as some of Peter's changes and there's a rdma case as well; so tidy up those cases that were looping over all (rather than just migratable) RAMBlocks and poison it so we don't end up doing the same thing again.

Dave

Dr. David Alan Gilbert (2):
migration: Fixes for non-migratable RAMBlocks
migration: Poison ramblock loops in migration

 include/exec/ramlist.h | 4 +++-
 migration/migration.h  | 3 +++
 migration/ram.c        | 8 +++++---
 migration/rdma.c       | 2 +-
 4 files changed, 12 insertions(+), 5 deletions(-)

Signature · 1 line

2.17.0

Dr. David Alan Gilbert (git)

author

2018-06-05 16:25 UTC #1 lore raw

PATCH 1/2 migration: Fixes for non-migratable RAMBlocks

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

There are still a few cases where migration code is using the macros and functions that do all RAMBlocks rather than just the migratable blocks; fix those up.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 migration/ram.c  | 4 ++--
 migration/rdma.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

migration/ram.c +2 -2

index a500015a2f..a7807cea84 100644
@@ -2516,7 +2516,7 @@ static void ram_state_resume_prepare(RAMState *rs, QEMUFile *out)
      * about dirty page logging as well.
      */
 
-    RAMBLOCK_FOREACH(block) {
+    RAMBLOCK_FOREACH_MIGRATABLE(block) {
         pages += bitmap_count_one(block->bmap,
                                   block->used_length >> TARGET_PAGE_BITS);
     }

@@ -3431,7 +3431,7 @@ static int ram_dirty_bitmap_sync_all(MigrationState *s, RAMState *rs)
 
     trace_ram_dirty_bitmap_sync_start();
 
-    RAMBLOCK_FOREACH(block) {
+    RAMBLOCK_FOREACH_MIGRATABLE(block) {
         qemu_savevm_send_recv_bitmap(file, block->idstr);
         trace_ram_dirty_bitmap_request(block->idstr);
         ramblock_count++;

migration/rdma.c +1 -1

index 05aee3d591..8bd7159059 100644
@@ -635,7 +635,7 @@ static int qemu_rdma_init_ram_blocks(RDMAContext *rdma)
 
     assert(rdma->blockmap == NULL);
     memset(local, 0, sizeof *local);
-    qemu_ram_foreach_block(qemu_rdma_init_one_block, rdma);
+    qemu_ram_foreach_migratable_block(qemu_rdma_init_one_block, rdma);
     trace_qemu_rdma_init_ram_blocks(local->nb_blocks);
     rdma->dest_blocks = g_new0(RDMADestBlock,
                                rdma->local_ram_blocks.nb_blocks);

Signature · 1 line

2.17.0

Peter Xu

Reviewed-by on v1 1/2

2018-06-06 03:39 UTC #2 lore raw

Re: [Qemu-devel] [PATCH 1/2] migration: Fixes for non-migratable RAMBlocks

Quoted 7 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>
There are still a few cases where migration code is using the macros and functions that do all RAMBlocks rather than just the migratable blocks; fix those up.
Signed-off-by Dr. David Alan Gilbert <[email protected]>

Indeed...

Reviewed-by Peter Xu <[email protected]>

Thanks,

Signature · 1 line

Peter Xu

Cédric Le Goater

Reviewed-by on v1 1/2

2018-06-06 18:04 UTC #3 lore raw

Re: [Qemu-devel] [PATCH 1/2] migration: Fixes for non-migratable RAMBlocks

In reply to Dr. David Alan Gilbert (git)

Quoted 7 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>
There are still a few cases where migration code is using the macros and functions that do all RAMBlocks rather than just the migratable blocks; fix those up.
Signed-off-by Dr. David Alan Gilbert <[email protected]>

Reviewed-by Cédric Le Goater <[email protected]>

Thanks,

Quoted 41 lines

---
 migration/ram.c  | 4 ++--
 migration/rdma.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

migration/ram.c +2 -2

index a500015a2f..a7807cea84 100644
@@ -2516,7 +2516,7 @@ static void ram_state_resume_prepare(RAMState *rs, QEMUFile *out)
      * about dirty page logging as well.
      */
 
-    RAMBLOCK_FOREACH(block) {
+    RAMBLOCK_FOREACH_MIGRATABLE(block) {
         pages += bitmap_count_one(block->bmap,
                                   block->used_length >> TARGET_PAGE_BITS);
     }

@@ -3431,7 +3431,7 @@ static int ram_dirty_bitmap_sync_all(MigrationState *s, RAMState *rs)
 
     trace_ram_dirty_bitmap_sync_start();
 
-    RAMBLOCK_FOREACH(block) {
+    RAMBLOCK_FOREACH_MIGRATABLE(block) {
         qemu_savevm_send_recv_bitmap(file, block->idstr);
         trace_ram_dirty_bitmap_request(block->idstr);
         ramblock_count++;

migration/rdma.c +1 -1

index 05aee3d591..8bd7159059 100644
@@ -635,7 +635,7 @@ static int qemu_rdma_init_ram_blocks(RDMAContext *rdma)
 
     assert(rdma->blockmap == NULL);
     memset(local, 0, sizeof *local);
-    qemu_ram_foreach_block(qemu_rdma_init_one_block, rdma);
+    qemu_ram_foreach_migratable_block(qemu_rdma_init_one_block, rdma);
     trace_qemu_rdma_init_ram_blocks(local->nb_blocks);
     rdma->dest_blocks = g_new0(RDMADestBlock,
                                rdma->local_ram_blocks.nb_blocks);

Dr. David Alan Gilbert (git)

author

2018-06-05 16:25 UTC #4 lore raw

PATCH 2/2 migration: Poison ramblock loops in migration

In reply to Dr. David Alan Gilbert (git)

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

The migration code should be using the
RAMBLOCK_FOREACH_MIGRATABLE and qemu_ram_foreach_block_migratable
not the all-block versions; poison them so that we can't accidentally use them.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 include/exec/ramlist.h | 4 +++-
 migration/migration.h  | 3 +++
 migration/ram.c        | 4 +++-
 3 files changed, 9 insertions(+), 2 deletions(-)

include/exec/ramlist.h +3 -1

index 2e2ac6cb99..bc4faa1b00 100644
@@ -56,8 +56,10 @@ typedef struct RAMList {
 extern RAMList ram_list;
 
 /* Should be holding either ram_list.mutex, or the RCU lock. */
-#define  RAMBLOCK_FOREACH(block)  \
+#define  INTERNAL_RAMBLOCK_FOREACH(block)  \
     QLIST_FOREACH_RCU(block, &ram_list.blocks, next)
+/* Never use the INTERNAL_ version except for defining other macros */
+#define RAMBLOCK_FOREACH(block) INTERNAL_RAMBLOCK_FOREACH(block)
 
 void qemu_mutex_lock_ramlist(void);
 void qemu_mutex_unlock_ramlist(void);

migration/migration.h +3 -0

index 5af57d616c..31d3ed12dc 100644
@@ -284,4 +284,7 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
 void dirty_bitmap_mig_before_vm_start(void);
 void init_dirty_bitmap_incoming_migration(void);
 
+#define qemu_ram_foreach_block \
+  #warning "Use qemu_ram_foreach_block_migratable in migration code"
+
 #endif

migration/ram.c +3 -1

index a7807cea84..e0d19305ee 100644
@@ -159,9 +159,11 @@ out:
 
 /* Should be holding either ram_list.mutex, or the RCU lock. */
 #define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
-    RAMBLOCK_FOREACH(block)                            \
+    INTERNAL_RAMBLOCK_FOREACH(block)                   \
         if (!qemu_ram_is_migratable(block)) {} else
 
+#undef RAMBLOCK_FOREACH
+
 static void ramblock_recv_map_init(void)
 {
     RAMBlock *rb;

Signature · 1 line

2.17.0

A message that is not in the archive (sent off-list or not yet received): 20180606033600.GA2606@xz-mi

Peter Xu

2018-06-06 03:37 UTC #6 lore raw

Re: [Qemu-devel] [PATCH 2/2] migration: Poison ramblock loops in migration

Quoted 55 lines · Peter Xu

Quoted 54 lines · Dr. David Alan Gilbert (git)

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

The migration code should be using the
RAMBLOCK_FOREACH_MIGRATABLE and qemu_ram_foreach_block_migratable
not the all-block versions; poison them so that we can't accidentally use them.

Signed-off-by Dr. David Alan Gilbert <[email protected]>

---
 include/exec/ramlist.h | 4 +++-
 migration/migration.h  | 3 +++
 migration/ram.c        | 4 +++-
 3 files changed, 9 insertions(+), 2 deletions(-)

include/exec/ramlist.h +3 -1

index 2e2ac6cb99..bc4faa1b00 100644
@@ -56,8 +56,10 @@ typedef struct RAMList {
 extern RAMList ram_list;
 
 /* Should be holding either ram_list.mutex, or the RCU lock. */
-#define  RAMBLOCK_FOREACH(block)  \
+#define  INTERNAL_RAMBLOCK_FOREACH(block)  \
     QLIST_FOREACH_RCU(block, &ram_list.blocks, next)
+/* Never use the INTERNAL_ version except for defining other macros */
+#define RAMBLOCK_FOREACH(block) INTERNAL_RAMBLOCK_FOREACH(block)
 
 void qemu_mutex_lock_ramlist(void);
 void qemu_mutex_unlock_ramlist(void);

migration/migration.h +3 -0

index 5af57d616c..31d3ed12dc 100644
@@ -284,4 +284,7 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
 void dirty_bitmap_mig_before_vm_start(void);
 void init_dirty_bitmap_incoming_migration(void);
 
+#define qemu_ram_foreach_block \
+  #warning "Use qemu_ram_foreach_block_migratable in migration code"
+
 #endif

migration/ram.c +1 -1

index a7807cea84..e0d19305ee 100644
@@ -159,9 +159,11 @@ out:
 
 /* Should be holding either ram_list.mutex, or the RCU lock. */
 #define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
-    RAMBLOCK_FOREACH(block)                            \
+    INTERNAL_RAMBLOCK_FOREACH(block)                   \
         if (!qemu_ram_is_migratable(block)) {} else

Quoted 7 lines

migration/ram.c out: · line 165
Quoted 1 line
migration/ram.c +1 -0
+#undef RAMBLOCK_FOREACH
This will only cover the ram.c file. How about we move RAMBLOCK_FOREACH_MIGRATABLE() directly into migration.h then undef RAMBLOCK_FOREACH there? Then since migration.h should be used in all the migration internal source files so the poisoned bit can be used in a broader aspect?

(I forgot to reply-all.... forwarding to the list)

Quoted 13 lines

Thanks,
Quoted 7 lines
migration/ram.c +1 -0
+
 static void ramblock_recv_map_init(void)
 {
     RAMBlock *rb;
Signature · 1 line
2.17.0
Signature · 1 line
Peter Xu

Signature · 1 line

Peter Xu

Dr. David Alan Gilbert

author

2018-06-06 08:35 UTC #7 lore raw

Re: [Qemu-devel] [PATCH 2/2] migration: Poison ramblock loops in migration

Quoted 64 lines · * Peter Xu ([email protected]) wrote:

Quoted 55 lines · Peter Xu
Quoted 54 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
The migration code should be using the
RAMBLOCK_FOREACH_MIGRATABLE and qemu_ram_foreach_block_migratable
not the all-block versions; poison them so that we can't accidentally use them.
Signed-off-by Dr. David Alan Gilbert <[email protected]>
---
 include/exec/ramlist.h | 4 +++-
 migration/migration.h  | 3 +++
 migration/ram.c        | 4 +++-
 3 files changed, 9 insertions(+), 2 deletions(-)
include/exec/ramlist.h +3 -1
index 2e2ac6cb99..bc4faa1b00 100644
@@ -56,8 +56,10 @@ typedef struct RAMList {
 extern RAMList ram_list;
 
 /* Should be holding either ram_list.mutex, or the RCU lock. */
-#define  RAMBLOCK_FOREACH(block)  \
+#define  INTERNAL_RAMBLOCK_FOREACH(block)  \
     QLIST_FOREACH_RCU(block, &ram_list.blocks, next)
+/* Never use the INTERNAL_ version except for defining other macros */
+#define RAMBLOCK_FOREACH(block) INTERNAL_RAMBLOCK_FOREACH(block)
 
 void qemu_mutex_lock_ramlist(void);
 void qemu_mutex_unlock_ramlist(void);
migration/migration.h +3 -0
index 5af57d616c..31d3ed12dc 100644
@@ -284,4 +284,7 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
 void dirty_bitmap_mig_before_vm_start(void);
 void init_dirty_bitmap_incoming_migration(void);
 
+#define qemu_ram_foreach_block \
+  #warning "Use qemu_ram_foreach_block_migratable in migration code"
+
 #endif
migration/ram.c +1 -1
index a7807cea84..e0d19305ee 100644
@@ -159,9 +159,11 @@ out:
 
 /* Should be holding either ram_list.mutex, or the RCU lock. */
 #define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
-    RAMBLOCK_FOREACH(block)                            \
+    INTERNAL_RAMBLOCK_FOREACH(block)                   \
         if (!qemu_ram_is_migratable(block)) {} else
 
Quoted 7 lines
migration/ram.c out: · line 165
Quoted 1 line
migration/ram.c +1 -0
+#undef RAMBLOCK_FOREACH
This will only cover the ram.c file. How about we move RAMBLOCK_FOREACH_MIGRATABLE() directly into migration.h then undef RAMBLOCK_FOREACH there? Then since migration.h should be used in all the migration internal source files so the poisoned bit can be used in a broader aspect?

Quoted 1 line

(I forgot to reply-all.... forwarding to the list)

Yes, we could; although it's only the ram.c that's compiled
target specific and can include the header that allows use of those macros anyway.

Dave

Quoted 11 lines

Quoted 11 lines
Thanks,
Quoted 7 lines
migration/ram.c +1 -0
+
 static void ramblock_recv_map_init(void)
 {
     RAMBlock *rb;
Signature · 1 line
2.17.0

Quoted 5 lines

Quoted 2 lines
Signature · 1 line
Peter Xu
Signature · 1 line
Peter Xu

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

Peter Xu

Reviewed-by

2018-06-06 09:09 UTC #8 lore raw

Re: [Qemu-devel] [PATCH 2/2] migration: Poison ramblock loops in migration

Quoted 65 lines · Dr. David Alan Gilbert

Quoted 64 lines · * Peter Xu ([email protected]) wrote:
Quoted 62 lines · Peter Xu
Quoted 47 lines · Dr. David Alan Gilbert (git)
From: "Dr. David Alan Gilbert" <[email protected]>
The migration code should be using the
RAMBLOCK_FOREACH_MIGRATABLE and qemu_ram_foreach_block_migratable
not the all-block versions; poison them so that we can't accidentally use them.
Signed-off-by Dr. David Alan Gilbert <[email protected]>
---
 include/exec/ramlist.h | 4 +++-
 migration/migration.h  | 3 +++
 migration/ram.c        | 4 +++-
 3 files changed, 9 insertions(+), 2 deletions(-)
include/exec/ramlist.h +3 -1
index 2e2ac6cb99..bc4faa1b00 100644
@@ -56,8 +56,10 @@ typedef struct RAMList {
 extern RAMList ram_list;
 
 /* Should be holding either ram_list.mutex, or the RCU lock. */
-#define  RAMBLOCK_FOREACH(block)  \
+#define  INTERNAL_RAMBLOCK_FOREACH(block)  \
     QLIST_FOREACH_RCU(block, &ram_list.blocks, next)
+/* Never use the INTERNAL_ version except for defining other macros */
+#define RAMBLOCK_FOREACH(block) INTERNAL_RAMBLOCK_FOREACH(block)
 
 void qemu_mutex_lock_ramlist(void);
 void qemu_mutex_unlock_ramlist(void);
migration/migration.h +3 -0
index 5af57d616c..31d3ed12dc 100644
@@ -284,4 +284,7 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
 void dirty_bitmap_mig_before_vm_start(void);
 void init_dirty_bitmap_incoming_migration(void);
 
+#define qemu_ram_foreach_block \
+  #warning "Use qemu_ram_foreach_block_migratable in migration code"
+
 #endif
migration/ram.c +0 -0
index a7807cea84..e0d19305ee 100644
migration/ram.c out: · line 165
Quoted 8 lines
migration/ram.c +2 -1
@@ -159,9 +159,11 @@ out:
 
 /* Should be holding either ram_list.mutex, or the RCU lock. */
 #define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
-    RAMBLOCK_FOREACH(block)                            \
+    INTERNAL_RAMBLOCK_FOREACH(block)                   \
         if (!qemu_ram_is_migratable(block)) {} else
 
+#undef RAMBLOCK_FOREACH
This will only cover the ram.c file. How about we move RAMBLOCK_FOREACH_MIGRATABLE() directly into migration.h then undef RAMBLOCK_FOREACH there? Then since migration.h should be used in all the migration internal source files so the poisoned bit can be used in a broader aspect?

Quoted 4 lines

Quoted 1 line
(I forgot to reply-all.... forwarding to the list)
Yes, we could; although it's only the ram.c that's compiled
target specific and can include the header that allows use of those macros anyway.

Yes, ram.c seems to be the only user. Then I'm fine with the patch:

Reviewed-by Peter Xu <[email protected]>

Thanks,

Signature · 1 line

Peter Xu

Cédric Le Goater

Reviewed-by on v1 2/2

2018-06-06 18:05 UTC #9 lore raw

Re: [Qemu-devel] [PATCH 2/2] migration: Poison ramblock loops in migration

In reply to Dr. David Alan Gilbert (git)

Quoted 8 lines · Dr. David Alan Gilbert (git)

From: "Dr. David Alan Gilbert" <[email protected]>
The migration code should be using the
RAMBLOCK_FOREACH_MIGRATABLE and qemu_ram_foreach_block_migratable
not the all-block versions; poison them so that we can't accidentally use them.
Signed-off-by Dr. David Alan Gilbert <[email protected]>

Reviewed-by Cédric Le Goater <[email protected]>

Thanks,

Quoted 52 lines

---
 include/exec/ramlist.h | 4 +++-
 migration/migration.h  | 3 +++
 migration/ram.c        | 4 +++-
 3 files changed, 9 insertions(+), 2 deletions(-)

include/exec/ramlist.h +3 -1

index 2e2ac6cb99..bc4faa1b00 100644
@@ -56,8 +56,10 @@ typedef struct RAMList {
 extern RAMList ram_list;
 
 /* Should be holding either ram_list.mutex, or the RCU lock. */
-#define  RAMBLOCK_FOREACH(block)  \
+#define  INTERNAL_RAMBLOCK_FOREACH(block)  \
     QLIST_FOREACH_RCU(block, &ram_list.blocks, next)
+/* Never use the INTERNAL_ version except for defining other macros */
+#define RAMBLOCK_FOREACH(block) INTERNAL_RAMBLOCK_FOREACH(block)
 
 void qemu_mutex_lock_ramlist(void);
 void qemu_mutex_unlock_ramlist(void);

migration/migration.h +3 -0

index 5af57d616c..31d3ed12dc 100644
@@ -284,4 +284,7 @@ void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
 void dirty_bitmap_mig_before_vm_start(void);
 void init_dirty_bitmap_incoming_migration(void);
 
+#define qemu_ram_foreach_block \
+  #warning "Use qemu_ram_foreach_block_migratable in migration code"
+
 #endif

migration/ram.c +3 -1

index a7807cea84..e0d19305ee 100644
@@ -159,9 +159,11 @@ out:
 
 /* Should be holding either ram_list.mutex, or the RCU lock. */
 #define RAMBLOCK_FOREACH_MIGRATABLE(block)             \
-    RAMBLOCK_FOREACH(block)                            \
+    INTERNAL_RAMBLOCK_FOREACH(block)                   \
         if (!qemu_ram_is_migratable(block)) {} else
 
+#undef RAMBLOCK_FOREACH
+
 static void ramblock_recv_map_init(void)
 {
     RAMBlock *rb;

Dr. David Alan Gilbert

author

2018-06-15 11:21 UTC #10 lore raw

In reply to Dr. David Alan Gilbert (git)

Quoted 9 lines · * Dr. David Alan Gilbert (git) ([email protected]) wrote:

From: "Dr. David Alan Gilbert" <[email protected]>
Hi,
Cédric's b895de50 got merged at about the same
time as some of Peter's changes and there's a rdma case as well; so tidy up those cases that were looping over all (rather than just migratable) RAMBlocks and poison it so we don't end up doing the same thing again.

Queued.

Quoted 6 lines

Dave
Dr. David Alan Gilbert (2):
migration: Fixes for non-migratable RAMBlocks
migration: Poison ramblock loops in migration

Quoted 10 lines

 include/exec/ramlist.h | 4 +++-
 migration/migration.h  | 3 +++
 migration/ram.c        | 8 +++++---
 migration/rdma.c       | 2 +-
 4 files changed, 12 insertions(+), 5 deletions(-)

Signature · 1 line

2.17.0

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