Remove duplicated bpf offload support check in IpServer
In current code, IpServer will ensure bpf offload is supported before
calling BpfCoordinator's APIs. But those APIs already have the same
checks inside the function. This CL removed duplicated checks in
IpServer.
After this change, the BPF offload support status is only determined
inside BpfCoordinator and it won't change during the life cycle of
Tethering. Tethering initializes mBpfCoordinator just once in the
constructor. After that point, the value of isUsingBpf can never
change, because mIsBpfEnabled is final and
mBpfCoordinatorShim.isInitialized() is either always true (api30) or
can either never change (api31).
Also fixed a bug in IpServerTest upon this change.
Test: atest TetheringTests
Bug: 261923493
Change-Id: I50f231a83bf678b90eae4fd9bfa6035e86978bb3
diff --git a/Tethering/src/android/net/ip/IpServer.java b/Tethering/src/android/net/ip/IpServer.java
index eadba58..a851410 100644
--- a/Tethering/src/android/net/ip/IpServer.java
+++ b/Tethering/src/android/net/ip/IpServer.java
@@ -252,7 +252,6 @@
private final int mInterfaceType;
private final LinkProperties mLinkProperties;
private final boolean mUsingLegacyDhcp;
- private final boolean mUsingBpfOffload;
private final int mP2pLeasesSubnetPrefixLength;
private final Dependencies mDeps;
@@ -314,7 +313,6 @@
mInterfaceType = interfaceType;
mLinkProperties = new LinkProperties();
mUsingLegacyDhcp = config.useLegacyDhcpServer();
- mUsingBpfOffload = config.isBpfOffloadEnabled();
mP2pLeasesSubnetPrefixLength = config.getP2pLeasesSubnetPrefixLength();
mPrivateAddressCoordinator = addressCoordinator;
mDeps = deps;
@@ -326,11 +324,9 @@
mIpNeighborMonitor = mDeps.getIpNeighborMonitor(getHandler(), mLog,
new MyNeighborEventConsumer());
- // IP neighbor monitor monitors the neighbor events for adding/removing offload
- // forwarding rules per client. If BPF offload is not supported, don't start listening
- // for neighbor events. See updateIpv6ForwardingRules, addIpv6DownstreamRule,
- // removeIpv6DownstreamRule.
- if (mUsingBpfOffload && !mIpNeighborMonitor.start()) {
+ // IP neighbor monitor monitors the neighbor events for adding/removing IPv6 downstream rule
+ // per client. If BPF offload is not supported, don't start listening for neighbor events.
+ if (mBpfCoordinator.isUsingBpfOffload() && !mIpNeighborMonitor.start()) {
mLog.e("Failed to create IpNeighborMonitor on " + mIfaceName);
}
@@ -891,37 +887,6 @@
}
}
- private void addIpv6DownstreamRule(Ipv6DownstreamRule rule) {
- // Theoretically, we don't need this check because IP neighbor monitor doesn't start if BPF
- // offload is disabled. Add this check just in case.
- // TODO: Perhaps remove this protection check.
- if (!mUsingBpfOffload) return;
-
- mBpfCoordinator.addIpv6DownstreamRule(this, rule);
- }
-
- private void removeIpv6DownstreamRule(Ipv6DownstreamRule rule) {
- // TODO: Perhaps remove this protection check.
- // See the related comment in #addIpv6DownstreamRule.
- if (!mUsingBpfOffload) return;
-
- mBpfCoordinator.removeIpv6DownstreamRule(this, rule);
- }
-
- private void clearIpv6ForwardingRules() {
- if (!mUsingBpfOffload) return;
-
- mBpfCoordinator.tetherOffloadRuleClear(this);
- }
-
- private void updateIpv6ForwardingRule(int newIfindex) {
- // TODO: Perhaps remove this protection check.
- // See the related comment in #addIpv6DownstreamRule.
- if (!mUsingBpfOffload) return;
-
- mBpfCoordinator.tetherOffloadRuleUpdate(this, newIfindex);
- }
-
// Handles all updates to IPv6 forwarding rules. These can currently change only if the upstream
// changes or if a neighbor event is received.
private void updateIpv6ForwardingRules(int prevUpstreamIfindex, int upstreamIfindex,
@@ -930,14 +895,14 @@
// nothing else.
// TODO: Rather than always clear rules, ensure whether ipv6 ever enable first.
if (upstreamIfindex == 0 || !upstreamSupportsBpf) {
- clearIpv6ForwardingRules();
+ mBpfCoordinator.tetherOffloadRuleClear(this);
return;
}
// If the upstream interface has changed, remove all rules and re-add them with the new
// upstream interface.
if (prevUpstreamIfindex != upstreamIfindex) {
- updateIpv6ForwardingRule(upstreamIfindex);
+ mBpfCoordinator.tetherOffloadRuleUpdate(this, upstreamIfindex);
}
// If we're here to process a NeighborEvent, do so now.
@@ -955,18 +920,14 @@
Ipv6DownstreamRule rule = new Ipv6DownstreamRule(upstreamIfindex, mInterfaceParams.index,
(Inet6Address) e.ip, mInterfaceParams.macAddr, dstMac);
if (e.isValid()) {
- addIpv6DownstreamRule(rule);
+ mBpfCoordinator.addIpv6DownstreamRule(this, rule);
} else {
- removeIpv6DownstreamRule(rule);
+ mBpfCoordinator.removeIpv6DownstreamRule(this, rule);
}
}
// TODO: consider moving into BpfCoordinator.
private void updateClientInfoIpv4(NeighborEvent e) {
- // TODO: Perhaps remove this protection check.
- // See the related comment in #addIpv6DownstreamRule.
- if (!mUsingBpfOffload) return;
-
if (e == null) return;
if (!(e.ip instanceof Inet4Address) || e.ip.isMulticastAddress()
|| e.ip.isLoopbackAddress() || e.ip.isLinkLocalAddress()) {
@@ -1342,7 +1303,7 @@
for (String ifname : mUpstreamIfaceSet.ifnames) cleanupUpstreamInterface(ifname);
mUpstreamIfaceSet = null;
- clearIpv6ForwardingRules();
+ mBpfCoordinator.tetherOffloadRuleClear(IpServer.this);
}
private void cleanupUpstreamInterface(String upstreamIface) {
diff --git a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java
index 7311125..1b23a6c 100644
--- a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java
+++ b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java
@@ -523,6 +523,18 @@
mLog.i("Polling stopped");
}
+ /**
+ * Return whether BPF offload is supported
+ */
+ public boolean isUsingBpfOffload() {
+ return isUsingBpf();
+ }
+
+ // This is identical to isUsingBpfOffload above but is only used internally.
+ // The reason for having two separate methods is that the code calls isUsingBpf
+ // very often. But the tests call verifyNoMoreInteractions, which will check all
+ // calls to public methods. If isUsingBpf were public, the test would need to
+ // verify all calls to it, which would clutter the test.
private boolean isUsingBpf() {
return mIsBpfEnabled && mBpfCoordinatorShim.isInitialized();
}
diff --git a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java
index 19d70c6..c0718d1 100644
--- a/Tethering/tests/unit/src/android/net/ip/IpServerTest.java
+++ b/Tethering/tests/unit/src/android/net/ip/IpServerTest.java
@@ -244,6 +244,8 @@
when(mTetherConfig.isBpfOffloadEnabled()).thenReturn(usingBpfOffload);
when(mTetherConfig.useLegacyDhcpServer()).thenReturn(usingLegacyDhcp);
when(mTetherConfig.getP2pLeasesSubnetPrefixLength()).thenReturn(P2P_SUBNET_PREFIX_LENGTH);
+ // Recreate mBpfCoordinator again here because mTetherConfig has changed
+ mBpfCoordinator = spy(new BpfCoordinator(mBpfDeps));
mIpServer = new IpServer(
IFACE_NAME, mLooper.getLooper(), interfaceType, mSharedLog, mNetd, mBpfCoordinator,
mCallback, mTetherConfig, mAddressCoordinator, mTetheringMetrics, mDependencies);
@@ -1244,13 +1246,11 @@
resetNetdBpfMapAndCoordinator();
recvNewNeigh(myIfindex, neigh, NUD_REACHABLE, macA);
- verify(mBpfCoordinator, never()).addIpv6DownstreamRule(any(), any());
verifyNeverTetherOffloadRuleAdd();
verifyNoUpstreamIpv6ForwardingChange(null);
resetNetdBpfMapAndCoordinator();
recvDelNeigh(myIfindex, neigh, NUD_STALE, macA);
- verify(mBpfCoordinator, never()).removeIpv6DownstreamRule(any(), any());
verifyNeverTetherOffloadRuleRemove();
verifyNoUpstreamIpv6ForwardingChange(null);
resetNetdBpfMapAndCoordinator();