From 1733d3e6be3953e3a391bfcf2ea259f0a547e089 Mon Sep 17 00:00:00 2001 From: Ryan Zuklie Date: Tue, 12 Sep 2023 10:28:21 -0700 Subject: [PATCH] Make BundleKey a distinct struct from PacketTrace. This change refactors BundleKey to be a separate struct. This allows us to do the conversion (e.g. of sport/dport, or icmp fields in the future) in the constructor and change types (use optionals) where it is convenient to simplify the code. This change also fixes a bunch of desigated initializer order warnings in the test (fields should be declared in same order as defined). Test: atest libnetworkstats_test Change-Id: I6373d2512280e93c54395fde38dcff43705b449e --- .../libnetworkstats/NetworkTraceHandler.cpp | 44 ++++++++++--------- .../NetworkTraceHandlerTest.cpp | 44 +++++++++---------- .../include/netdbpf/NetworkTraceHandler.h | 30 ++++++++++--- 3 files changed, 69 insertions(+), 49 deletions(-) diff --git a/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp b/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp index ec63e41694..3520254acb 100644 --- a/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp +++ b/service-t/native/libs/libnetworkstats/NetworkTraceHandler.cpp @@ -75,9 +75,20 @@ struct BundleDetails { uint32_t bytes = 0; }; -#define AGG_FIELDS(x) \ - (x).ifindex, (x).uid, (x).tag, (x).sport, (x).dport, (x).egress, \ - (x).ipProto, (x).tcpFlags +BundleKey::BundleKey(const PacketTrace& pkt) + : ifindex(pkt.ifindex), + uid(pkt.uid), + tag(pkt.tag), + egress(pkt.egress), + ipProto(pkt.ipProto), + ipVersion(pkt.ipVersion), + tcpFlags(pkt.tcpFlags), + localPort(ntohs(pkt.egress ? pkt.sport : pkt.dport)), + remotePort(ntohs(pkt.egress ? pkt.dport : pkt.sport)) {} + +#define AGG_FIELDS(x) \ + (x).ifindex, (x).uid, (x).tag, (x).egress, (x).ipProto, (x).ipVersion, \ + (x).tcpFlags, (x).localPort, (x).remotePort std::size_t BundleHash::operator()(const BundleKey& a) const { std::size_t seed = 0; @@ -179,7 +190,7 @@ void NetworkTraceHandler::Write(const std::vector& packets, dst->set_timestamp(pkt.timestampNs); auto* event = dst->set_network_packet(); event->set_length(pkt.length); - Fill(pkt, event); + Fill(BundleKey(pkt), event); } return; } @@ -187,14 +198,13 @@ void NetworkTraceHandler::Write(const std::vector& packets, uint64_t minTs = std::numeric_limits::max(); std::unordered_map bundles; for (const PacketTrace& pkt : packets) { - BundleKey key = pkt; + BundleKey key(pkt); // Dropping fields should remove them from the output and remove them from - // the aggregation key. In order to do the latter without changing the hash - // function, set the dropped fields to zero. - if (mDropTcpFlags) key.tcpFlags = 0; - if (mDropLocalPort) (key.egress ? key.sport : key.dport) = 0; - if (mDropRemotePort) (key.egress ? key.dport : key.sport) = 0; + // the aggregation key. Reset the optionals to indicate omission. + if (mDropTcpFlags) key.tcpFlags.reset(); + if (mDropLocalPort) key.localPort.reset(); + if (mDropRemotePort) key.remotePort.reset(); minTs = std::min(minTs, pkt.timestampNs); @@ -245,22 +255,16 @@ void NetworkTraceHandler::Write(const std::vector& packets, } } -void NetworkTraceHandler::Fill(const PacketTrace& src, +void NetworkTraceHandler::Fill(const BundleKey& src, NetworkPacketEvent* event) { event->set_direction(src.egress ? TrafficDirection::DIR_EGRESS : TrafficDirection::DIR_INGRESS); event->set_uid(src.uid); event->set_tag(src.tag); - if (!mDropLocalPort) { - event->set_local_port(ntohs(src.egress ? src.sport : src.dport)); - } - if (!mDropRemotePort) { - event->set_remote_port(ntohs(src.egress ? src.dport : src.sport)); - } - if (!mDropTcpFlags) { - event->set_tcp_flags(src.tcpFlags); - } + if (src.tcpFlags.has_value()) event->set_tcp_flags(*src.tcpFlags); + if (src.localPort.has_value()) event->set_local_port(*src.localPort); + if (src.remotePort.has_value()) event->set_remote_port(*src.remotePort); event->set_ip_proto(src.ipProto); diff --git a/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp b/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp index f2c1a867bf..2eb2e4f313 100644 --- a/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp +++ b/service-t/native/libs/libnetworkstats/NetworkTraceHandlerTest.cpp @@ -168,12 +168,12 @@ TEST_F(NetworkTraceHandlerTest, BasicBundling) { config.set_aggregation_threshold(10); std::vector input = { - PacketTrace{.uid = 123, .timestampNs = 2, .length = 200}, - PacketTrace{.uid = 123, .timestampNs = 1, .length = 100}, - PacketTrace{.uid = 123, .timestampNs = 4, .length = 300}, + PacketTrace{.timestampNs = 2, .length = 200, .uid = 123}, + PacketTrace{.timestampNs = 1, .length = 100, .uid = 123}, + PacketTrace{.timestampNs = 4, .length = 300, .uid = 123}, - PacketTrace{.uid = 456, .timestampNs = 2, .length = 400}, - PacketTrace{.uid = 456, .timestampNs = 4, .length = 100}, + PacketTrace{.timestampNs = 2, .length = 400, .uid = 456}, + PacketTrace{.timestampNs = 4, .length = 100, .uid = 456}, }; std::vector events; @@ -203,12 +203,12 @@ TEST_F(NetworkTraceHandlerTest, AggregationThreshold) { config.set_aggregation_threshold(3); std::vector input = { - PacketTrace{.uid = 123, .timestampNs = 2, .length = 200}, - PacketTrace{.uid = 123, .timestampNs = 1, .length = 100}, - PacketTrace{.uid = 123, .timestampNs = 4, .length = 300}, + PacketTrace{.timestampNs = 2, .length = 200, .uid = 123}, + PacketTrace{.timestampNs = 1, .length = 100, .uid = 123}, + PacketTrace{.timestampNs = 4, .length = 300, .uid = 123}, - PacketTrace{.uid = 456, .timestampNs = 2, .length = 400}, - PacketTrace{.uid = 456, .timestampNs = 4, .length = 100}, + PacketTrace{.timestampNs = 2, .length = 400, .uid = 456}, + PacketTrace{.timestampNs = 4, .length = 100, .uid = 456}, }; std::vector events; @@ -239,10 +239,10 @@ TEST_F(NetworkTraceHandlerTest, DropLocalPort) { __be16 b = htons(10001); std::vector input = { // Recall that local is `src` for egress and `dst` for ingress. - PacketTrace{.timestampNs = 1, .length = 2, .egress = true, .sport = a}, - PacketTrace{.timestampNs = 2, .length = 4, .egress = false, .dport = a}, - PacketTrace{.timestampNs = 3, .length = 6, .egress = true, .sport = b}, - PacketTrace{.timestampNs = 4, .length = 8, .egress = false, .dport = b}, + PacketTrace{.timestampNs = 1, .length = 2, .sport = a, .egress = true}, + PacketTrace{.timestampNs = 2, .length = 4, .dport = a, .egress = false}, + PacketTrace{.timestampNs = 3, .length = 6, .sport = b, .egress = true}, + PacketTrace{.timestampNs = 4, .length = 8, .dport = b, .egress = false}, }; std::vector events; @@ -274,10 +274,10 @@ TEST_F(NetworkTraceHandlerTest, DropRemotePort) { __be16 b = htons(80); std::vector input = { // Recall that remote is `dst` for egress and `src` for ingress. - PacketTrace{.timestampNs = 1, .length = 2, .egress = true, .dport = a}, - PacketTrace{.timestampNs = 2, .length = 4, .egress = false, .sport = a}, - PacketTrace{.timestampNs = 3, .length = 6, .egress = true, .dport = b}, - PacketTrace{.timestampNs = 4, .length = 8, .egress = false, .sport = b}, + PacketTrace{.timestampNs = 1, .length = 2, .dport = a, .egress = true}, + PacketTrace{.timestampNs = 2, .length = 4, .sport = a, .egress = false}, + PacketTrace{.timestampNs = 3, .length = 6, .dport = b, .egress = true}, + PacketTrace{.timestampNs = 4, .length = 8, .sport = b, .egress = false}, }; std::vector events; @@ -306,10 +306,10 @@ TEST_F(NetworkTraceHandlerTest, DropTcpFlags) { config.set_aggregation_threshold(10); std::vector input = { - PacketTrace{.timestampNs = 1, .uid = 123, .length = 1, .tcpFlags = 1}, - PacketTrace{.timestampNs = 2, .uid = 123, .length = 2, .tcpFlags = 2}, - PacketTrace{.timestampNs = 3, .uid = 456, .length = 3, .tcpFlags = 1}, - PacketTrace{.timestampNs = 4, .uid = 456, .length = 4, .tcpFlags = 2}, + PacketTrace{.timestampNs = 1, .length = 1, .uid = 123, .tcpFlags = 1}, + PacketTrace{.timestampNs = 2, .length = 2, .uid = 123, .tcpFlags = 2}, + PacketTrace{.timestampNs = 3, .length = 3, .uid = 456, .tcpFlags = 1}, + PacketTrace{.timestampNs = 4, .length = 4, .uid = 456, .tcpFlags = 2}, }; std::vector events; diff --git a/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h b/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h index bc10e68cce..853e26e09e 100644 --- a/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h +++ b/service-t/native/libs/libnetworkstats/include/netdbpf/NetworkTraceHandler.h @@ -30,15 +30,31 @@ namespace android { namespace bpf { -// BundleKeys are PacketTraces where timestamp and length are ignored. -using BundleKey = PacketTrace; +// BundleKey encodes a PacketTrace minus timestamp and length. The key should +// match many packets over time for interning. For convenience, sport/dport +// are parsed here as either local/remote port. +struct BundleKey { + explicit BundleKey(const PacketTrace& pkt); -// BundleKeys are hashed using all fields except timestamp/length. + uint32_t ifindex; + uint32_t uid; + uint32_t tag; + + bool egress; + uint8_t ipProto; + uint8_t ipVersion; + + std::optional tcpFlags; + std::optional localPort; + std::optional remotePort; +}; + +// BundleKeys are hashed using a simple hash combine. struct BundleHash { std::size_t operator()(const BundleKey& a) const; }; -// BundleKeys are equal if all fields except timestamp/length are equal. +// BundleKeys are equal if all fields are equal. struct BundleEq { bool operator()(const BundleKey& a, const BundleKey& b) const; }; @@ -84,13 +100,13 @@ class NetworkTraceHandler NetworkTraceHandler::TraceContext& ctx); private: - // Convert a PacketTrace into a Perfetto trace packet. - void Fill(const PacketTrace& src, + // Fills in contextual information from a bundle without interning. + void Fill(const BundleKey& src, ::perfetto::protos::pbzero::NetworkPacketEvent* event); // Fills in contextual information either inline or via interning. ::perfetto::protos::pbzero::NetworkPacketBundle* FillWithInterning( - NetworkTraceState* state, const BundleKey& key, + NetworkTraceState* state, const BundleKey& src, ::perfetto::protos::pbzero::TracePacket* dst); static internal::NetworkTracePoller sPoller;