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

[Ian Jackson <ian.jackson@xxxxxxxxxxxxx>]

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