[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v6 COLO 06/15] libxc/save: support COLO save
- To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, <xen-devel@xxxxxxxxxxxxx>
- From: Yang Hongyang <yanghy@xxxxxxxxxxxxxx>
- Date: Tue, 9 Jun 2015 16:45:25 +0800
- Cc: wei.liu2@xxxxxxxxxx, ian.campbell@xxxxxxxxxx, wency@xxxxxxxxxxxxxx, guijianfeng@xxxxxxxxxxxxxx, yunhong.jiang@xxxxxxxxx, eddie.dong@xxxxxxxxx, rshriram@xxxxxxxxx, ian.jackson@xxxxxxxxxxxxx
- Delivery-date: Tue, 09 Jun 2015 08:45:52 +0000
- List-id: Xen developer discussion <xen-devel.lists.xen.org>
On 06/09/2015 03:20 PM, Andrew Cooper wrote:
On 09/06/2015 04:15, Yang Hongyang wrote:
On 06/08/2015 09:04 PM, Andrew Cooper wrote:
On 08/06/15 04:45, Yang Hongyang wrote:
call callbacks->get_dirty_pfn() after suspend primary vm to
get dirty pages on secondary vm, and send pages both dirty on
primary/secondary to secondary.
Signed-off-by: Yang Hongyang <yanghy@xxxxxxxxxxxxxx>
Signed-off-by: Wen Congyang <wency@xxxxxxxxxxxxxx>
CC: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
tools/libxc/xc_sr_save.c | 49
+++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 48 insertions(+), 1 deletion(-)
diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index d63b783..cda61ed 100644
--- a/tools/libxc/xc_sr_save.c
+++ b/tools/libxc/xc_sr_save.c
@@ -515,6 +515,31 @@ static int send_memory_live(struct
xc_sr_context *ctx)
return rc;
}
+static int update_dirty_bitmap(uint8_t *(*get_dirty_pfn)(void *),
void *data,
+ unsigned long p2m_size, unsigned
long *bitmap)
This function should take a ctx rather than having the caller expand 3
parameters. Also, "update_dirty_bitmap" is a little misleading, as it
isn't querying the hypervisor for the dirty bitmap.
ok.
(Merging the other thread)
how about merge_secondary_dirty_bitmap()?
Much better!
+{
+ uint64_t *pfn_list;
+ uint64_t count, i;
+ uint64_t pfn;
+
+ pfn_list = (uint64_t *)get_dirty_pfn(data);
This looks like a recipe for width-errors. The get_dirty_pfn() call
should take a pointer to a struct for it to fill.
but the size is unknown for the caller.pfn_list[0] is the count of
pfn.
+ assert(pfn_list);
This should turn into an error rather than an abort().
Even if there are no dirty pages on secondary, pfn_list shouldn't be
NULL, it's just that pfn_list[0] will be 0. if pfn_list is NULL,
there might be unexpected error happened.
get_dirty_pfn() should be declared alongside a
struct pfn_data
{
uint64_t count;
uint64_t *pfns;
};
and this function here should create one of these on the stack and pass
it by pointer to get_dirty_pfn(). I might also be tempted to rename
this to get_remote_logdirty() or similar, to indicate that it is a
source of logdirty data from something other than the current hypervisor.
This is a callback, I can't find a way to pass pointer from libxc to libxl,
libxl can not access the pointer data...The struct can be used for represent
the data however.
I like with the rename part, sounds much better.
+
+ count = pfn_list[0];
+ for (i = 0; i < count; i++) {
style
+ pfn = pfn_list[i + 1];
+ if (pfn > p2m_size) {
+ errno = EINVAL;
+ return -1;
+ }
+
+ set_bit(pfn, bitmap);
+ }
+
+ free(pfn_list);
+ return 0;
+}
+
/*
* Suspend the domain and send dirty memory.
* This is the last iteration of the live migration and the
@@ -555,6 +580,19 @@ static int suspend_and_send_dirty(struct
xc_sr_context *ctx)
bitmap_or(dirty_bitmap, ctx->save.deferred_pages,
ctx->save.p2m_size);
+ if ( !ctx->save.live && ctx->save.callbacks->get_dirty_pfn )
+ {
Shouldn't get_dirty_pfn be mandatory for COLO streams (even if it is a
noop to start with) ?
It should be mandatory, it shouldn't be noop under COLO. perhaps we
should
add sanity check at the beginning. But problem is save side do not
have a param
passed from libxl to indicate the stream type(like checkpointed_stream in
restore side). So we may need to add another XCFLAGS? Currently there is
XCFLAGS_CHECKPOINTED which represents Remus, we might need to change
this to
XCFLAGS_STREAM_REMUS
XCFLAGS_STREAM_COLO
so that we can know what kind of stream we are handling?
checkpointed_stream started out as a bugfix for a legacy stream
migration breakage. Really, this information should have been passed
right from the start.
Did I miss the bugfix? is it not in upstream?
It would probably be best to take the enum{} suggested elsewhere and
make it a top level ctx item, and have it present for both save and
restore, with sutable parameters passed in from the top. (When I am
finally able to take out the legacy code, there is going to be a severe
pruning/consolidation of the parameters.)
This is what I thought when I saw the enum{} suggested.
~Andrew
~Andrew
+ rc = update_dirty_bitmap(ctx->save.callbacks->get_dirty_pfn,
+ ctx->save.callbacks->data,
+ ctx->save.p2m_size,
+ dirty_bitmap);
+ if ( rc )
+ {
+ PERROR("Failed to get secondary vm's dirty pages");
+ goto out;
+ }
+ }
+
rc = send_dirty_pages(ctx, stats.dirty_count +
ctx->save.nr_deferred_pages);
if ( rc )
goto out;
@@ -784,7 +822,16 @@ static int save(struct xc_sr_context *ctx,
uint16_t guest_type)
if ( rc )
goto err;
- ctx->save.callbacks->postcopy(ctx->save.callbacks->data);
+ rc =
ctx->save.callbacks->postcopy(ctx->save.callbacks->data);
+ if ( !rc ) {
+ if ( !errno )
+ {
+ /* Postcopy request failed (without errno,
using EINVAL) */
+ errno = EINVAL;
+ }
+ rc = -1;
+ goto err;
+ }
rc =
ctx->save.callbacks->checkpoint(ctx->save.callbacks->data);
if ( rc <= 0 )
.
.
--
Thanks,
Yang.
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
