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