Switch if_lagg(4) to use counters from underlying interfaces instead of per-packet accounting.

[melifaro]

Summary

While counting packets using per-cpu counters might not introduce
any signifficant overhead at current rates, we do not need to
do such accounting at all.

lagg in general is pure control-plane interface, its action on
receive should be just to change packet src if pointer.
Its action on transmit should be just selecting output interface
based on flowid.
It should not generate any errors on its own.

In fact, RX lagg path can be skipped by setting correct ifp inside
NIC driver. TX path should be handled by generic multipath L2 nexthops
inside routing code.

This is first step for implementing this scenario.
One side effect is that we're now collecting all counters (including errors)
from underlying interfaces. Generally most networking HW vendors implement
this behavior for their equipment and this is really the reasonable thing to do.

[asomers]

sys/net/if_lagg.c

1060

This divide operation will be slow. But I bet that it will be done at compile time if you move it into the switch...case statement.

[melifaro]

Use array of unnamed counters instead of huge case().

[melifaro]

Do not use explicit typecast while performit initial portcounters read.

[asomers]

An oddity of Phabricator is that when you update a review, you must upload all files that were changed anywhere in that review. Otherwise, it will assume that you want to revert those files. For example, you modified if_lagg.h when you created the review, but you did not reupload it when you amended the review. That's why if_lagg.h doesn't show up now, and the review history looks wrong. You need to reupload all the files to make the overall review diff correct.

[melifaro]

Add if_lagg.h to previous patch.

[asomers]

Except for a few minor issues that I raised inline, it looks fine to me. However, I'm going on vacation beginning tomorrow, so I won't be available to review any future changes. I'll go ahead and accept the revision now.

sys/net/if_lagg.c

818

It would be more self-documenting to use IFCOUNTER_LAST instead of IFCOUNTER_NOPROTO. Also, if it were me I would've used to lp->port_counters.val as an array. I find it more readable, and it's easier for static analysis tools as well. But that's just my preference.

for (i=IFCOUNTER_IPACKETS, i <= IFCOUNTER_LAST; i++)
    lp->port_counters.val[i-1] = ifp->if_get_counter(ifp, i);

1013

I don't think it's necessary to use an off_t to index an array. And I don't think it's appropriate when the array elements are of more than 1 byte in size. ifnet_counter is probably the best type to use here. Also, I don't like the variable name "off". It made sense for the first version of your review when it was an offset in bytes, but now it's an array index.

1041

Head's up: this will give you a merge conflict due to r271752, which renamed if_get_counter_compat to if_get_counter_default.

1049

This line changes the meaning of vsum. Now it's a total, not a difference. Changing the meanings of variables is a pet peeve of mine. I would prefer to use a different variable for the total.

sys/net/if_lagg.h

241

Unlike proto_counters, port_counters is a sum, not a copy

[melifaro]

Use simple approach for accouting suggested by glebius.

[glebius]

sys/net/if_lagg.c

1015–1016

These two lines are not needed. We should not handle quering anything outside the enum.

sys/net/if_var.h

112 ↗

(On Diff #1664)

I'd suggest to set IFCOUNTER_IPACKETS to 0, and then put IFCOUNTER_LAST inside the enum clause, as the last member. May be name IFCOUNTER_SIZE would be better. Or IFCOUNTERS.

[melifaro]

Committed as r272211.