Commit Graph

44 Commits (d05dd41bc18780b5271cce26d8a22428606fd92d)

Author SHA1 Message Date
Brad Fitzpatrick 7c671b0220 .github/workflows: add gofmt (goimports) check
Change-Id: Iceb3182827b9c65f28f0351e0e254abe4a95e4de
Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
2 years ago
Brad Fitzpatrick 700badd8f8 util/deephash: move internal/deephash to util/deephash
No code changes. Just a minor package doc addition about lack of API
stability.
3 years ago
Josh Bleecher Snyder 7f095617f2 internal/deephash: 8 bits of output is not enough
Running hex.Encode(b, b) is a bad idea.
The first byte of input will overwrite the first two bytes of output.
Subsequent bytes have no impact on the output.

Not related to today's IPv6 bug, but...wh::ps.

This caused us to spuriously ignore some wireguard config updates.

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
3 years ago
Josh Bleecher Snyder c065cc6169 internal/deephash: remove remaining type special cases
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder 4b51fbf48c internal/deephash: increase scratch space size
e66d4e4c81 added AppendTo methods
to some key types. Their marshaled form is longer than 64 bytes.

name    old time/op    new time/op    delta
Hash-8    15.5µs ± 1%    14.8µs ± 1%   -4.17%  (p=0.000 n=9+9)

name    old alloc/op   new alloc/op   delta
Hash-8    1.18kB ± 0%    0.47kB ± 0%  -59.87%  (p=0.000 n=10+10)

name    old allocs/op  new allocs/op  delta
Hash-8      12.0 ± 0%       6.0 ± 0%  -50.00%  (p=0.000 n=10+10)

This is still a bit worse than explicitly handling the types,
but much nicer.

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder b340beff8e internal/deephash: reset scratch before appending to it
Oops. In practice this doesn't matter, but it's still wrong.

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder 15a7ff83de internal/deephash: remove netaddr special cases
All netaddr types that we are concerned with now implement AppendTo.
Use the AppendTo method if available, and remove all references to netaddr.

This is slower but cleaner, and more readily re-usable by others.

name              old time/op    new time/op    delta
Hash-8              12.6µs ± 0%    14.8µs ± 1%  +18.05%  (p=0.000 n=8+10)
HashMapAcyclic-8    21.4µs ± 1%    21.9µs ± 1%   +2.39%  (p=0.000 n=10+9)

name              old alloc/op   new alloc/op   delta
Hash-8                408B ± 0%      408B ± 0%     ~     (p=1.000 n=10+10)
HashMapAcyclic-8     1.00B ± 0%     1.00B ± 0%     ~     (all equal)

name              old allocs/op  new allocs/op  delta
Hash-8                6.00 ± 0%      6.00 ± 0%     ~     (all equal)
HashMapAcyclic-8      0.00           0.00          ~     (all equal)

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder 051d2f47e5 internal/deephash: re-use MapIter
name              old time/op    new time/op    delta
Hash-8              12.4µs ± 0%    12.4µs ± 0%    -0.33%  (p=0.002 n=10+9)
HashMapAcyclic-8    21.2µs ± 0%    21.3µs ± 0%    +0.45%  (p=0.000 n=8+8)

name              old alloc/op   new alloc/op   delta
Hash-8                793B ± 0%      408B ± 0%   -48.55%  (p=0.000 n=10+10)
HashMapAcyclic-8      128B ± 0%        0B       -100.00%  (p=0.000 n=10+10)

name              old allocs/op  new allocs/op  delta
Hash-8                9.00 ± 0%      6.00 ± 0%   -33.33%  (p=0.000 n=10+10)
HashMapAcyclic-8      1.00 ± 0%      0.00       -100.00%  (p=0.000 n=10+10)

Depends on https://github.com/golang/go/issues/46293.

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder c06ec45f09 internal/deephash: document MapIter shims
These exist so we can use the optimized MapIter APIs
while still working with released versions of Go.
They're pretty simple, but some docs won't hurt.

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder ce7a87e5e4 internal/deephash: use hash.BlockSize instead of a constant
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder 135b641332 internal/deephash: add re-usable scratch space
name    old time/op    new time/op    delta
Hash-8    13.9µs ± 0%    12.5µs ± 0%  -10.10%  (p=0.008 n=5+5)

name    old alloc/op   new alloc/op   delta
Hash-8      793B ± 0%      793B ± 0%     ~     (all equal)

name    old allocs/op  new allocs/op  delta
Hash-8      14.0 ± 0%      12.0 ± 0%  -14.29%  (p=0.008 n=5+5)

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder 988dfcabef internal/deephash: simplify API
Reduce to just a single external endpoint.
Convert from a variadic number of interfaces to a slice there.

name    old time/op    new time/op    delta
Hash-8    14.4µs ± 0%    14.0µs ± 1%   -3.08%  (p=0.000 n=9+9)

name    old alloc/op   new alloc/op   delta
Hash-8      873B ± 0%      793B ± 0%   -9.16%  (p=0.000 n=9+6)

name    old allocs/op  new allocs/op  delta
Hash-8      18.0 ± 0%      14.0 ± 0%  -22.22%  (p=0.000 n=10+10)

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder b371588ce6 internal/deephash: use netaddr AppendTo methods
Slightly slower, but lots less garbage.
We will recover the speed lost in a follow-up commit.

name    old time/op    new time/op    delta
Hash-8    13.5µs ± 1%    14.3µs ± 0%   +5.84%  (p=0.000 n=10+9)

name    old alloc/op   new alloc/op   delta
Hash-8    1.46kB ± 0%    0.87kB ± 0%  -40.10%  (p=0.000 n=7+10)

name    old allocs/op  new allocs/op  delta
Hash-8      43.0 ± 0%      18.0 ± 0%  -58.14%  (p=0.000 n=10+10)

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder 09afb8e35b internal/deephash: re-use map iteration values
This requires changes to the Go toolchain.
The changes are upstream at https://golang.org/cl/320929.
They haven't been pulled into our fork yet.

No need to allocate new iteration scratch values for every map.

name              old time/op    new time/op    delta
Hash-8              13.6µs ± 0%    13.5µs ± 0%   -1.01%  (p=0.008 n=5+5)
HashMapAcyclic-8    21.2µs ± 1%    21.1µs ± 2%     ~     (p=0.310 n=5+5)

name              old alloc/op   new alloc/op   delta
Hash-8              1.58kB ± 0%    1.46kB ± 0%   -7.60%  (p=0.008 n=5+5)
HashMapAcyclic-8      152B ± 0%      128B ± 0%  -15.79%  (p=0.008 n=5+5)

name              old allocs/op  new allocs/op  delta
Hash-8                49.0 ± 0%      43.0 ± 0%  -12.24%  (p=0.008 n=5+5)
HashMapAcyclic-8      4.00 ± 0%      2.00 ± 0%  -50.00%  (p=0.008 n=5+5)

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder a2d7a2aeb1 internal/deephash: use MapIter.Set{Key,Value}
To get the benefit of this optimization requires help from the Go toolchain.
The changes are upstream at https://golang.org/cl/320929,
and have been pulled into the Tailscale fork at
728ecc58fd.
It also requires building with the build tag tailscale_go.

name              old time/op    new time/op    delta
Hash-8              14.0µs ± 0%    13.6µs ± 0%   -2.88%  (p=0.008 n=5+5)
HashMapAcyclic-8    24.3µs ± 1%    21.2µs ± 1%  -12.47%  (p=0.008 n=5+5)

name              old alloc/op   new alloc/op   delta
Hash-8              2.16kB ± 0%    1.58kB ± 0%  -27.01%  (p=0.008 n=5+5)
HashMapAcyclic-8    2.53kB ± 0%    0.15kB ± 0%  -93.99%  (p=0.008 n=5+5)

name              old allocs/op  new allocs/op  delta
Hash-8                77.0 ± 0%      49.0 ± 0%  -36.36%  (p=0.008 n=5+5)
HashMapAcyclic-8       202 ± 0%         4 ± 0%  -98.02%  (p=0.008 n=5+5)

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>

setkey
4 years ago
Josh Bleecher Snyder 020e904f4e internal/deephash: add special handling for netaddr.IPPort
The acyclic map code interacts badly with netaddr.IPs.
One of the netaddr.IP fields is an *intern.Value,
and we use a few sentinel values.
Those sentinel values make many of the netaddr data structures appear cyclic.

One option would be to replace the cycle-detection code with
a Floyd-Warshall style algorithm. The downside is that this will take
longer to detect cycles, particularly if the cycle is long.

This problem is exacerbated by the fact that the acyclic cycle detection
code shares a single visited map for the entire data structure,
not just the subsection of the data structure localized to the map.
Unfortunately, the extra allocations and work (and code) to use per-map
visited maps make this option not viable.

Instead, continue to special-case netaddr data types.

name              old time/op    new time/op    delta
Hash-8              22.4µs ± 0%    14.0µs ± 0%  -37.59%  (p=0.008 n=5+5)
HashMapAcyclic-8    23.8µs ± 0%    24.3µs ± 1%   +1.75%  (p=0.008 n=5+5)

name              old alloc/op   new alloc/op   delta
Hash-8              2.49kB ± 0%    2.16kB ± 0%     ~     (p=0.079 n=4+5)
HashMapAcyclic-8    2.53kB ± 0%    2.53kB ± 0%     ~     (all equal)

name              old allocs/op  new allocs/op  delta
Hash-8                86.0 ± 0%      77.0 ± 0%  -10.47%  (p=0.008 n=5+5)
HashMapAcyclic-8       202 ± 0%       202 ± 0%     ~     (all equal)

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder bbb79f2d6a internal/deephash: fix accidental naked return
name              old time/op    new time/op    delta
Hash-8              23.0µs ± 1%    22.4µs ± 0%   -2.43%  (p=0.008 n=5+5)
HashMapAcyclic-8    24.0µs ± 0%    23.8µs ± 0%   -0.56%  (p=0.008 n=5+5)

name              old alloc/op   new alloc/op   delta
Hash-8              2.92kB ± 0%    2.49kB ± 0%  -14.80%  (p=0.000 n=5+4)
HashMapAcyclic-8    2.53kB ± 0%    2.53kB ± 0%     ~     (all equal)

name              old allocs/op  new allocs/op  delta
Hash-8                93.0 ± 0%      86.0 ± 0%   -7.53%  (p=0.008 n=5+5)
HashMapAcyclic-8       202 ± 0%       202 ± 0%     ~     (all equal)

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Brad Fitzpatrick 79b7fa9ac3 internal/deephash: hash maps without sorting in the acyclic common case
Hash and xor each entry instead, then write final xor'ed result.

name    old time/op    new time/op    delta
Hash-4    33.6µs ± 4%    34.6µs ± 3%  +3.03%  (p=0.013 n=10+9)

name    old alloc/op   new alloc/op   delta
Hash-4    1.86kB ± 0%    1.77kB ± 0%  -5.10%  (p=0.000 n=10+9)

name    old allocs/op  new allocs/op  delta
Hash-4      51.0 ± 0%      49.0 ± 0%  -3.92%  (p=0.000 n=10+10)

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Josh Bleecher Snyder cd54792fe9 internal/deephash: add a few more benchmarking map entries
Typical maps in production are considerably longer.
This helps benchmarks more accurately reflect the costs per key
vs the costs per map in deephash.

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder 25df067dd0 all: adapt to opaque netaddr types
This commit is a mishmash of automated edits using gofmt:

gofmt -r 'netaddr.IPPort{IP: a, Port: b} -> netaddr.IPPortFrom(a, b)' -w .
gofmt -r 'netaddr.IPPrefix{IP: a, Port: b} -> netaddr.IPPrefixFrom(a, b)' -w .

gofmt -r 'a.IP.Is4 -> a.IP().Is4' -w .
gofmt -r 'a.IP.As16 -> a.IP().As16' -w .
gofmt -r 'a.IP.Is6 -> a.IP().Is6' -w .
gofmt -r 'a.IP.As4 -> a.IP().As4' -w .
gofmt -r 'a.IP.String -> a.IP().String' -w .

And regexps:

\w*(.*)\.Port = (.*)  ->  $1 = $1.WithPort($2)
\w*(.*)\.IP = (.*)  ->  $1 = $1.WithIP($2)

And lots of manual fixups.

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder aacb2107ae all: add extra information to serialized endpoints
magicsock.Conn.ParseEndpoint requires a peer's public key,
disco key, and legacy ip/ports in order to do its job.
We currently accomplish that by:

* adding the public key in our wireguard-go fork
* encoding the disco key as magic hostname
* using a bespoke comma-separated encoding

It's a bit messy.

Instead, switch to something simpler: use a json-encoded struct
containing exactly the information we need, in the form we use it.

Our wireguard-go fork still adds the public key to the
address when it passes it to ParseEndpoint, but now the code
compensating for that is just a couple of simple, well-commented lines.
Once this commit is in, we can remove that part of the fork
and remove the compensating code.

Signed-off-by: Josh Bleecher Snyder <josharian@gmail.com>
4 years ago
Brad Fitzpatrick 36a26e6a71 internal/deephash: rename from deepprint
Yes, it printed, but that was an implementation detail for hashing.

And coming optimization will make it print even less.

Signed-off-by: Brad Fitzpatrick <bradfitz@tailscale.com>
4 years ago
Josh Bleecher Snyder 6ab2176dc7 internal/deepprint: improve benchmark
This more closely matches our real usage of deepprint.

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder 712774a697 internal/deepprint: close struct curly parens
Not that it matters, but we were missing a close parens.
It's cheap, so add it.

name    old time/op    new time/op    delta
Hash-8    6.64µs ± 0%    6.67µs ± 1%  +0.42%  (p=0.008 n=9+10)

name    old alloc/op   new alloc/op   delta
Hash-8    1.54kB ± 0%    1.54kB ± 0%    ~     (all equal)

name    old allocs/op  new allocs/op  delta
Hash-8      37.0 ± 0%      37.0 ± 0%    ~     (all equal)

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder 8368bac847 internal/deepprint: stop printing struct field names
The struct field names don't change within a single run,
so they are irrelevant. Use the field index instead.

name    old time/op    new time/op    delta
Hash-8    6.52µs ± 0%    6.64µs ± 0%   +1.91%  (p=0.000 n=6+9)

name    old alloc/op   new alloc/op   delta
Hash-8    1.67kB ± 0%    1.54kB ± 0%   -7.66%  (p=0.000 n=10+10)

name    old allocs/op  new allocs/op  delta
Hash-8      53.0 ± 0%      37.0 ± 0%  -30.19%  (p=0.000 n=10+10)

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder dfa0c90955 internal/deepprint: replace Fprintf(w, const) with w.WriteString
name    old time/op    new time/op    delta
Hash-8    7.77µs ± 0%    6.29µs ± 1%  -19.11%  (p=0.000 n=9+10)

name    old alloc/op   new alloc/op   delta
Hash-8    1.67kB ± 0%    1.67kB ± 0%     ~     (all equal)

name    old allocs/op  new allocs/op  delta
Hash-8      53.0 ± 0%      53.0 ± 0%     ~     (all equal)

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder d4f805339e internal/deepprint: special-case some common types
These show up a lot in our data structures.

name    old time/op    new time/op    delta
Hash-8    11.5µs ± 1%     7.8µs ± 1%  -32.17%  (p=0.000 n=10+10)

name    old alloc/op   new alloc/op   delta
Hash-8    1.98kB ± 0%    1.67kB ± 0%  -15.73%  (p=0.000 n=10+10)

name    old allocs/op  new allocs/op  delta
Hash-8      82.0 ± 0%      53.0 ± 0%  -35.37%  (p=0.000 n=10+10)

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder 752f8c0f2f internal/deepprint: buffer writes
The sha256 hash writer doesn't implement WriteString.
(See https://github.com/golang/go/issues/38776.)
As a consequence, we end up converting many strings to []byte.

Wrapping a bufio.Writer around the hash writer lets us
avoid these conversions by using WriteString.

Using a bufio.Writer is, perhaps surprisingly, almost as cheap as using unsafe.
The reason is that the sha256 writer does internal buffering,
but doesn't do any when handed larger writers.
Using a bufio.Writer merely shifts the data copying from one buffer
to a different one.

Using a concrete type for Print and print cuts 10% off of the execution time.

name    old time/op    new time/op    delta
Hash-8    15.3µs ± 0%    11.5µs ± 0%  -24.84%  (p=0.000 n=10+10)

name    old alloc/op   new alloc/op   delta
Hash-8    2.82kB ± 0%    1.98kB ± 0%  -29.57%  (p=0.000 n=10+10)

name    old allocs/op  new allocs/op  delta
Hash-8       140 ± 0%        82 ± 0%  -41.43%  (p=0.000 n=10+10)

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder 7891b34266 internal/deepprint: add BenchmarkHash
deepprint currently accounts for 15% of allocs in tailscaled.
This is a useful benchmark to have.

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
David Anderson 4c61ebacf4 wgengine: move DNS configuration out of wgengine/router.
Signed-off-by: David Anderson <danderson@tailscale.com>
4 years ago
Josh Bleecher Snyder 69cdc30c6d wgengine/wgcfg: remove Config.ListenPort
We don't use the port that wireguard-go passes to us (via magicsock.connBind.Open).
We ignore it entirely and use the port we selected.

When we tell wireguard-go that we're changing the listen_port,
it calls connBind.Close and then connBind.Open.
And in the meantime, it stops calling the receive functions,
which means that we stop receiving and processing UDP and DERP packets.
And that is Very Bad.

That was never a problem prior to b3ceca1dd7,
because we passed the SkipBindUpdate flag to our wireguard-go fork,
which told wireguard-go not to re-bind on listen_port changes.
That commit eliminated the SkipBindUpdate flag.

We could write a bunch of code to work around the gap.
We could add background readers that process UDP and DERP packets when wireguard-go isn't.
But it's simpler to never create the conditions in which wireguard-go rebinds.

The other scenario in which wireguard-go re-binds is device.Down.
Conveniently, we never call device.Down. We go from device.Up to device.Close,
and the latter only when we're shutting down a magicsock.Conn completely.

Rubber-ducked-by: Avery Pennarun <apenwarr@tailscale.com>
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
David Anderson 8af9d770cf net/dns: rename Config to OSConfig.
Making way for a new higher level config struct.

Signed-off-by: David Anderson <danderson@tailscale.com>
4 years ago
David Anderson 672731ac6f many: gofmt.
Signed-off-by: David Anderson <danderson@tailscale.com>
4 years ago
David Anderson 9f7f2af008 wgengine/router/dns: move to net/dns.
Preparation for merging the APIs and whatnot.

Signed-off-by: David Anderson <danderson@tailscale.com>
4 years ago
Josh Bleecher Snyder fe7c3e9c17 all: move wgcfg from wireguard-go
This is mostly code movement from the wireguard-go repo.

Most of the new wgcfg package corresponds to the wireguard-go wgcfg package.

wgengine/wgcfg/device{_test}.go was device/config{_test}.go.
There were substantive but simple changes to device_test.go to remove
internal package device references.

The API of device.Config (now wgcfg.DeviceConfig) grew an error return;
we previously logged the error and threw it away.

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder 654b5f1570 all: convert from []wgcfg.Endpoint to string
This eliminates a dependency on wgcfg.Endpoint,
as part of the effort to eliminate our wireguard-go fork.

Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Josh Bleecher Snyder 2fe770ed72 all: replace wgcfg.IP and wgcfg.CIDR with netaddr types
Signed-off-by: Josh Bleecher Snyder <josh@tailscale.com>
4 years ago
Brad Fitzpatrick 7795fcf464 Add tooldeps package to keep depaware pinned in go.mod. 4 years ago
Dmytro Shynkevych 28e52a0492
all: dns refactor, add Proxied and PerDomain flags from control (#615)
Signed-off-by: Dmytro Shynkevych <dmytro@tailscale.com>
4 years ago
Dmytro Shynkevych c7582dc234
ipn: fix netmap change tracking and dns map generation (#609)
Signed-off-by: Dmytro Shynkevych <dmytro@tailscale.com>
4 years ago
Dmytro Shynkevych 30bbbe9467
wgengine/router: dns: unify on *BSD, multimode on Linux, Magic DNS (#536)
Signed-off-by: Dmytro Shynkevych <dmytro@tailscale.com>
4 years ago
Brad Fitzpatrick 790ef2bc5f internal/deepprint: update copyright header to appease license checker script
Plus mention that it's not an exact copy.
4 years ago
Brad Fitzpatrick 7ca911a5c6 internal/deepprint: add missing copyright headers 4 years ago
Brad Fitzpatrick 6f73f2c15a wgengine, internal/deepprint: replace UAPI usage as hash func; add deepprint
The new deepprint package just walks a Go data structure and writes to
an io.Writer. It's not pretty like go-spew, etc.

We then use it to replace the use of UAPI (which we have a TODO to
remove) to generate signatures of data structures to detect whether
anything changed (without retaining the old copy).

This was necessary because the UAPI conversion ends up trying to do
DNS lookups which an upcoming change depends on not happening.
4 years ago