Discussion:
patch make struct protosw pr_input non-variadic
(too old to reply)
Tyler Retzlaff
2016-05-13 19:56:11 UTC
Permalink
Hi,

The attached patch for review (originally produced by riastradh@)
that provides the structural changes needed to make
protosw::pr_input protocol-specific and ditch its variadic
prototype pr_input(struct mbuf *, ...).

Similar work has already been done & discussed.

https://mail-index.netbsd.org/tech-net/2016/01/16/msg005484.html

Once the structural changes are in I'll generate a second patch
adapting the variadic implementations of pr_input to functions with
apropriate prototypes.

This patch:
* moves pr_input out of struct protosw and places it into <proto>sw
specitic structure.
* converts domain::dom_protosw to an array of pointers

comments? objections? cleanups?

Thanks
Christos Zoulas
2016-05-13 23:14:13 UTC
Permalink
-=-=-=-=-=-
Hi,
that provides the structural changes needed to make
protosw::pr_input protocol-specific and ditch its variadic
prototype pr_input(struct mbuf *, ...).
Similar work has already been done & discussed.
https://mail-index.netbsd.org/tech-net/2016/01/16/msg005484.html
Once the structural changes are in I'll generate a second patch
adapting the variadic implementations of pr_input to functions with
apropriate prototypes.
* moves pr_input out of struct protosw and places it into <proto>sw
specitic structure.
* converts domain::dom_protosw to an array of pointers
Why? What's the point of the extra indirection? You could just stick
and & in front of the array element assignment.

christos



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Tyler Retzlaff
2016-05-14 01:07:36 UTC
Permalink
Post by Christos Zoulas
-=-=-=-=-=-
* moves pr_input out of struct protosw and places it into <proto>sw
specitic structure.
* converts domain::dom_protosw to an array of pointers
Why? What's the point of the extra indirection? You could just stick
and & in front of the array element assignment.
I admit I didn't evaluate why this change was made. It was
part of the original patch I received.

Upon examination it looks as if the rationale is to compensate for the
const of the declaration in domain::dom_protosw.

for protocols that implement <proto>pr_input domain::dom_protosw is
initialized via <proto>_dom_init() by looping through and assigning
the the <proto>sw members (introduced in the patch).

I did discuss the merits of initializing using a loop vs maintaining
separate arrays of structures (one for <proto>pr_protosw and one for
<proto>pr_input) but two arrays seemed less desirable because keeping
the elements in sync would likely be error prone.

I'm open to alternatives you may suggest. One thought might be to just
provide a suitable macro and ditching the loop.

Let me know I'll go re-work it.

Thanks
Post by Christos Zoulas
christos
--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Christos Zoulas
2016-05-14 01:13:33 UTC
Permalink
On May 13, 9:07pm, ***@netbsd.org (Tyler Retzlaff) wrote:
-- Subject: Re: patch make struct protosw pr_input non-variadic

| I'm open to alternatives you may suggest. One thought might be to just
| provide a suitable macro and ditching the loop.

This was my first thought; we provide iterator macros for everything, why
open-code this one?

christos

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Taylor R Campbell
2016-05-14 16:47:28 UTC
Permalink
Date: Fri, 13 May 2016 23:14:13 +0000 (UTC)
Post by Tyler Retzlaff
[...]
* converts domain::dom_protosw to an array of pointers
Why? What's the point of the extra indirection? You could just stick
and & in front of the array element assignment.

Currently every struct <protospecific>protosw has to have exactly the
same size and layout so that the type-punning works -- and nothing
checks this because it is all done with explicit casts that suppress
warnings. If we ever changed, e.g., struct ip6protosw to add another
method, then this would break, without help from the compiler to
detect the breakage.

We already abuse this type-punning to give IPv4 pr_input methods a
different true prototype from IPv6 pr_input methods, but again there's
no static checking of this, so it is easy to make a mistake. The next
step after this change will actually give pr_input real prototypes by
moving it out of struct protosw and into struct ipprotosw, struct
ip6protosw, &c.

Changing dom_protosw to be an array of pointers to struct protosw
instead of an array of struct protosw grants us flexibility in using
struct <protospecific>protosw structure and still get real type
checking for everything -- we simply fill dom_protosw with pointers to
an embedded struct protosw inside struct <protospecific>protosw.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Taylor R Campbell
2016-05-14 19:11:38 UTC
Permalink
Date: Sat, 14 May 2016 16:47:28 +0000
From: Taylor R Campbell <campbell+netbsd-tech-***@mumble.net>

Date: Fri, 13 May 2016 23:14:13 +0000 (UTC)
Post by Tyler Retzlaff
[...]
* converts domain::dom_protosw to an array of pointers
Why? What's the point of the extra indirection? You could just stick
and & in front of the array element assignment.

[...]
Changing dom_protosw to be an array of pointers to struct protosw
instead of an array of struct protosw grants us flexibility in using
struct <protospecific>protosw structure and still get real type
checking for everything -- we simply fill dom_protosw with pointers to
an embedded struct protosw inside struct <protospecific>protosw.

I should add that I believe this indirection does not affect any hot
paths. For example, in the packet-processing path, ip_input still
uses the direct array, which dom_protosw is an array of pointers into;
the actual machine code in ip_input shouldn't change at all.

As far as I could tell, dom_protosw is used only in cold paths, such
as in ip_init, in domain_attach, in route-changing ctlinput packet
processing, &c., and only for linear scans through the whole array
anyway.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Christos Zoulas
2016-05-14 19:29:36 UTC
Permalink
On May 14, 7:11pm, campbell+netbsd-tech-***@mumble.net (Taylor R Campbell) wrote:
-- Subject: Re: patch make struct protosw pr_input non-variadic

| Date: Sat, 14 May 2016 16:47:28 +0000
| From: Taylor R Campbell <campbell+netbsd-tech-***@mumble.net>
|
| Date: Fri, 13 May 2016 23:14:13 +0000 (UTC)
| From: ***@astron.com (Christos Zoulas)
|
| In article <***@pris>,
| Tyler Retzlaff <***@netbsd.org> wrote:
| >This patch:
| >[...]
| >* converts domain::dom_protosw to an array of pointers
|
| Why? What's the point of the extra indirection? You could just stick
| and & in front of the array element assignment.
|
| [...]
| Changing dom_protosw to be an array of pointers to struct protosw
| instead of an array of struct protosw grants us flexibility in using
| struct <protospecific>protosw structure and still get real type
| checking for everything -- we simply fill dom_protosw with pointers to
| an embedded struct protosw inside struct <protospecific>protosw.
|
| I should add that I believe this indirection does not affect any hot
| paths. For example, in the packet-processing path, ip_input still
| uses the direct array, which dom_protosw is an array of pointers into;
| the actual machine code in ip_input shouldn't change at all.
|
| As far as I could tell, dom_protosw is used only in cold paths, such
| as in ip_init, in domain_attach, in route-changing ctlinput packet
| processing, &c., and only for linear scans through the whole array
| anyway.

But you are casting structs now... How is that safe?
You might as well make it an array for void *'s...

christos


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Tyler Retzlaff
2016-05-14 21:28:37 UTC
Permalink
Post by Christos Zoulas
-- Subject: Re: patch make struct protosw pr_input non-variadic
| Date: Sat, 14 May 2016 16:47:28 +0000
|
| Date: Fri, 13 May 2016 23:14:13 +0000 (UTC)
|
| >[...]
| >* converts domain::dom_protosw to an array of pointers
|
| Why? What's the point of the extra indirection? You could just stick
| and & in front of the array element assignment.
|
| [...]
| Changing dom_protosw to be an array of pointers to struct protosw
| instead of an array of struct protosw grants us flexibility in using
| struct <protospecific>protosw structure and still get real type
| checking for everything -- we simply fill dom_protosw with pointers to
| an embedded struct protosw inside struct <protospecific>protosw.
|
| I should add that I believe this indirection does not affect any hot
| paths. For example, in the packet-processing path, ip_input still
| uses the direct array, which dom_protosw is an array of pointers into;
| the actual machine code in ip_input shouldn't change at all.
|
| As far as I could tell, dom_protosw is used only in cold paths, such
| as in ip_init, in domain_attach, in route-changing ctlinput packet
| processing, &c., and only for linear scans through the whole array
| anyway.
But you are casting structs now... How is that safe?
You might as well make it an array for void *'s...
The only casts I see in the patch are those producing the address of
struct protosw that are the product of a designated initialization? Or
am I missing something?


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Christos Zoulas
2016-05-14 22:22:08 UTC
Permalink
On May 14, 5:28pm, ***@netbsd.org (Tyler Retzlaff) wrote:
-- Subject: Re: patch make struct protosw pr_input non-variadic

| The only casts I see in the patch are those producing the address of
| struct protosw that are the product of a designated initialization? Or
| am I missing something?

Yes, I guess the only way to avoid those is to have static variables for
them?

christos

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Taylor R Campbell
2016-05-15 14:54:56 UTC
Permalink
Date: Sat, 14 May 2016 18:22:08 -0400
From: ***@zoulas.com (Christos Zoulas)

On May 14, 5:28pm, ***@netbsd.org (Tyler Retzlaff) wrote:
-- Subject: Re: patch make struct protosw pr_input non-variadic

| The only casts I see in the patch are those producing the address of
| struct protosw that are the product of a designated initialization? Or
| am I missing something?

Yes, I guess the only way to avoid those is to have static variables for
them?

These aren't casts, per se -- they're what C99 calls compound literals
(Sec. 6.5.2.5, p. 75). They do not indicate any sort of type punning:
it's just a way to create an anonymous structure object and get its
address as a constant initializer. I did this to make the patch
easier to review and to avoid a proliferation of global constants.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Christos Zoulas
2016-05-15 17:33:46 UTC
Permalink
Post by Taylor R Campbell
These aren't casts, per se -- they're what C99 calls compound literals
it's just a way to create an anonymous structure object and get its
address as a constant initializer. I did this to make the patch
easier to review and to avoid a proliferation of global constants.
Looks like I'll need to fix lint again :-)

christos

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Tyler Retzlaff
2016-05-14 23:06:47 UTC
Permalink
Post by Christos Zoulas
-- Subject: Re: patch make struct protosw pr_input non-variadic
| The only casts I see in the patch are those producing the address of
| struct protosw that are the product of a designated initialization? Or
| am I missing something?
Yes, I guess the only way to avoid those is to have static variables for
them?
That would be my expectation if it what was desired. It seems harder to
maintain though.

(1)

const struct protosw unixsw[] = {
{ /* 0 */
... initialize ...
},
{ /* 1 */
... initialize ...
},
};

const struct protosw *unixswp[] = {
&unixsw[0],
&unixsw[1],
};

struct domain unixdomain = {
.dom_protosw = unixswp,
.dom_nprotosw = __arraycount(unixswp)
... other members ...
};

Would be awful.

The other possibility is to populate at domain initialization.

(2)

void
un_dom_init()
{
for (i = 0; i < __arraycount(unixsw); i++) {
unixdomain.dom_protosw[i] = &unixsw[i];
}
}

Neither of these seems better than just placing the cast at the point
of designated initialization. I guess you would agree?
Post by Christos Zoulas
christos
--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Taylor R Campbell
2016-05-15 14:37:40 UTC
Permalink
Date: Sat, 14 May 2016 15:29:36 -0400
From: ***@zoulas.com (Christos Zoulas)

But you are casting structs now... How is that safe?
You might as well make it an array for void *'s...

Before, with casts for type puns and strict aliasing violation:

struct protosw {
int pr_type;
int (*pr_input)(struct mbuf *, ...);
};

struct ip6protosw {
int pr_type;
int (*pr_input)(struct mbuf **, int *, int);
};

struct ip6protosw inet6sw[] = {
{
.pr_type = SOCK_DGRAM,
.pr_input = udp6_input,
},
...
};

struct domain inet6domain = {
/*
* Type pun in violation of strict aliasing. inet6sw is *not*
* an array of struct protosw.
*/
---> .dom_protosw = (struct protosw *)inet6sw,
...
}

After, with neither casts nor void * nor strict aliasing violations:

struct protosw {
int pr_type;
};

struct ip6protosw {
struct protosw ip6pr_protosw;
int (*ip6pr_input)(struct mbuf **, int *, int);
};

struct ip6protosw inet6sw[] = {
{
.ip6pr_protosw = { .pr_type = SOCK_DGRAM },
.ip6pr_input = udp6_input,
},
...
};

struct protosw *inet6protosw[] = {
/* Effectively, this initializer, but written at run-time. */
&inet6sw[0].ip6pr_protosw,
...
};

struct domain inet6domain = {
/* No type pun: LHS and RHS are both struct protosw **. */
---> .dom_protosw = inet6protosw,
...
};

(But ip6_input still goes through inet6sw directly -- so there is no
change to the protocol-dispatching code in the packet-processing
path.)

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Thor Lancelot Simon
2016-05-15 14:42:24 UTC
Permalink
Post by Christos Zoulas
Post by Tyler Retzlaff
* moves pr_input out of struct protosw and places it into <proto>sw
specitic structure.
* converts domain::dom_protosw to an array of pointers
Why? What's the point of the extra indirection? You could just stick
and & in front of the array element assignment.
I want to reemphasize the potentially negative performance impact of
this kind of change. It turns the branch into a computed branch in a
way that will defeat branch prediction on many CPUs and thus cause
stalls. Don't call through pointers if you don't have to.
--
Thor Lancelot Simon ***@panix.com

"We cannot usually in social life pursue a single value or a single moral
aim, untroubled by the need to compromise with others." - H.L.A. Hart

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Taylor R Campbell
2016-05-15 15:10:21 UTC
Permalink
Date: Sun, 15 May 2016 10:42:24 -0400
Post by Christos Zoulas
Post by Tyler Retzlaff
* moves pr_input out of struct protosw and places it into <proto>sw
specitic structure.
* converts domain::dom_protosw to an array of pointers
Why? What's the point of the extra indirection? You could just stick
and & in front of the array element assignment.
I want to reemphasize the potentially negative performance impact of
this kind of change. It turns the branch into a computed branch in a
way that will defeat branch prediction on many CPUs and thus cause
stalls. Don't call through pointers if you don't have to.

It may well have a negative performance impact to change

(*inetsw[ip_proto[nh]].pr_input)(m, off, nh)

in ip_input to

(*inetsw[ip_proto[nh]]->pr_input)(m, off, nh)

That's why this patch does *not* do that. The actual change to
ip_input is only

(*inetsw[ip_proto[nh]].ippr_input)(m, off, nh)

where we can now give struct ipprotosw::ippr_input a true prototype to
distinguish it from, e.g., the IPv6 code which passes &m and &off
instead of m and off.

The only place the extra indirection is used is in paths that are cold
anyway, where the caller is iterating over all protocols in a domain,
mostly in uipc_domain.c, during kernel initialization, socket
creation, routing table updates, &c.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Tyler Retzlaff
2016-05-15 23:50:19 UTC
Permalink
So I lost track.

Have we addressed all the concerns that have been raised?

* We're removing type-punning.
* We aren't adding additional levels of indirection in the hot path.
* We aren't badly casting structs.

Do we need more discussion or shall we proceed?

Thanks for the feedback.
Post by Taylor R Campbell
Date: Sun, 15 May 2016 10:42:24 -0400
Post by Christos Zoulas
Post by Tyler Retzlaff
* moves pr_input out of struct protosw and places it into <proto>sw
specitic structure.
* converts domain::dom_protosw to an array of pointers
Why? What's the point of the extra indirection? You could just stick
and & in front of the array element assignment.
I want to reemphasize the potentially negative performance impact of
this kind of change. It turns the branch into a computed branch in a
way that will defeat branch prediction on many CPUs and thus cause
stalls. Don't call through pointers if you don't have to.
It may well have a negative performance impact to change
(*inetsw[ip_proto[nh]].pr_input)(m, off, nh)
in ip_input to
(*inetsw[ip_proto[nh]]->pr_input)(m, off, nh)
That's why this patch does *not* do that. The actual change to
ip_input is only
(*inetsw[ip_proto[nh]].ippr_input)(m, off, nh)
where we can now give struct ipprotosw::ippr_input a true prototype to
distinguish it from, e.g., the IPv6 code which passes &m and &off
instead of m and off.
The only place the extra indirection is used is in paths that are cold
anyway, where the caller is iterating over all protocols in a domain,
mostly in uipc_domain.c, during kernel initialization, socket
creation, routing table updates, &c.
--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Robert Elz
2016-05-16 01:25:23 UTC
Permalink
Date: Sun, 15 May 2016 19:50:19 -0400
From: Tyler Retzlaff <***@netbsd.org>
Message-ID: <de1ac713-5693-2aca-2c7a-***@netbsd.org>

Just a comment off the side ...

| Have we addressed all the concerns that have been raised?

And not about that (from what I can see the change looks OK, but I have
not looked in detail at the code) but...

| Do we need more discussion or shall we proceed?

Back in the good old days - I mean 4BSD, and certainly 4.1 .. 4.3 BSD
(I wasn't involved as much with 4.4, so can't really say for that one
way or the other) no-one would have even dared suggest a change anything
like this without backing it up with performance measurements, gprof
data showing that (at least) it was not worse then before, and before and
after throughput measurements.

We seem to have lost that here, and comments are made (both side) based upon
speculation about what seems likely might hurt, improve, or not affect
performamce, rather than by any actual testing.

It is different for an obvious bug fix (code that did not work before has
0 performance after all, anything is better) but when the change is intended
in some way to improve things, it ought be shown that it does. The
"cleanliness" improvement seems obvious here, but if there were to be a
performance downside (and I am not saying that there is, I have not measured
it either) then there would need to be a decision on whether or not the code
improvement is worth the cost (of course, what sometimes happens, is that
cleaner code is also faster, and everyone is happy.)

kre


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Tyler Retzlaff
2016-05-16 02:23:46 UTC
Permalink
Post by Robert Elz
| Do we need more discussion or shall we proceed?
Back in the good old days - I mean 4BSD, and certainly 4.1 .. 4.3 BSD
(I wasn't involved as much with 4.4, so can't really say for that one
way or the other) no-one would have even dared suggest a change anything
like this without backing it up with performance measurements, gprof
data showing that (at least) it was not worse then before, and before and
after throughput measurements.
I can't actually understand how we got on to the topic of performance.
Post by Robert Elz
We seem to have lost that here, and comments are made (both side) based upon
speculation about what seems likely might hurt, improve, or not affect
performamce, rather than by any actual testing.
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.

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

This patch is a re-structure it just moved a pointer to the input function
out of one structure and into another. The way that pointer is used has
not been changed.
Post by Robert Elz
It is different for an obvious bug fix (code that did not work before has
0 performance after all, anything is better) but when the change is intended
in some way to improve things, it ought be shown that it does. The
"cleanliness" improvement seems obvious here, but if there were to be a
performance downside (and I am not saying that there is, I have not measured
it either) then there would need to be a decision on whether or not the code
improvement is worth the cost (of course, what sometimes happens, is that
cleaner code is also faster, and everyone is happy.)
I'm encouraged that you observe it as a cleanliness improvement. I would
hope that the patch (that will follow this) will also be considered as
such.

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

http://mail-index.netbsd.org/tech-net/2016/05/14/msg005869.html

It may be of interest that a review of the callers of pr_input do not
always pass 2 variadic arguments as most input functions seem to unpack.
A situation that would not be possible if this (and subsequent change)
is made. Probably the callers "know" which implementation of input
function they will get but not brilliant in terms of an abstraction.


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Robert Elz
2016-05-16 03:03:18 UTC
Permalink
Date: Sun, 15 May 2016 22:23:46 -0400
From: Tyler Retzlaff <tech-***@netbsd.org>
Message-ID: <***@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


--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Taylor R Campbell
2016-05-16 04:05:36 UTC
Permalink
Date: Mon, 16 May 2016 10:03:18 +0700
From: Robert Elz <***@munnari.OZ.AU>

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.

IPv4 packets go through inetsw; IPv6 through inet6sw, a different
table altogether. The only change is that the structure members in
inetsw are reordered -- though I believe the only one that is used in
the packet-processing path is pr_input.

It's true that the reordering of the structure members may conceivably
affect performance by changing which cache line is used. But the work
that Ozaki-san and Nakahara-san are doing to parallelize the network
stack have much greater impact on performance (measured by Ozaki-san),
and some of their work has been hindered by the lack of the
compiler-assisted refactoring that true prototypes enable.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Taylor R Campbell
2017-01-07 03:52:27 UTC
Permalink
Date: Mon, 16 May 2016 04:05:36 +0000
From: Taylor R Campbell <campbell+netbsd-tech-***@mumble.net>

It's true that the reordering of the structure members may conceivably
affect performance by changing which cache line is used. But the work
that Ozaki-san and Nakahara-san are doing to parallelize the network
stack have much greater impact on performance (measured by Ozaki-san),
and some of their work has been hindered by the lack of the
compiler-assisted refactoring that true prototypes enable.

To revive this thread from last year, since I'd still like to see this
patch committed...

I asked Ozaki-san to measure the performance impact of the patch. It
is, of course, impossible to characterize the full distribution of
performance changes over all network traffic patterns. But at least
in the test setups that Ozaki-san has tried, there seems to be no
major performance impact.

Specifically, in the setup presented at EuroBSDcon[1], Ozaki-san ran
two performance evaluations, each one with and without the patch.
Quoting his explanation to me:

`Anyway I evaluated again three times for each kernel with
(a) simple throughput benchmark and (b) RFC 2544 benchmark.

(a) just measures a throughput with 200 Mbps offered traffic.
(b) tries to find the best performance while no packet drops happens
by changing offered traffic with bisecting. So its results should
be more accurate and stable than (a).

The results are:
(a)
w/ the patch: 152.0, 153.4, 153.7
w/o the patch: 152.7, 151.7, 153.5

(b)
w/ the patch: 162.7, 164.2, 162.7
w/o the patch: 162.9, 162.0, 162.7'

What do we conclude from this? It's reasonable to assume that all
these distributions are roughly normally distributed (there is a hard
lower bound to the possible run time, but I expect there is enough
white noise all the time that a normal approximates it well enough).

We might perform a traditional frequentist hypothesis test: for each
evaluation, apply a test to reject the null hypothesis, that the two
distributions -- with patch, without patch -- are equal, in favour of
the alternative hypothesis that the distributions are different,
namely a Welch's t-test, with false rejection rate alpha = 5%.
Computed by scipy and rounded by my wetware to two decimal places,

(a) t = .54, p = .62 > .05 => fail to reject null hypothesis
(b) t = 1.2, p = .32 > .05 => fail to reject null hypothesis

Power analysis: For this sample size, there would be an 80% chance of
rejecting the null hypothesis if the means were different by ~3 sigma,
50% if ~2 sigma, &c. This is a fairly weak test -- if we had a sample
size of thirty instead of three, there would be an 80% chance of
rejecting if the means differed by only ~.7 sigma. Nevertheless, this
suggests there is not a major performance impact.

I didn't preregister this experiment or hypothesis test, but this is
exactly what any undergrad in a statistics class would be inclined to
do, if they didn't reach for a Student's t-test which assumes equal
variance, and which gives essentially the same results anyway.

We might do a Bayesian analysis with priors on the means and standard
deviations of these distribution, and then ask for the posterior
predictive distribution on, say, the squared distances between the
means. That's a little more work than I care to do at the moment.

In contrast, Ozaki-san has measured much bigger performance
differences, ~8 sigma, with psref -- in serial, but of course psref
enables the network stack to scale in parallel. This is why many uses
of psref are still conditional on NET_MPSAFE. However, the world is
scaling in parallel, not in serial. And psref is also only a
provisional tool until we can restructure the hot paths to avoid
sleeping altogether and replace psref by pserialize which does no
linked list operations.

So. OK to commit?


[1] Ryota Ozaki and Kengo Nakahara, `Toward MP-safe Networking in
NetBSD', EuroBSDcon 2016, Stockholm.
https://www.netbsd.org/gallery/presentations/ozaki-r/2016_EuroBSDcon/EuroBSDcon2016-ozaki-nakahara.pdf

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Joerg Sonnenberger
2017-01-07 09:03:21 UTC
Permalink
Post by Taylor R Campbell
So. OK to commit?
Yes.

Joerg

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Ryota Ozaki
2017-01-11 08:37:20 UTC
Permalink
Post by Taylor R Campbell
So. OK to commit?
Yes.
ok from me too. And I hope the patch is committed soon because
we would work on pr_input in the near future :)

ozaki-r

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Matt Thomas
2017-01-11 20:55:26 UTC
Permalink
Post by Taylor R Campbell
So. OK to commit?
Yes.
For me it's a no. I find it ugly. I'm going to give an alternate
approach while is almost as ugly but a little simpler.

Why not replace void (*pr_input)(struct mbuf *, ...); in protosw
with a union? The first member of the union can be that and then
include the (mbuf *, int, int) as a secondary member.

Then the netinet/netinet6 can just call the alternate member and
completely avoid variadic style. initializers can use the addition
member as well.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Taylor R Campbell
2017-01-12 15:16:16 UTC
Permalink
Date: Wed, 11 Jan 2017 12:55:26 -0800
From: Matt Thomas <***@3am-software.com>

Why not replace void (*pr_input)(struct mbuf *, ...); in protosw
with a union? The first member of the union can be that and then
include the (mbuf *, int, int) as a secondary member.

We did that for struct encapsw. It doesn't provide the same type
checking, however. It would fail to detect the specific mistake that
I saw Nakahara-san make during the encap work, of initializing one
union member and then using another.

The change that rtr and I proposed would detect exactly that by
causing inet6sw, by which the various xyz6_input routines are called,
to have a different structure type altogether.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Matt Thomas
2017-01-12 17:45:52 UTC
Permalink
Post by Taylor R Campbell
Date: Wed, 11 Jan 2017 12:55:26 -0800
Why not replace void (*pr_input)(struct mbuf *, ...); in protosw
with a union? The first member of the union can be that and then
include the (mbuf *, int, int) as a secondary member.
We did that for struct encapsw. It doesn't provide the same type
checking, however. It would fail to detect the specific mistake that
I saw Nakahara-san make during the encap work, of initializing one
union member and then using another.
The change that rtr and I proposed would detect exactly that by
causing inet6sw, by which the various xyz6_input routines are called,
to have a different structure type altogether.
I don't think the additional ugliness is worth it. Why not just

#define pr_input do_not_use

to prevent it's use?
--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Taylor R Campbell
2017-01-12 18:14:18 UTC
Permalink
Date: Thu, 12 Jan 2017 09:45:52 -0800
From: Matt Thomas <***@3am-software.com>

I don't think the additional ugliness is worth it. Why not just

#define pr_input do_not_use

to prevent it's use?

I don't understand how that helps to solve the problem of giving
compiler-checked protocol-dependent types to the input routines.

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Matt Thomas
2017-01-12 20:28:34 UTC
Permalink
Post by Taylor R Campbell
Date: Thu, 12 Jan 2017 09:45:52 -0800
I don't think the additional ugliness is worth it. Why not just
#define pr_input do_not_use
to prevent it's use?
I don't understand how that helps to solve the problem of giving
compiler-checked protocol-dependent types to the input routines.
because they only be called by netinet/netinet6 files. If those sources use pr_input they will get an error. If they use pr_ipinput (or whatever the variant is called) they won't get an error.
--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Christos Zoulas
2017-01-13 04:29:51 UTC
Permalink
Post by Taylor R Campbell
Date: Thu, 12 Jan 2017 09:45:52 -0800
I don't think the additional ugliness is worth it. Why not just
#define pr_input do_not_use
to prevent it's use?
I don't understand how that helps to solve the problem of giving
compiler-checked protocol-dependent types to the input routines.
Because we only care about netinet{,6} and doing that in the
inet headers can just prevent its use there while retaining the varyadic
nature of the input function in the rest of the tree?

christos



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Christos Zoulas
2017-01-12 02:17:56 UTC
Permalink
Post by Matt Thomas
Post by Taylor R Campbell
So. OK to commit?
Yes.
For me it's a no. I find it ugly. I'm going to give an alternate
approach while is almost as ugly but a little simpler.
Why not replace void (*pr_input)(struct mbuf *, ...); in protosw
with a union? The first member of the union can be that and then
include the (mbuf *, int, int) as a secondary member.
Then the netinet/netinet6 can just call the alternate member and
completely avoid variadic style. initializers can use the addition
member as well.
I like that... Can't it be done with a transparent union and c99 initializers?

christos



--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de
Loading...