* [Qemu-devel] [PATCH 1/2] Move dirty page search state into separate structure
2015-09-16 18:11 [Qemu-devel] [PATCH 0/2] Split up ram_find_and_save_block Dr. David Alan Gilbert (git)
@ 2015-09-16 18:11 ` Dr. David Alan Gilbert (git)
2015-09-23 12:20 ` Amit Shah
2015-09-16 18:11 ` [Qemu-devel] [PATCH 2/2] ram_find_and_save_block: Split out the finding Dr. David Alan Gilbert (git)
1 sibling, 1 reply; 6+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2015-09-16 18:11 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, quintela
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Pull the sarch state for one iteration of the dirty page
search into a structure.
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
migration/ram.c | 54 ++++++++++++++++++++++++++++++++++--------------------
1 file changed, 34 insertions(+), 20 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 7df9157..1fadf52 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -227,6 +227,17 @@ static uint64_t migration_dirty_pages;
static uint32_t last_version;
static bool ram_bulk_stage;
+/* used by the search for pages to send */
+struct PageSearchStatus {
+ /* Current block being searched */
+ RAMBlock *block;
+ /* Current offset to search from */
+ ram_addr_t offset;
+ /* Set once we wrap around */
+ bool complete_round;
+};
+typdef struct PageSearchStatus PageSearchStatus;
+
struct CompressParam {
bool start;
bool done;
@@ -531,7 +542,6 @@ static void migration_bitmap_sync_range(ram_addr_t start, ram_addr_t length)
cpu_physical_memory_sync_dirty_bitmap(bitmap, start, length);
}
-
/* Fix me: there are too many global variables used in migration process. */
static int64_t start_time;
static int64_t bytes_xfer_prev;
@@ -923,26 +933,29 @@ static int ram_save_compressed_page(QEMUFile *f, RAMBlock *block,
static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
uint64_t *bytes_transferred)
{
- RAMBlock *block = last_seen_block;
- ram_addr_t offset = last_offset;
- bool complete_round = false;
+ PageSearchStatus pss;
int pages = 0;
- if (!block)
- block = QLIST_FIRST_RCU(&ram_list.blocks);
+ pss.block = last_seen_block;
+ pss.offset = last_offset;
+ pss.complete_round = false;
+
+ if (!pss.block)
+ pss.block = QLIST_FIRST_RCU(&ram_list.blocks);
while (true) {
- offset = migration_bitmap_find_and_reset_dirty(block, offset);
- if (complete_round && block == last_seen_block &&
- offset >= last_offset) {
+ pss.offset = migration_bitmap_find_and_reset_dirty(pss.block,
+ pss.offset);
+ if (pss.complete_round && pss.block == last_seen_block &&
+ pss.offset >= last_offset) {
break;
}
- if (offset >= block->used_length) {
- offset = 0;
- block = QLIST_NEXT_RCU(block, next);
- if (!block) {
- block = QLIST_FIRST_RCU(&ram_list.blocks);
- complete_round = true;
+ if (pss.offset >= pss.block->used_length) {
+ pss.offset = 0;
+ pss.block = QLIST_NEXT_RCU(pss.block, next);
+ if (!pss.block) {
+ pss.block = QLIST_FIRST_RCU(&ram_list.blocks);
+ pss.complete_round = true;
ram_bulk_stage = false;
if (migrate_use_xbzrle()) {
/* If xbzrle is on, stop using the data compression at this
@@ -954,23 +967,24 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
}
} else {
if (compression_switch && migrate_use_compression()) {
- pages = ram_save_compressed_page(f, block, offset, last_stage,
+ pages = ram_save_compressed_page(f, pss.block, pss.offset,
+ last_stage,
bytes_transferred);
} else {
- pages = ram_save_page(f, block, offset, last_stage,
+ pages = ram_save_page(f, pss.block, pss.offset, last_stage,
bytes_transferred);
}
/* if page is unmodified, continue to the next */
if (pages > 0) {
- last_sent_block = block;
+ last_sent_block = pss.block;
break;
}
}
}
- last_seen_block = block;
- last_offset = offset;
+ last_seen_block = pss.block;
+ last_offset = pss.offset;
return pages;
}
--
2.4.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 1/2] Move dirty page search state into separate structure
2015-09-16 18:11 ` [Qemu-devel] [PATCH 1/2] Move dirty page search state into separate structure Dr. David Alan Gilbert (git)
@ 2015-09-23 12:20 ` Amit Shah
0 siblings, 0 replies; 6+ messages in thread
From: Amit Shah @ 2015-09-23 12:20 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git); +Cc: qemu-devel, quintela
On (Wed) 16 Sep 2015 [19:11:53], Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Pull the sarch state for one iteration of the dirty page
typo in 'search'
> search into a structure.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> @@ -923,26 +933,29 @@ static int ram_save_compressed_page(QEMUFile *f, RAMBlock *block,
> static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
> uint64_t *bytes_transferred)
> {
> - RAMBlock *block = last_seen_block;
> - ram_addr_t offset = last_offset;
> - bool complete_round = false;
> + PageSearchStatus pss;
> int pages = 0;
>
> - if (!block)
> - block = QLIST_FIRST_RCU(&ram_list.blocks);
> + pss.block = last_seen_block;
> + pss.offset = last_offset;
> + pss.complete_round = false;
> +
> + if (!pss.block)
> + pss.block = QLIST_FIRST_RCU(&ram_list.blocks);
Missing {} around if
With those fixed:
Reviewed-by: Amit Shah <amit.shah@redhat.com>
Amit
^ permalink raw reply [flat|nested] 6+ messages in thread
* [Qemu-devel] [PATCH 2/2] ram_find_and_save_block: Split out the finding
2015-09-16 18:11 [Qemu-devel] [PATCH 0/2] Split up ram_find_and_save_block Dr. David Alan Gilbert (git)
2015-09-16 18:11 ` [Qemu-devel] [PATCH 1/2] Move dirty page search state into separate structure Dr. David Alan Gilbert (git)
@ 2015-09-16 18:11 ` Dr. David Alan Gilbert (git)
2015-09-23 12:26 ` Amit Shah
1 sibling, 1 reply; 6+ messages in thread
From: Dr. David Alan Gilbert (git) @ 2015-09-16 18:11 UTC (permalink / raw)
To: qemu-devel; +Cc: amit.shah, quintela
From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Split out the finding of the dirty page and all the wrap detection
into a separate function since it was getting a bit hairy.
Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
migration/ram.c | 87 ++++++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 61 insertions(+), 26 deletions(-)
diff --git a/migration/ram.c b/migration/ram.c
index 1fadf52..d2982e9 100644
--- a/migration/ram.c
+++ b/migration/ram.c
@@ -236,7 +236,7 @@ struct PageSearchStatus {
/* Set once we wrap around */
bool complete_round;
};
-typdef struct PageSearchStatus PageSearchStatus;
+typedef struct PageSearchStatus PageSearchStatus;
struct CompressParam {
bool start;
@@ -917,6 +917,59 @@ static int ram_save_compressed_page(QEMUFile *f, RAMBlock *block,
return pages;
}
+/*
+ * Find the next dirty page and update any state associated with
+ * the search process.
+ *
+ * Returns: True if a page is found
+ *
+ * @f: Current migration stream.
+ * @pss: Data about the state of the current dirty page scan.
+ * @*again: Set to false if the search has scanned the whole of RAM
+ */
+static bool find_dirty_block(QEMUFile *f, PageSearchStatus *pss,
+ bool *again)
+{
+ pss->offset = migration_bitmap_find_and_reset_dirty(pss->block,
+ pss->offset);
+ if (pss->complete_round && pss->block == last_seen_block &&
+ pss->offset >= last_offset) {
+ /*
+ * We've been once around the RAM and haven't found anything
+ * give up.
+ */
+ *again = false;
+ return false;
+ }
+ if (pss->offset >= pss->block->used_length) {
+ /* Didn't find anything in this RAM Block */
+ pss->offset = 0;
+ pss->block = QLIST_NEXT_RCU(pss->block, next);
+ if (!pss->block) {
+ /* Hit the end of the list */
+ pss->block = QLIST_FIRST_RCU(&ram_list.blocks);
+ /* Flag that we've looped */
+ pss->complete_round = true;
+ ram_bulk_stage = false;
+ if (migrate_use_xbzrle()) {
+ /* If xbzrle is on, stop using the data compression at this
+ * point. In theory, xbzrle can do better than compression.
+ */
+ flush_compressed_data(f);
+ compression_switch = false;
+ }
+ }
+ /* Didn't find anything this time, but try again on the new block */
+ *again = true;
+ return false;
+ } else {
+ /* Can go around again, but... */
+ *again = true;
+ /* We've found something so probably don't need to */
+ return true;
+ }
+}
+
/**
* ram_find_and_save_block: Finds a dirty page and sends it to f
*
@@ -935,6 +988,7 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
{
PageSearchStatus pss;
int pages = 0;
+ bool again, found;
pss.block = last_seen_block;
pss.offset = last_offset;
@@ -943,29 +997,11 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
if (!pss.block)
pss.block = QLIST_FIRST_RCU(&ram_list.blocks);
- while (true) {
- pss.offset = migration_bitmap_find_and_reset_dirty(pss.block,
- pss.offset);
- if (pss.complete_round && pss.block == last_seen_block &&
- pss.offset >= last_offset) {
- break;
- }
- if (pss.offset >= pss.block->used_length) {
- pss.offset = 0;
- pss.block = QLIST_NEXT_RCU(pss.block, next);
- if (!pss.block) {
- pss.block = QLIST_FIRST_RCU(&ram_list.blocks);
- pss.complete_round = true;
- ram_bulk_stage = false;
- if (migrate_use_xbzrle()) {
- /* If xbzrle is on, stop using the data compression at this
- * point. In theory, xbzrle can do better than compression.
- */
- flush_compressed_data(f);
- compression_switch = false;
- }
- }
- } else {
+ do {
+ again = true;
+ found = find_dirty_block(f, &pss, &again);
+
+ if (found) {
if (compression_switch && migrate_use_compression()) {
pages = ram_save_compressed_page(f, pss.block, pss.offset,
last_stage,
@@ -978,10 +1014,9 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
/* if page is unmodified, continue to the next */
if (pages > 0) {
last_sent_block = pss.block;
- break;
}
}
- }
+ } while (!pages && again);
last_seen_block = pss.block;
last_offset = pss.offset;
--
2.4.3
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 2/2] ram_find_and_save_block: Split out the finding
2015-09-16 18:11 ` [Qemu-devel] [PATCH 2/2] ram_find_and_save_block: Split out the finding Dr. David Alan Gilbert (git)
@ 2015-09-23 12:26 ` Amit Shah
2015-09-23 12:42 ` Dr. David Alan Gilbert
0 siblings, 1 reply; 6+ messages in thread
From: Amit Shah @ 2015-09-23 12:26 UTC (permalink / raw)
To: Dr. David Alan Gilbert (git); +Cc: qemu-devel, quintela
On (Wed) 16 Sep 2015 [19:11:54], Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Split out the finding of the dirty page and all the wrap detection
> into a separate function since it was getting a bit hairy.
>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> migration/ram.c | 87 ++++++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 61 insertions(+), 26 deletions(-)
>
> diff --git a/migration/ram.c b/migration/ram.c
> index 1fadf52..d2982e9 100644
> --- a/migration/ram.c
> +++ b/migration/ram.c
> @@ -236,7 +236,7 @@ struct PageSearchStatus {
> /* Set once we wrap around */
> bool complete_round;
> };
> -typdef struct PageSearchStatus PageSearchStatus;
> +typedef struct PageSearchStatus PageSearchStatus;
:-)
> +/*
> + * Find the next dirty page and update any state associated with
> + * the search process.
> + *
> + * Returns: True if a page is found
> + *
> + * @f: Current migration stream.
> + * @pss: Data about the state of the current dirty page scan.
> + * @*again: Set to false if the search has scanned the whole of RAM
> + */
> +static bool find_dirty_block(QEMUFile *f, PageSearchStatus *pss,
> + bool *again)
> +{
> + pss->offset = migration_bitmap_find_and_reset_dirty(pss->block,
> + pss->offset);
> + if (pss->complete_round && pss->block == last_seen_block &&
> + pss->offset >= last_offset) {
> + /*
> + * We've been once around the RAM and haven't found anything
> + * give up.
> + */
> + *again = false;
> + return false;
> + }
> + if (pss->offset >= pss->block->used_length) {
> + /* Didn't find anything in this RAM Block */
> + pss->offset = 0;
> + pss->block = QLIST_NEXT_RCU(pss->block, next);
> + if (!pss->block) {
> + /* Hit the end of the list */
> + pss->block = QLIST_FIRST_RCU(&ram_list.blocks);
> + /* Flag that we've looped */
> + pss->complete_round = true;
> + ram_bulk_stage = false;
> + if (migrate_use_xbzrle()) {
> + /* If xbzrle is on, stop using the data compression at this
> + * point. In theory, xbzrle can do better than compression.
> + */
> + flush_compressed_data(f);
> + compression_switch = false;
> + }
> + }
> + /* Didn't find anything this time, but try again on the new block */
> + *again = true;
> + return false;
> + } else {
> + /* Can go around again, but... */
> + *again = true;
> + /* We've found something so probably don't need to */
> + return true;
These comments are very good. Had you not introduced them, I'd have
recommended to just set *again to true before the if; and get the
'return true' case handled earlier so that all the nesting too goes
off (each case has a return, so no point in continuing with if..else).
Also, the dance with the return value and the 'again' is also slightly
complex -- but I doubt it can be made any simpler.
> + }
> +}
> +
> /**
> * ram_find_and_save_block: Finds a dirty page and sends it to f
> *
> @@ -935,6 +988,7 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
> {
> PageSearchStatus pss;
> int pages = 0;
> + bool again, found;
>
> pss.block = last_seen_block;
> pss.offset = last_offset;
> @@ -943,29 +997,11 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
> if (!pss.block)
> pss.block = QLIST_FIRST_RCU(&ram_list.blocks);
>
> - while (true) {
> - pss.offset = migration_bitmap_find_and_reset_dirty(pss.block,
> - pss.offset);
> - if (pss.complete_round && pss.block == last_seen_block &&
> - pss.offset >= last_offset) {
> - break;
> - }
> - if (pss.offset >= pss.block->used_length) {
> - pss.offset = 0;
> - pss.block = QLIST_NEXT_RCU(pss.block, next);
> - if (!pss.block) {
> - pss.block = QLIST_FIRST_RCU(&ram_list.blocks);
> - pss.complete_round = true;
> - ram_bulk_stage = false;
> - if (migrate_use_xbzrle()) {
> - /* If xbzrle is on, stop using the data compression at this
> - * point. In theory, xbzrle can do better than compression.
> - */
> - flush_compressed_data(f);
> - compression_switch = false;
> - }
> - }
> - } else {
> + do {
> + again = true;
This assignment isn't needed -- did any tool complain?
> + found = find_dirty_block(f, &pss, &again);
> +
> + if (found) {
Amit
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PATCH 2/2] ram_find_and_save_block: Split out the finding
2015-09-23 12:26 ` Amit Shah
@ 2015-09-23 12:42 ` Dr. David Alan Gilbert
0 siblings, 0 replies; 6+ messages in thread
From: Dr. David Alan Gilbert @ 2015-09-23 12:42 UTC (permalink / raw)
To: Amit Shah; +Cc: qemu-devel, quintela
* Amit Shah (amit.shah@redhat.com) wrote:
> On (Wed) 16 Sep 2015 [19:11:54], Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Split out the finding of the dirty page and all the wrap detection
> > into a separate function since it was getting a bit hairy.
> >
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> > migration/ram.c | 87 ++++++++++++++++++++++++++++++++++++++++-----------------
> > 1 file changed, 61 insertions(+), 26 deletions(-)
> >
> > diff --git a/migration/ram.c b/migration/ram.c
> > index 1fadf52..d2982e9 100644
> > --- a/migration/ram.c
> > +++ b/migration/ram.c
> > @@ -236,7 +236,7 @@ struct PageSearchStatus {
> > /* Set once we wrap around */
> > bool complete_round;
> > };
> > -typdef struct PageSearchStatus PageSearchStatus;
> > +typedef struct PageSearchStatus PageSearchStatus;
>
> :-)
Ahem - I didn't retest after I split it into two subpatches; thanks; I'll recut this soon.
> > +/*
> > + * Find the next dirty page and update any state associated with
> > + * the search process.
> > + *
> > + * Returns: True if a page is found
> > + *
> > + * @f: Current migration stream.
> > + * @pss: Data about the state of the current dirty page scan.
> > + * @*again: Set to false if the search has scanned the whole of RAM
> > + */
> > +static bool find_dirty_block(QEMUFile *f, PageSearchStatus *pss,
> > + bool *again)
> > +{
> > + pss->offset = migration_bitmap_find_and_reset_dirty(pss->block,
> > + pss->offset);
> > + if (pss->complete_round && pss->block == last_seen_block &&
> > + pss->offset >= last_offset) {
> > + /*
> > + * We've been once around the RAM and haven't found anything
> > + * give up.
> > + */
> > + *again = false;
> > + return false;
> > + }
> > + if (pss->offset >= pss->block->used_length) {
> > + /* Didn't find anything in this RAM Block */
> > + pss->offset = 0;
> > + pss->block = QLIST_NEXT_RCU(pss->block, next);
> > + if (!pss->block) {
> > + /* Hit the end of the list */
> > + pss->block = QLIST_FIRST_RCU(&ram_list.blocks);
> > + /* Flag that we've looped */
> > + pss->complete_round = true;
> > + ram_bulk_stage = false;
> > + if (migrate_use_xbzrle()) {
> > + /* If xbzrle is on, stop using the data compression at this
> > + * point. In theory, xbzrle can do better than compression.
> > + */
> > + flush_compressed_data(f);
> > + compression_switch = false;
> > + }
> > + }
> > + /* Didn't find anything this time, but try again on the new block */
> > + *again = true;
> > + return false;
> > + } else {
> > + /* Can go around again, but... */
> > + *again = true;
> > + /* We've found something so probably don't need to */
> > + return true;
>
> These comments are very good. Had you not introduced them, I'd have
> recommended to just set *again to true before the if; and get the
> 'return true' case handled earlier so that all the nesting too goes
> off (each case has a return, so no point in continuing with if..else).
Yes, I needed to add them to help me understand what was going on.
> Also, the dance with the return value and the 'again' is also slightly
> complex -- but I doubt it can be made any simpler.
>
> > + }
> > +}
> > +
> > /**
> > * ram_find_and_save_block: Finds a dirty page and sends it to f
> > *
> > @@ -935,6 +988,7 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
> > {
> > PageSearchStatus pss;
> > int pages = 0;
> > + bool again, found;
> >
> > pss.block = last_seen_block;
> > pss.offset = last_offset;
> > @@ -943,29 +997,11 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage,
> > if (!pss.block)
> > pss.block = QLIST_FIRST_RCU(&ram_list.blocks);
> >
> > - while (true) {
> > - pss.offset = migration_bitmap_find_and_reset_dirty(pss.block,
> > - pss.offset);
> > - if (pss.complete_round && pss.block == last_seen_block &&
> > - pss.offset >= last_offset) {
> > - break;
> > - }
> > - if (pss.offset >= pss.block->used_length) {
> > - pss.offset = 0;
> > - pss.block = QLIST_NEXT_RCU(pss.block, next);
> > - if (!pss.block) {
> > - pss.block = QLIST_FIRST_RCU(&ram_list.blocks);
> > - pss.complete_round = true;
> > - ram_bulk_stage = false;
> > - if (migrate_use_xbzrle()) {
> > - /* If xbzrle is on, stop using the data compression at this
> > - * point. In theory, xbzrle can do better than compression.
> > - */
> > - flush_compressed_data(f);
> > - compression_switch = false;
> > - }
> > - }
> > - } else {
> > + do {
> > + again = true;
>
> This assignment isn't needed -- did any tool complain?
No, it just seemed neater; I'll remove it when I repost.
Dave
>
> > + found = find_dirty_block(f, &pss, &again);
> > +
> > + if (found) {
>
>
> Amit
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 6+ messages in thread