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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel