Page MenuHomeFreeBSD

Remove per-ifa outgoing packet accounting from ip[6]_output.
Needs RevisionPublic

Authored by melifaro on Jan 5 2016, 6:51 AM.

Details

Reviewers
bz
Group Reviewers
network
Summary

Per-ifa packet accounting is what is seen in 'netstat -i' output for IPv4/IPv6 addresses.

The main reason besides this change is that new routing API does not return individual route entries anymore (so you need to perform additional ia lookup/ref for accurate IPv4 packet accounting).
However, there are other reasons:

  1. IPv4 accouting is not always correct (for example, locally-originated TCP traffic w/ source address bound to IPv4 alias would account for 'primary' IPv4 address).
  2. In IPv6 due to much more complicated SAS already performs addtional ia lookup for non-frag cases which is too costy (lookup + atomic ref/unref) and uses rt_ifa for fragments (which can easily be LL address (e.g. wrong)).

On the other side, precise accounting could easily be implemented using simple (ipfw,pf) set of rules.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 1959
Build 1966: arc lint + arc unit

Event Timeline

melifaro updated this revision to Diff 11938.Jan 5 2016, 6:51 AM
melifaro retitled this revision from to Remove per-ifa outgoing packet accounting from ip[6]_output..
melifaro updated this object.
melifaro updated this object.Jan 5 2016, 6:58 AM
melifaro added a reviewer: network.
bz requested changes to this revision.Jan 5 2016, 10:32 AM
bz added a reviewer: bz.
bz added a subscriber: bz.

This is a straight reject to the idea from my view. Sorry.
People had been asking for this for IPv4 and I did the patch but never committed it as the penalty was noticeable. We should not lose these features in favour of simplicity but make them perform well when designing things. Having had per-address counters has been very valuable in the last years for IPv6 to debug and account various things. And sorry, using a firewall is not a solution.

This revision now requires changes to proceed.Jan 5 2016, 10:32 AM
In D4794#101759, @bz wrote:

This is a straight reject to the idea from my view. Sorry.
People had been asking for this for IPv4 and I did the patch but never committed it as the penalty was noticeable. We should not lose these features in favour of simplicity but make them perform well when designing things. Having had per-address counters has been very valuable in the last years for IPv6 to debug and account various things. And sorry, using a firewall is not a solution.

I thought it won't be easy but I had to start with something :)
Okay. So for IPv6 situation is not that complicated:
function like inc_ia6_countrers(ifp, addr, opackets, obytes) which internally finds appropriate ifa under ifaddr lock and increments pcpu counters under that lock, w/o the need to do heavy refcounting. It would both improve the performance and increase accuracy.

Probably the same can be done for IPv4, but here I'd like to hear more feedback from other people about what direction should we go:
do precise accounting (with some performance penalty) or leave it to user (either by some sysctl, or not doing in in-kernel, or..)

melifaro updated this revision to Diff 11948.Jan 5 2016, 1:10 PM
melifaro edited edge metadata.

Update IPv6 part (add precise accounting using newly-added in6_accountoifa()) per bz@ comments.

In D4794#101759, @bz wrote:

People had been asking for this for IPv4 and I did the patch but never committed it as the penalty was noticeable. We should not lose these features in favour of simplicity but make them perform well when designing things. Having had per-address counters has been very valuable in the last years for IPv6 to debug and account various things.

I've updated the patch.
IPv6 accounting for most common case (non-fragmented packets) should be slightly better (no ifa ref/unref cost). It is still costy, however, due to IF_ADDR_RLOCK() which is rwlock.
Sending frags is also accounted the same way which may degrade performance for that path.

Does these changes look better to you?

glebius added a subscriber: glebius.Apr 3 2019, 8:18 PM

After I spent sometime around this code, I support melifaro@'s suggestion.

This revision now requires changes to proceed.Apr 3 2019, 8:18 PM
bz added a comment.Apr 3 2019, 9:16 PM

After I spent sometime around this code, I support melifaro@'s suggestion.

Can you be more precise in what you mean by that? I am not sure the extra costs are still that big with epoch in place. I think for me the description of the change as it stands now might be misleading?