Re: [Xen-devel] [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value
On 25/10/17 15:55, Ian Jackson wrote: > I have reordered the quoted text, and my replies, so as to address the > most technical points first and leave what might be described as > process arguments and tone complaints for later. I will keep my reply to the technical points. I didn't enjoy writing that email, but it has unblocking things in a more productive direction. > > > Andrew Cooper writes ("Re: [PATCH for-4.10 1/2] tools/libxc: Fix > precopy_policy() to not pass a structure by value"): >> someone who does understand why hiding a prologue memcpy() is bad for >> performance. > To address your actual technical point about the memcpy. > > Not all functions in the toolstack are performance critical and not > all putative tiny performance benefits are worthwhile. Most are not. > Code should generally be written to be as clear and simple as > possible, and clarity and simplicity should be traded off for > performance only when this will produce a noticeable difference. > > Obviously clarity is a matter of opinion, but I would generally say > that a struct containing plain data is simpler than a pointer to a > similar struct. And of course passing as a pointer requires (or, in > this case, will eventually require) additional complexity in the > message generator script. > > Against that, in this case, the cost of the additional alleged-memcpy > seems to me, at first glance, to be completely irrelevant. > > Of course it probably isn't going to be a memcpy; the struct contents > will probably be passed in registers (I haven't double-checked ABI > manuals so this may be wrong on some register-poor architectures). 32bit would pass this as memory. 64bit (I think) will end up in registers with its current content and location in the parameter list, but will end up as memory if it grows any further. > Given the small size of this struct, it might well be slightly faster > to pass it in a pair of registers rather than a pointer to memory, for > locality of reference reasons. This will depend on register pressure in the callee, and whether that causes it to be spilling to the stack. I will be spilled to the stack by the callee if the structures address is ever taken. > I think the most significant proportional performance impact would be > the case where there is a callback but only within the migration > process. Ie, an out-of-tree provider of the precopy_policy hook. > (If there is no callback provided, there is no cost; and an in-tree > consumer will have the IPC cost which will dominate.) The callback is via function pointer, so can't be inlined. The default case puts simple_policy() in the hook if no hook was provided. After that, it depends how many functions are called between send_memory_live() and the variable having useful actions performed on it. >> On 19/10/17 16:17, Ian Jackson wrote: >>> Andrew Cooper writes ("Re: [PATCH for-4.10 1/2] tools/libxc: Fix >>> precopy_policy() to not pass a structure by value"): On 16/10/17 16:07, Ian Jackson wrote: > This statement is true only if you think "the precopy callback" refers > to the stub generated by libxl_save_msgs_gen. The commit is about save_callbacks.precopy_policy() specifically (and IMO, obviously). Given this, the statement is true. >>> I don't agree. >> Don't agree with what? The justification for why passing-by-value is >> supposedly necessary is bogus irrespective of whether you consider just >> the libxc part of the callback, or the end-to-end path into libxl. >> >> No argument concerning address space (separate or otherwise) is a >> related to how parameter passing needs to happen at this level. >> >> FAOD, the actual reason why it was done that way was because no-one >> wanted to edit libxl_save_msgs_gen.pl to be able to cope with pointers, >> but "$WE don't want to do it properly" is not a reasonable justification. > This argument depends on a view of "properly" which I don't share. > Ie your argument seems circular to me. My argument is not circular. The code in tree at the moment reads: /* * A precopy_policy callback may not be running in the same address * space as libxc an so precopy_stats is passed by value. */ which is factually incorrect. If the comment instead read "the libxl code generated by libxl_save_msgs_gen.pl likes to take its parameters by value", then it would be a very different matter (but still not a good enough reason IMO to break from common case and pass by pointer) hence why I adjusted the code in the way that I did. > > However, I hope it is
Re: [Xen-devel] [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value
I have reordered the quoted text, and my replies, so as to address the most technical points first and leave what might be described as process arguments and tone complaints for later. Andrew Cooper writes ("Re: [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value"): > someone who does understand why hiding a prologue memcpy() is bad for > performance. To address your actual technical point about the memcpy. Not all functions in the toolstack are performance critical and not all putative tiny performance benefits are worthwhile. Most are not. Code should generally be written to be as clear and simple as possible, and clarity and simplicity should be traded off for performance only when this will produce a noticeable difference. Obviously clarity is a matter of opinion, but I would generally say that a struct containing plain data is simpler than a pointer to a similar struct. And of course passing as a pointer requires (or, in this case, will eventually require) additional complexity in the message generator script. Against that, in this case, the cost of the additional alleged-memcpy seems to me, at first glance, to be completely irrelevant. Of course it probably isn't going to be a memcpy; the struct contents will probably be passed in registers (I haven't double-checked ABI manuals so this may be wrong on some register-poor architectures). Given the small size of this struct, it might well be slightly faster to pass it in a pair of registers rather than a pointer to memory, for locality of reference reasons. But ISTM that all of this is going to be swamped by the other costs of making a function call (at least where the callback is provided - where it isn't provided, it doesn't matter). And for in-tree consumers, the cost of the copy-by-value is going to be dwarfed by the IPC costs (as indeed you notice). If you have a better argument than "passing a struct by value should be avoided in all cases for performance reasons" you need to make it. > Having an IPC call in the middle of the live loop it is bad, and will > increase the downtime of the VM. However, the IPC call is optional > which means the common case doesn't need to suffer the overhead. > Passing by value even in the common case is an entirely unnecessary > overhead, and this fact alone is justification enough to not do it. At last, we're starting to get towards a technical argument here. I think the most significant proportional performance impact would be the case where there is a callback but only within the migration process. Ie, an out-of-tree provider of the precopy_policy hook. (If there is no callback provided, there is no cost; and an in-tree consumer will have the IPC cost which will dominate.) Would you care to hazard a guess at the quantifiable peformance loss from passing this by value, in that case ? Perhaps you would like to exhibit some assembly snippets, or show a benchmark result. > However, what is really irritating me is that, not only am I having to > divert time from more important tasks to try and fix this code before we > ship a release with it in, but that I'm having to fight you for the > privilege of maintaining that the migration code doesn't regress back > into the cesspit that was the legacy code. Since we are still talking about a libxc interface, there is no concern from an ABI/API stability point of view. If we decide, later, that the pointer is better (whether because we have changed our mind, or because of changed circumstances such as the struct growing significantly) we can just change it. Of course it's better to get it right first time so if there is a good reason. > On 19/10/17 16:17, Ian Jackson wrote: > > Andrew Cooper writes ("Re: [PATCH for-4.10 1/2] tools/libxc: Fix > > precopy_policy() to not pass a structure by value"): > >> On 16/10/17 16:07, Ian Jackson wrote: > >>> This statement is true only if you think "the precopy callback" refers > >>> to the stub generated by libxl_save_msgs_gen. > >> The commit is about save_callbacks.precopy_policy() specifically (and > >> IMO, obviously). > >> Given this, the statement is true. > > I don't agree. > > Don't agree with what? The justification for why passing-by-value is > supposedly necessary is bogus irrespective of whether you consider just > the libxc part of the callback, or the end-to-end path into libxl. > > No argument concerning address space (separate or otherwise) is a > related to how parameter passing needs to happen at this level. > > FAOD, the actual reason why it was done that way was because no-one > wanted to edit libxl_save_msgs_gen.pl to be able to cope with pointers, > but "$WE don't want to do it properly" is not a reasonable justification. This argument depends on a view of "properly" which I don't share. Ie your argument seems circular to me. However, I hope it is not necessary to resolve our disagreement over whether your proposed the commit message wording is
Re: [Xen-devel] [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value
On 19/10/17 16:17, Ian Jackson wrote: > Andrew Cooper writes ("Re: [PATCH for-4.10 1/2] tools/libxc: Fix > precopy_policy() to not pass a structure by value"): >> On 16/10/17 16:07, Ian Jackson wrote: >>> This statement is true only if you think "the precopy callback" refers >>> to the stub generated by libxl_save_msgs_gen. >> The commit is about save_callbacks.precopy_policy() specifically (and >> IMO, obviously). >> >> Given this, the statement is true. > I don't agree. Don't agree with what? The justification for why passing-by-value is supposedly necessary is bogus irrespective of whether you consider just the libxc part of the callback, or the end-to-end path into libxl. No argument concerning address space (separate or otherwise) is a related to how parameter passing needs to happen at this level. FAOD, the actual reason why it was done that way was because no-one wanted to edit libxl_save_msgs_gen.pl to be able to cope with pointers, but "$WE don't want to do it properly" is not a reasonable justification. > >>> But a more natural >>> reading is that "the precopy callback" refers to the actual code which >>> implements whatever logic is required. >>> >>> In a system using libxl, that code definitely _is_ executing in a >>> separate address space. And passing the stats by value rather than >>> reference does make it marginally easier. >> There is no libxl code for any of this. > That is of course a deficiency which we hope will be remedied, > surely. We should expect there to be libxl code. All the more reason to fix this nonsense before a libxl gains a production use. > Switch the callback to passing by pointer which is far more efficient, and drop the typedef (because none of the other callback have this oddity). >>> I would like you to expand on this efficiency argument. >> The two most common mechanisms are either to pass the object split >> across pre-agreed registers, or the compiler rearranges things to have a >> local stack object, pass by pointer, and have the prologue memcpy() it >> into local scope. >> >> The resulting change in calling convention is implementation defined, >> and subject to several different code-gen options in GCC or Clang. >> >> Therefore it is inappropriate for such an interface to exist in the >> libxenguest ABI. > I asked you to expand on an efficiency argument and instead If you don't understand the explanation in the first paragraph, then say so and I will try to explain it in more simple temrs, or defer to someone who does understand why hiding a prologue memcpy() is bad for performance. Frankly, I'm annoyed that the first patch got committed, as the code is not in a fit state and it had obvious open objections. However, what is really irritating me is that, not only am I having to divert time from more important tasks to try and fix this code before we ship a release with it in, but that I'm having to fight you for the privilege of maintaining that the migration code doesn't regress back into the cesspit that was the legacy code. The live migration algorithm is the most performance-critical part of libxenguest. Having an IPC call in the middle of the live loop it is bad, and will increase the downtime of the VM. However, the IPC call is optional which means the common case doesn't need to suffer the overhead. Passing by value even in the common case is an entirely unnecessary overhead, and this fact alone is justification enough to not do it. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value
Andrew Cooper writes ("Re: [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value"): > On 16/10/17 16:07, Ian Jackson wrote: > > This statement is true only if you think "the precopy callback" refers > > to the stub generated by libxl_save_msgs_gen. > > The commit is about save_callbacks.precopy_policy() specifically (and > IMO, obviously). > > Given this, the statement is true. I don't agree. > > But a more natural > > reading is that "the precopy callback" refers to the actual code which > > implements whatever logic is required. > > > > In a system using libxl, that code definitely _is_ executing in a > > separate address space. And passing the stats by value rather than > > reference does make it marginally easier. > > There is no libxl code for any of this. That is of course a deficiency which we hope will be remedied, surely. We should expect there to be libxl code. > >> Switch the callback to passing by pointer which is far more efficient, and > >> drop the typedef (because none of the other callback have this oddity). > > I would like you to expand on this efficiency argument. > > The two most common mechanisms are either to pass the object split > across pre-agreed registers, or the compiler rearranges things to have a > local stack object, pass by pointer, and have the prologue memcpy() it > into local scope. > > The resulting change in calling convention is implementation defined, > and subject to several different code-gen options in GCC or Clang. > > Therefore it is inappropriate for such an interface to exist in the > libxenguest ABI. I asked you to expand on an efficiency argument and instead you have provided an argument based on calling convention. Furthermore, the argument you are now presenting is simply wrong. Yes, there are options in compilers which can change the calling conventions in incompatible ways (for structs passed as arguments, as for various other things). But one must not pass those arguments when expecting to link against nonconsenting libraries. The C standard has permitted passing structs by value for a long time now, and all the (CPU architecture, operating system) combinations we support have a well-defined ABI doing so. Passing structs by value is far from unusual. Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value
On 16/10/17 16:07, Ian Jackson wrote: > Wei Liu writes ("Re: [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() > to not pass a structure by value"): >> On Mon, Oct 16, 2017 at 02:51:54PM +0100, Andrew Cooper wrote: > ... >>> With Joshua's patch in place, the implementer of this callback is the >>> code generated by libxl_save_msgs_gen.pl, which is the aformentioned >>> extra process. Passing by pointer or value has nothing to do with the >>> fact that the automatically generated code needs to know how to >>> serialise/deserialise the data to feed it back to the main process. >> Right. I agree with you here after going back to the old thread. > ISTM that the callback being a struct rather than a pointer does make > the code in libxl_save_msgs_gen.pl simpler, since it can simply memcpy > the struct. Passing by pointer does not affect libxl_save_msgs_gen.pl's ability to use memcpy(). True, the code does need to learn about indirection, but this is a trivial detail. > > I certainly dislike your 1/2 patch with the current commit message. > > Andrew Cooper writes ("[PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() > to not pass a structure by value"): >> c/s 4d69b3495 "Introduce migration precopy policy" uses bogus reasoning to >> justify passing precopy_stats by value. >> >> Under no circumstances can the precopy callback ever be executing in a >> separate address space. > This statement is true only if you think "the precopy callback" refers > to the stub generated by libxl_save_msgs_gen. The commit is about save_callbacks.precopy_policy() specifically (and IMO, obviously). Given this, the statement is true. > But a more natural > reading is that "the precopy callback" refers to the actual code which > implements whatever logic is required. > > In a system using libxl, that code definitely _is_ executing in a > separate address space. And passing the stats by value rather than > reference does make it marginally easier. There is no libxl code for any of this. > >> Switch the callback to passing by pointer which is far more efficient, and >> drop the typedef (because none of the other callback have this oddity). > I would like you to expand on this efficiency argument. The two most common mechanisms are either to pass the object split across pre-agreed registers, or the compiler rearranges things to have a local stack object, pass by pointer, and have the prologue memcpy() it into local scope. The resulting change in calling convention is implementation defined, and subject to several different code-gen options in GCC or Clang. Therefore it is inappropriate for such an interface to exist in the libxenguest ABI. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value
On Mon, Oct 16, 2017 at 04:07:32PM +0100, Ian Jackson wrote: > Wei Liu writes ("Re: [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() > to not pass a structure by value"): > > On Mon, Oct 16, 2017 at 02:51:54PM +0100, Andrew Cooper wrote: > ... > > > With Joshua's patch in place, the implementer of this callback is the > > > code generated by libxl_save_msgs_gen.pl, which is the aformentioned > > > extra process. Passing by pointer or value has nothing to do with the > > > fact that the automatically generated code needs to know how to > > > serialise/deserialise the data to feed it back to the main process. > > > > Right. I agree with you here after going back to the old thread. > > ISTM that the callback being a struct rather than a pointer does make > the code in libxl_save_msgs_gen.pl simpler, since it can simply memcpy > the struct. Yes, that's the case. > > I certainly dislike your 1/2 patch with the current commit message. > > Andrew Cooper writes ("[PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() > to not pass a structure by value"): > > c/s 4d69b3495 "Introduce migration precopy policy" uses bogus reasoning to > > justify passing precopy_stats by value. > > > > Under no circumstances can the precopy callback ever be executing in a > > separate address space. > > This statement is true only if you think "the precopy callback" refers > to the stub generated by libxl_save_msgs_gen. But a more natural > reading is that "the precopy callback" refers to the actual code which > implements whatever logic is required. > > In a system using libxl, that code definitely _is_ executing in a > separate address space. And passing the stats by value rather than > reference does make it marginally easier. > > > Switch the callback to passing by pointer which is far more efficient, and > > drop the typedef (because none of the other callback have this oddity). > > I would like you to expand on this efficiency argument. > > Certainly, with libxl (which is the primary upstream-supported > toolstack) there is no discernable efficiency gain here. The data > must be copied back and forth between processes. > This is true. > If you are talking about out-of-tree consumers then you should say > so. And you should also give a realistic explanation of the size of > the supposed performance benefit. > > (FAOD: Nacked-by: Ian Jackson) > > Sorry, > Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value
Wei Liu writes ("Re: [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value"): > On Mon, Oct 16, 2017 at 02:51:54PM +0100, Andrew Cooper wrote: ... > > With Joshua's patch in place, the implementer of this callback is the > > code generated by libxl_save_msgs_gen.pl, which is the aformentioned > > extra process. Passing by pointer or value has nothing to do with the > > fact that the automatically generated code needs to know how to > > serialise/deserialise the data to feed it back to the main process. > > Right. I agree with you here after going back to the old thread. ISTM that the callback being a struct rather than a pointer does make the code in libxl_save_msgs_gen.pl simpler, since it can simply memcpy the struct. I certainly dislike your 1/2 patch with the current commit message. Andrew Cooper writes ("[PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value"): > c/s 4d69b3495 "Introduce migration precopy policy" uses bogus reasoning to > justify passing precopy_stats by value. > > Under no circumstances can the precopy callback ever be executing in a > separate address space. This statement is true only if you think "the precopy callback" refers to the stub generated by libxl_save_msgs_gen. But a more natural reading is that "the precopy callback" refers to the actual code which implements whatever logic is required. In a system using libxl, that code definitely _is_ executing in a separate address space. And passing the stats by value rather than reference does make it marginally easier. > Switch the callback to passing by pointer which is far more efficient, and > drop the typedef (because none of the other callback have this oddity). I would like you to expand on this efficiency argument. Certainly, with libxl (which is the primary upstream-supported toolstack) there is no discernable efficiency gain here. The data must be copied back and forth between processes. If you are talking about out-of-tree consumers then you should say so. And you should also give a realistic explanation of the size of the supposed performance benefit. (FAOD: Nacked-by: Ian Jackson) Sorry, Ian. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value
On Mon, Oct 16, 2017 at 02:51:54PM +0100, Andrew Cooper wrote: > On 16/10/17 14:40, Wei Liu wrote: > > On Fri, Oct 13, 2017 at 06:32:18PM +0100, Andrew Cooper wrote: > >> c/s 4d69b3495 "Introduce migration precopy policy" uses bogus reasoning to > >> justify passing precopy_stats by value. > >> > >> Under no circumstances can the precopy callback ever be executing in a > >> separate address space. > >> > > The callback is not executed in a separate address space. > > > > Have you checked > > <1506365735-133776-4-git-send-email-jennifer.herb...@citrix.com>? > > > > The open source toolstack spawns another process to save vm image. In > > order to let libxl control the process (in the future) there is > > information passed across process boundary. > > > > Your code might work for now because Joshua's patch is not yet applied. > > I'm perfectly aware of that discussion, and it is factually incorrect. > Nothing, not even Joshua's patch, can cause the callback to be executed > in a separate address space. > > With Joshua's patch in place, the implementer of this callback is the > code generated by libxl_save_msgs_gen.pl, which is the aformentioned > extra process. Passing by pointer or value has nothing to do with the > fact that the automatically generated code needs to know how to > serialise/deserialise the data to feed it back to the main process. > Right. I agree with you here after going back to the old thread. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value
On 16/10/17 14:40, Wei Liu wrote: > On Fri, Oct 13, 2017 at 06:32:18PM +0100, Andrew Cooper wrote: >> c/s 4d69b3495 "Introduce migration precopy policy" uses bogus reasoning to >> justify passing precopy_stats by value. >> >> Under no circumstances can the precopy callback ever be executing in a >> separate address space. >> > The callback is not executed in a separate address space. > > Have you checked > <1506365735-133776-4-git-send-email-jennifer.herb...@citrix.com>? > > The open source toolstack spawns another process to save vm image. In > order to let libxl control the process (in the future) there is > information passed across process boundary. > > Your code might work for now because Joshua's patch is not yet applied. I'm perfectly aware of that discussion, and it is factually incorrect. Nothing, not even Joshua's patch, can cause the callback to be executed in a separate address space. With Joshua's patch in place, the implementer of this callback is the code generated by libxl_save_msgs_gen.pl, which is the aformentioned extra process. Passing by pointer or value has nothing to do with the fact that the automatically generated code needs to know how to serialise/deserialise the data to feed it back to the main process. ~Andrew ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value
On Fri, Oct 13, 2017 at 06:32:18PM +0100, Andrew Cooper wrote: > c/s 4d69b3495 "Introduce migration precopy policy" uses bogus reasoning to > justify passing precopy_stats by value. > > Under no circumstances can the precopy callback ever be executing in a > separate address space. > The callback is not executed in a separate address space. Have you checked <1506365735-133776-4-git-send-email-jennifer.herb...@citrix.com>? The open source toolstack spawns another process to save vm image. In order to let libxl control the process (in the future) there is information passed across process boundary. Your code might work for now because Joshua's patch is not yet applied. ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value
c/s 4d69b3495 "Introduce migration precopy policy" uses bogus reasoning to justify passing precopy_stats by value. Under no circumstances can the precopy callback ever be executing in a separate address space. Switch the callback to passing by pointer which is far more efficient, and drop the typedef (because none of the other callback have this oddity). This is no functional change, as there are no users of this interface yet. Signed-off-by: Andrew Cooper--- CC: Ian Jackson CC: Wei Liu CC: Julien Grall Appologies for this being late. I did intend to get it sorted before code freeze, but I was otherwise busy. --- tools/libxc/include/xenguest.h | 8 +--- tools/libxc/xc_sr_save.c | 19 +-- 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/tools/libxc/include/xenguest.h b/tools/libxc/include/xenguest.h index b4b2e19..f6a61ab 100644 --- a/tools/libxc/include/xenguest.h +++ b/tools/libxc/include/xenguest.h @@ -47,12 +47,6 @@ struct precopy_stats long dirty_count; /* -1 if unknown */ }; -/* - * A precopy_policy callback may not be running in the same address - * space as libxc an so precopy_stats is passed by value. - */ -typedef int (*precopy_policy_t)(struct precopy_stats, void *); - /* callbacks provided by xc_domain_save */ struct save_callbacks { /* Called after expiration of checkpoint interval, @@ -72,7 +66,7 @@ struct save_callbacks { #define XGS_POLICY_CONTINUE_PRECOPY 0 /* Remain in the precopy phase. */ #define XGS_POLICY_STOP_AND_COPY1 /* Immediately suspend and transmit the * remaining dirty pages. */ -precopy_policy_t precopy_policy; +int (*precopy_policy)(struct precopy_stats *, void *); /* * Called after the guest's dirty pages have been diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c index 5a40e58..beceb6a 100644 --- a/tools/libxc/xc_sr_save.c +++ b/tools/libxc/xc_sr_save.c @@ -478,11 +478,11 @@ static int update_progress_string(struct xc_sr_context *ctx, char **str) #define SPP_MAX_ITERATIONS 5 #define SPP_TARGET_DIRTY_COUNT 50 -static int simple_precopy_policy(struct precopy_stats stats, void *user) +static int simple_precopy_policy(struct precopy_stats *stats, void *user) { -return ((stats.dirty_count >= 0 && -stats.dirty_count < SPP_TARGET_DIRTY_COUNT) || -stats.iteration >= SPP_MAX_ITERATIONS) +return ((stats->dirty_count >= 0 && + stats->dirty_count < SPP_TARGET_DIRTY_COUNT) || +stats->iteration >= SPP_MAX_ITERATIONS) ? XGS_POLICY_STOP_AND_COPY : XGS_POLICY_CONTINUE_PRECOPY; } @@ -502,7 +502,9 @@ static int send_memory_live(struct xc_sr_context *ctx) DECLARE_HYPERCALL_BUFFER_SHADOW(unsigned long, dirty_bitmap, >save.dirty_bitmap_hbuf); -precopy_policy_t precopy_policy = ctx->save.callbacks->precopy_policy; +typeof(ctx->save.callbacks->precopy_policy) precopy_policy = +ctx->save.callbacks->precopy_policy ?: simple_precopy_policy; + void *data = ctx->save.callbacks->data; struct precopy_stats *policy_stats; @@ -515,14 +517,11 @@ static int send_memory_live(struct xc_sr_context *ctx) { .dirty_count = ctx->save.p2m_size }; policy_stats = >save.stats; -if ( precopy_policy == NULL ) - precopy_policy = simple_precopy_policy; - bitmap_set(dirty_bitmap, ctx->save.p2m_size); for ( ; ; ) { -policy_decision = precopy_policy(*policy_stats, data); +policy_decision = precopy_policy(policy_stats, data); x++; if ( stats.dirty_count > 0 && policy_decision != XGS_POLICY_ABORT ) @@ -543,7 +542,7 @@ static int send_memory_live(struct xc_sr_context *ctx) policy_stats->total_written += policy_stats->dirty_count; policy_stats->dirty_count = -1; -policy_decision = precopy_policy(*policy_stats, data); +policy_decision = precopy_policy(policy_stats, data); if ( policy_decision != XGS_POLICY_CONTINUE_PRECOPY ) break; -- 2.1.4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel