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