[PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value

[PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value

[Andrew Cooper]

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 <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>

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_COPY    1  /* 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,
                                     &ctx->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 = &ctx->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

[PATCH for-4.10 2/2] tools/libxc: Fix various code smells in send_memory_live()

[Andrew Cooper]

 * Don't zero ctx->save.stats; it is already zeroed
 * No need for x as it duplicates ctx->save.stats.iteration
 * Defer setting dirty_count until the bitmap has been filled to match the
   behaviour of XEN_DOMCTL_SHADOW_OP_CLEAN
 * Drop spurious blank line

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>
---
 tools/libxc/xc_sr_save.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/tools/libxc/xc_sr_save.c b/tools/libxc/xc_sr_save.c
index beceb6a..afc5cb9 100644
--- a/tools/libxc/xc_sr_save.c
+++ b/tools/libxc/xc_sr_save.c
@@ -495,7 +495,6 @@ static int send_memory_live(struct xc_sr_context *ctx)
     xc_interface *xch = ctx->xch;
     xc_shadow_op_stats_t stats = { 0, ctx->save.p2m_size };
     char *progress_str = NULL;
-    unsigned int x = 0;
     int rc;
     int policy_decision;
 
@@ -506,23 +505,18 @@ static int send_memory_live(struct xc_sr_context *ctx)
         ctx->save.callbacks->precopy_policy ?: simple_precopy_policy;
 
     void *data = ctx->save.callbacks->data;
-
-    struct precopy_stats *policy_stats;
+    struct precopy_stats *policy_stats = &ctx->save.stats;
 
     rc = update_progress_string(ctx, &progress_str);
     if ( rc )
         goto out;
 
-    ctx->save.stats = (struct precopy_stats)
-        { .dirty_count   = ctx->save.p2m_size };
-    policy_stats = &ctx->save.stats;
-
     bitmap_set(dirty_bitmap, ctx->save.p2m_size);
+    policy_stats->dirty_count = ctx->save.p2m_size;
 
     for ( ; ; )
     {
         policy_decision = precopy_policy(policy_stats, data);
-        x++;
 
         if ( stats.dirty_count > 0 && policy_decision != XGS_POLICY_ABORT )
         {
@@ -538,7 +532,7 @@ static int send_memory_live(struct xc_sr_context *ctx)
         if ( policy_decision != XGS_POLICY_CONTINUE_PRECOPY )
             break;
 
-        policy_stats->iteration     = x;
+        policy_stats->iteration++;
         policy_stats->total_written += policy_stats->dirty_count;
         policy_stats->dirty_count   = -1;
 
@@ -558,7 +552,6 @@ static int send_memory_live(struct xc_sr_context *ctx)
         }
 
         policy_stats->dirty_count = stats.dirty_count;
-
     }
 
  out:
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value

[Wei Liu]

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.Herbert@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

Re: [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value

[Andrew Cooper]

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.Herbert@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: [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value

[Wei Liu]

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.Herbert@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: [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value

[Ian Jackson]

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 <ian.jackson@eu.citrix.com>)

Sorry,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10 2/2] tools/libxc: Fix various code smells in send_memory_live()

[Ian Jackson]

~Andrew Cooper writes ("[PATCH for-4.10 2/2] tools/libxc: Fix various code smells in send_memory_live()"):
>  * Don't zero ctx->save.stats; it is already zeroed
>  * No need for x as it duplicates ctx->save.stats.iteration
>  * Defer setting dirty_count until the bitmap has been filled to match the
>    behaviour of XEN_DOMCTL_SHADOW_OP_CLEAN
>  * Drop spurious blank line

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value

[Wei Liu]

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 <ian.jackson@eu.citrix.com>)
> 
> Sorry,
> Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value

[Andrew Cooper]

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: [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value

[Ian Jackson]

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: [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value

[Andrew Cooper]

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: [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value

[Ian Jackson]

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 accurate, or
whether it is a calumny.  I'm sure we can find some way to write the
commit message that would avoid the claim I want you to avoid.

How about:

  This was originally passed as a parameter to avoid having to (in the
  future) teach libxl_save_msgs_gen.pl to copy (by value) a struct
  which is referred to by a pointer parameter to a hook function.

  However, it is preferable for that parameter to be a pointer because
  ...


I appreciate that you are trying to keep the code at a high level of
quality.  But that some people disagree with you about what
constitutes high quality does not mean that you should feel free to
insult them, or their work.  (See your tendentious comment about
"cesspit" I quote above, too.)

> > 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.

I think this talk of "nonsense" is unhelpful.  Please concentrate on
your technical reasons for preferring the pointer argument.


> >>>> 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.

Now I am actually annoyed.

As I tried to explain, I asked you to expand on an efficiency argument
(which is the objection you are trying to put forward) and instead you
provided a novel argument based on calling convention incompatibility.

The argument you make in your sentences "The resulting change ... is
implementation defined ... ABI" has nothing to do with performance.
It is an explanation of how passing structs as values is inappropriate
an API/ABI for calling convention compatibility reasons.

That argument is, as I explained, both (i) simply false (it may have
been true in 1990) (ii) not the same as the peformance argument I was
reasonably asking you to expand on.

When I pointed this out, you resorted to accusing me of ignorance.
That is a completely inappropriate way of carrying on the
conversation.


> 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.

I was aware of your objection when the patch was committed.  I just
didn't agree with it.

An objection is not a veto.  As the maintainer I have the
responsibility to consider objections, but I have the responsibility
to override them if, after discussion, I don't agree with them.

> However, what is really irritating me is that, not only am I having to
> divert time from more important tasks [...]

I'm terribly sorry that you can't just get your own way by insisting
really hard.  I'm sure that would save you time, but I doubt it would
be good for project governance.

Thanks for your attention.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH for-4.10 1/2] tools/libxc: Fix precopy_policy() to not pass a structure by value

[Andrew Cooper]

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 not necessary to resolve our disagreement over
> whether your proposed the commit message wording is accurate, or
> whether it is a calumny.  I'm sure we can find some way to write the
> commit message that would avoid the claim I want you to avoid.
>
> How about:
>
>   This was originally passed as a parameter to avoid having to (in the
>   future) teach libxl_save_msgs_gen.pl to copy (by value) a struct
>   which is referred to by a pointer parameter to a hook function.
>
>   However, it is preferable for that parameter to be a pointer because
>   ...

I shall see about adjusting the wording.  However, I wish to make it
clear that patch is fixing a technical error (even if only
documentational in nature).

I will see if I can formulate something in the middle.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel