Re: patch make struct protosw pr_input non-variadic

[Robert Elz <kre%munnari.OZ.AU@localhost>]

    Date:        Sun, 15 May 2016 22:23:46 -0400
    From:        Tyler Retzlaff <tech-net%netbsd.org@localhost>
    Message-ID:  <20160516022346.GA21161@pris>

  | I can't actually understand how we got on to the topic of performance.

There are three basic types of changes that can be made (to anything).

One is a bug fix - either the code as is dumps core, returns the wrong
answer, or ....  and needs to be fixed.   In that case, the performance
cost of the fix would only ever be an issue if there were two competing
fixes - other than in that case, the fix is needed.   (Then if performance
needs improving, that's a separate issue - another bug to fix perhaps.)

Second are no-impact changes, KNF, white space, comment cleanups, ...
The stuff where (when complex enough) people sometimes verify that a
compile produces the exact same code before and after.   Obviously those
do not affect (cannot affect) performance, either way, and do not need to
be measured.

And third are all the others.

  | I've yet to see anyone highlight a section of the patch that actually
  | represents a change to how protocol input has changed in a way that would
  | harm performance.

First, note that I did not say that there was, but ...

  |         SOFTNET_LOCK();
  | -       (*inetsw[ip_protox[nh]].pr_input)(m, off, nh);
  | +       (*inetsw[ip_protox[nh]].ippr_input)(m, off, nh);
  |         SOFTNET_UNLOCK();

that easily might, you're accessing a different field in the struct.
That is likely to be in a different cache line, perhaps evicting some
other data that was not being evicted before.

If a system is receiving alternating v4 & v6 packets (say), now, as I
understand the patch, the v4 will all be referencing one field, and the
v6 a different one, whereas before they were all accessing the same one.
That makes it more likely in the earlier code that the data would be
cached, and less likely that someone else's data would be being flushed.

I have no idea if that (if it does happen that way) makes any kind of
measurable difference or not.   My point was that nor does anyone else.
That's why performance measurements are so important.   The change above
might even improve cache performance for some reason or other.   Without
data, how can anyone know?

  | The benefit is not intended to be a performance improvement the
  | perceived benefits were highlighted by Taylor here.

Sure, understood, and even if it costs some performance, that does not
automatically mean it should be rejected.   Most changes cost something.

What matters then is whether the benefit is worth the cost.   The point
here is that no-one can (rationally) make that judgement without knowing
what the cost will be (unless you consider it so important that any cost
is worthwhile - and I doubt this one really falls into that category.)

kre