joy/android_packages_modules_Connectivity
Watch
1
Fork
You've already forked android_packages_modules_Connectivity
 0
Code Issues Pull requests Projects Releases Wiki Activity

Simplify tethering active session counting

Browse source 
The previous implementation for tracking tethering active sessions
was overly complex. This commit simplifies the logic by:

- Moving the counting to the BpfCoordinatorShimImpl (31 impl)
  where rule addition and deletion are centralized.
- Eliminating the unnecessary Android version check (S+) as
  the 31 shim only runs on S+.
- Relocating the tracking counters from
  ConntrackEventConsumer to the shim class, improving code
  locality and encapsulation.

This change addresses the feedback from the networking mainline
release council and resolves a release blocker.

Test: atest TetheringTests:com.android.networkstack.tethering.BpfCoordinatorTest
Fix: 370652384
Change-Id: I70ca58fc45694c738745d7a872272641a62551fd
This commit is contained in:
Junyu Lai 2024-10-01 16:34:32 +08:00
commit 3e90bc85b3
5 changed files with 100 additions and 111 deletions
Download patch file Download diff file Expand all files Collapse all files

9
Tethering/apishim/30/com/android/networkstack/tethering/apishim/api30/BpfCoordinatorShimImpl.java
View file

@ -198,4 +198,13 @@ public class BpfCoordinatorShimImpl
public String toString() {
return "Netd used";
}
@Override
public int getLastMaxConnectionAndResetToCurrent() {
return 0;
}
@Override
public void clearConnectionCounters() {
}
}

61
Tethering/apishim/31/com/android/networkstack/tethering/apishim/api31/BpfCoordinatorShimImpl.java
View file

@ -19,6 +19,7 @@ package com.android.networkstack.tethering.apishim.api31;
import static android.net.netstats.provider.NetworkStatsProvider.QUOTA_UNLIMITED;
import static com.android.net.module.util.NetworkStackConstants.RFC7421_PREFIX_LENGTH;
import static com.android.networkstack.tethering.TetheringConfiguration.TETHER_ACTIVE_SESSIONS_METRICS;
import android.system.ErrnoException;
import android.system.Os;
@ -108,6 +109,22 @@ public class BpfCoordinatorShimImpl
// TODO: Add IPv6 rule count.
private final SparseArray<Integer> mRule4CountOnUpstream = new SparseArray<>();
private final boolean mSupportActiveSessionsMetrics;
/**
* Tracks the current number of tethering connections and the maximum
* observed since the last metrics collection. Used to provide insights
* into the distribution of active tethering sessions for metrics reporting.
* These variables are accessed on the handler thread, which includes:
* 1. ConntrackEvents signaling the addition or removal of an IPv4 rule.
* 2. ConntrackEvents indicating the removal of a tethering client,
* triggering the removal of associated rules.
* 3. Removal of the last IpServer, which resets counters to handle
* potential synchronization issues.
*/
private int mLastMaxConnectionCount = 0;
private int mCurrentConnectionCount = 0;
public BpfCoordinatorShimImpl(@NonNull final Dependencies deps) {
mLog = deps.getSharedLog().forSubComponent(TAG);
@ -156,6 +173,9 @@ public class BpfCoordinatorShimImpl
} catch (ErrnoException e) {
mLog.e("Could not clear mBpfDevMap: " + e);
}
mSupportActiveSessionsMetrics = deps.isFeatureEnabled(deps.getContext(),
TETHER_ACTIVE_SESSIONS_METRICS);
}
@Override
@ -350,6 +370,12 @@ public class BpfCoordinatorShimImpl
final int upstreamIfindex = (int) key.iif;
int count = mRule4CountOnUpstream.get(upstreamIfindex, 0 /* default */);
mRule4CountOnUpstream.put(upstreamIfindex, ++count);
if (mSupportActiveSessionsMetrics) {
mCurrentConnectionCount++;
mLastMaxConnectionCount = Math.max(mCurrentConnectionCount,
mLastMaxConnectionCount);
}
} else {
mBpfUpstream4Map.insertEntry(key, value);
}
@ -385,6 +411,10 @@ public class BpfCoordinatorShimImpl
} else {
mRule4CountOnUpstream.put(upstreamIfindex, count);
}
if (mSupportActiveSessionsMetrics) {
mCurrentConnectionCount--;
}
} else {
if (!mBpfUpstream4Map.deleteEntry(key)) return false; // Rule did not exist
}
@ -465,14 +495,16 @@ public class BpfCoordinatorShimImpl
@Override
public String toString() {
return String.join(", ", new String[] {
mapStatus(mBpfDownstream6Map, "mBpfDownstream6Map"),
mapStatus(mBpfUpstream6Map, "mBpfUpstream6Map"),
mapStatus(mBpfDownstream4Map, "mBpfDownstream4Map"),
mapStatus(mBpfUpstream4Map, "mBpfUpstream4Map"),
mapStatus(mBpfStatsMap, "mBpfStatsMap"),
mapStatus(mBpfLimitMap, "mBpfLimitMap"),
mapStatus(mBpfDevMap, "mBpfDevMap")
return String.join(", ", new String[]{
mapStatus(mBpfDownstream6Map, "mBpfDownstream6Map"),
mapStatus(mBpfUpstream6Map, "mBpfUpstream6Map"),
mapStatus(mBpfDownstream4Map, "mBpfDownstream4Map"),
mapStatus(mBpfUpstream4Map, "mBpfUpstream4Map"),
mapStatus(mBpfStatsMap, "mBpfStatsMap"),
mapStatus(mBpfLimitMap, "mBpfLimitMap"),
mapStatus(mBpfDevMap, "mBpfDevMap"),
"mCurrentConnectionCount=" + mCurrentConnectionCount,
"mLastMaxConnectionCount=" + mLastMaxConnectionCount
});
}
@ -507,4 +539,17 @@ public class BpfCoordinatorShimImpl
return 0;
}
/** Get last max connection count and reset to current count. */
public int getLastMaxConnectionAndResetToCurrent() {
final int ret = mLastMaxConnectionCount;
mLastMaxConnectionCount = mCurrentConnectionCount;
return ret;
}
/** Clear current connection count. */
public void clearConnectionCounters() {
mCurrentConnectionCount = 0;
mLastMaxConnectionCount = 0;
}
}

6
Tethering/apishim/common/com/android/networkstack/tethering/apishim/common/BpfCoordinatorShim.java
View file

@ -202,5 +202,11 @@ public abstract class BpfCoordinatorShim {
* Remove interface index mapping.
*/
public abstract boolean removeDevMap(int ifIndex);
/** Get last max connection count and reset to current count. */
public abstract int getLastMaxConnectionAndResetToCurrent();
/** Clear current connection count. */
public abstract void clearConnectionCounters();
}

73
Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java
View file

@ -27,6 +27,7 @@ import static android.net.netstats.provider.NetworkStatsProvider.QUOTA_UNLIMITED
import static android.system.OsConstants.ETH_P_IP;
import static android.system.OsConstants.ETH_P_IPV6;
import static com.android.internal.annotations.VisibleForTesting.Visibility.PRIVATE;
import static com.android.net.module.util.NetworkStackConstants.IPV4_MIN_MTU;
import static com.android.net.module.util.NetworkStackConstants.IPV6_ADDR_LEN;
import static com.android.net.module.util.ip.ConntrackMonitor.ConntrackEvent;
@ -334,7 +335,6 @@ public class BpfCoordinator {
};
// TODO: add BpfMap<TetherDownstream64Key, TetherDownstream64Value> retrieving function.
@VisibleForTesting
public abstract static class Dependencies {
/** Get handler. */
@NonNull public abstract Handler getHandler();
@ -585,14 +585,10 @@ public class BpfCoordinator {
if (mHandler.hasCallbacks(mScheduledConntrackMetricsSampling)) {
mHandler.removeCallbacks(mScheduledConntrackMetricsSampling);
}
final int currentCount = mBpfConntrackEventConsumer.getCurrentConnectionCount();
if (currentCount != 0) {
Log.wtf(TAG, "Unexpected CurrentConnectionCount: " + currentCount);
}
// Avoid sending metrics when tethering is about to close.
// This leads to a missing final sample before disconnect
// but avoids possibly duplicating the last metric in the upload.
mBpfConntrackEventConsumer.clearConnectionCounters();
mBpfCoordinatorShim.clearConnectionCounters();
}
// Stop scheduled polling stats and poll the latest stats from BPF maps.
if (mHandler.hasCallbacks(mScheduledPollingStats)) {
@ -1091,10 +1087,6 @@ public class BpfCoordinator {
for (final Tether4Key k : deleteDownstreamRuleKeys) {
mBpfCoordinatorShim.tetherOffloadRuleRemove(DOWNSTREAM, k);
}
if (mSupportActiveSessionsMetrics) {
mBpfConntrackEventConsumer.decreaseCurrentConnectionCount(
deleteUpstreamRuleKeys.size());
}
// Cleanup each upstream interface by a set which avoids duplicated work on the same
// upstream interface. Cleaning up the same interface twice (or more) here may raise
@ -1367,10 +1359,6 @@ public class BpfCoordinator {
pw.println();
pw.println("mSupportActiveSessionsMetrics: " + mSupportActiveSessionsMetrics);
pw.println("getLastMaxConnectionCount: "
+ mBpfConntrackEventConsumer.getLastMaxConnectionCount());
pw.println("getCurrentConnectionCount: "
+ mBpfConntrackEventConsumer.getCurrentConnectionCount());
}
private void dumpStats(@NonNull IndentingPrintWriter pw) {
@ -2062,21 +2050,6 @@ public class BpfCoordinator {
// while TCP status is established.
@VisibleForTesting
class BpfConntrackEventConsumer implements ConntrackEventConsumer {
/**
* Tracks the current number of tethering connections and the maximum
* observed since the last metrics collection. Used to provide insights
* into the distribution of active tethering sessions for metrics reporting.
* These variables are accessed on the handler thread, which includes:
* 1. ConntrackEvents signaling the addition or removal of an IPv4 rule.
* 2. ConntrackEvents indicating the removal of a tethering client,
* triggering the removal of associated rules.
* 3. Removal of the last IpServer, which resets counters to handle
* potential synchronization issues.
*/
private int mLastMaxConnectionCount = 0;
private int mCurrentConnectionCount = 0;
// The upstream4 and downstream4 rules are built as the following tables. Only raw ip
// upstream interface is supported. Note that the field "lastUsed" is only updated by
// BPF program which records the last used time for a given rule.
@ -2210,10 +2183,6 @@ public class BpfCoordinator {
return;
}
if (mSupportActiveSessionsMetrics) {
decreaseCurrentConnectionCount(1);
}
maybeClearLimit(upstreamIndex);
return;
}
@ -2237,40 +2206,12 @@ public class BpfCoordinator {
+ ", downstream: " + addedDownstream + ")");
return;
}
if (mSupportActiveSessionsMetrics && addedUpstream && addedDownstream) {
mCurrentConnectionCount++;
mLastMaxConnectionCount = Math.max(mCurrentConnectionCount,
mLastMaxConnectionCount);
}
}
}
public int getLastMaxConnectionAndResetToCurrent() {
final int ret = mLastMaxConnectionCount;
mLastMaxConnectionCount = mCurrentConnectionCount;
return ret;
}
/** For dumping current state only. */
public int getLastMaxConnectionCount() {
return mLastMaxConnectionCount;
}
public int getCurrentConnectionCount() {
return mCurrentConnectionCount;
}
public void decreaseCurrentConnectionCount(int count) {
mCurrentConnectionCount -= count;
if (mCurrentConnectionCount < 0) {
Log.wtf(TAG, "Unexpected mCurrentConnectionCount: "
+ mCurrentConnectionCount);
}
}
public void clearConnectionCounters() {
mCurrentConnectionCount = 0;
mLastMaxConnectionCount = 0;
}
@VisibleForTesting(visibility = PRIVATE)
public int getLastMaxConnectionAndResetToCurrent() {
return mBpfCoordinatorShim.getLastMaxConnectionAndResetToCurrent();
}
@VisibleForTesting
@ -2611,7 +2552,7 @@ public class BpfCoordinator {
private void uploadConntrackMetricsSample() {
mDeps.sendTetheringActiveSessionsReported(
mBpfConntrackEventConsumer.getLastMaxConnectionAndResetToCurrent());
mBpfCoordinatorShim.getLastMaxConnectionAndResetToCurrent());
}
private void schedulePollingStats() {

62
Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java
View file

@ -2032,7 +2032,7 @@ public class BpfCoordinatorTest {
final BpfCoordinator coordinator = makeBpfCoordinator();
initBpfCoordinatorForRule4(coordinator);
resetNetdAndBpfMaps();
assertEquals(0, mConsumer.getLastMaxConnectionAndResetToCurrent());
assertEquals(0, coordinator.getLastMaxConnectionAndResetToCurrent());
// Prepare add/delete rule events.
final ArrayList<ConntrackEvent> addRuleEvents = new ArrayList<>();
@ -2049,49 +2049,44 @@ public class BpfCoordinatorTest {
// Add rules, verify counter increases.
for (int i = 0; i < 5; i++) {
mConsumer.accept(addRuleEvents.get(i));
assertConsumerCountersEquals(supportActiveSessionsMetrics ? i + 1 : 0);
assertEquals(supportActiveSessionsMetrics ? i + 1 : 0,
coordinator.getLastMaxConnectionAndResetToCurrent());
}
// Add the same events again should not increase the counter because
// all events are already exist.
for (final ConntrackEvent event : addRuleEvents) {
mConsumer.accept(event);
assertConsumerCountersEquals(supportActiveSessionsMetrics ? 5 : 0);
assertEquals(supportActiveSessionsMetrics ? 5 : 0,
coordinator.getLastMaxConnectionAndResetToCurrent());
}
// Verify removing non-existent items won't change the counters.
for (int i = 5; i < 8; i++) {
mConsumer.accept(new TestConntrackEvent.Builder().setMsgType(
IPCTNL_MSG_CT_DELETE).setProto(IPPROTO_TCP).setRemotePort(i).build());
assertConsumerCountersEquals(supportActiveSessionsMetrics ? 5 : 0);
assertEquals(supportActiveSessionsMetrics ? 5 : 0,
coordinator.getLastMaxConnectionAndResetToCurrent());
}
// Verify remove the rules decrease the counter.
// Note the max counter returns the max, so it returns the count before deleting.
for (int i = 0; i < 5; i++) {
mConsumer.accept(delRuleEvents.get(i));
assertEquals(supportActiveSessionsMetrics ? 4 - i : 0,
mConsumer.getCurrentConnectionCount());
assertEquals(supportActiveSessionsMetrics ? 5 - i : 0,
mConsumer.getLastMaxConnectionCount());
assertEquals(supportActiveSessionsMetrics ? 5 - i : 0,
mConsumer.getLastMaxConnectionAndResetToCurrent());
}
// The maximum number of rules observed is still 5.
assertEquals(supportActiveSessionsMetrics ? 5 : 0,
coordinator.getLastMaxConnectionAndResetToCurrent());
// After the reset, the maximum number of rules observed is 0.
assertEquals(0, coordinator.getLastMaxConnectionAndResetToCurrent());
// Verify remove these rules again doesn't decrease the counter.
for (int i = 0; i < 5; i++) {
mConsumer.accept(delRuleEvents.get(i));
assertConsumerCountersEquals(0);
assertEquals(0, coordinator.getLastMaxConnectionAndResetToCurrent());
}
}
// Helper method to assert all counter values inside consumer.
private void assertConsumerCountersEquals(int expectedCount) {
assertEquals(expectedCount, mConsumer.getCurrentConnectionCount());
assertEquals(expectedCount, mConsumer.getLastMaxConnectionCount());
assertEquals(expectedCount, mConsumer.getLastMaxConnectionAndResetToCurrent());
}
@FeatureFlag(name = TETHER_ACTIVE_SESSIONS_METRICS)
// BPF IPv4 forwarding only supports on S+.
@IgnoreUpTo(Build.VERSION_CODES.R)
@ -2121,7 +2116,7 @@ public class BpfCoordinatorTest {
coordinator.tetherOffloadClientAdd(mIpServer, clientB);
assertClientInfoExists(mIpServer, clientA);
assertClientInfoExists(mIpServer, clientB);
assertEquals(0, mConsumer.getLastMaxConnectionAndResetToCurrent());
assertEquals(0, coordinator.getLastMaxConnectionAndResetToCurrent());
// Add some rules for both clients.
final int addr1RuleCount = 5;
@ -2145,31 +2140,24 @@ public class BpfCoordinatorTest {
.build());
}
assertConsumerCountersEquals(
supportActiveSessionsMetrics ? addr1RuleCount + addr2RuleCount : 0);
assertEquals(supportActiveSessionsMetrics ? addr1RuleCount + addr2RuleCount : 0,
coordinator.getLastMaxConnectionAndResetToCurrent());
// Remove 1 client. Since the 1st poll will return the LastMaxCounter and
// update it to the current, the max counter will be kept at 1st poll, while
// the current counter reflect the rule decreasing.
// update it to the current, the max counter will be kept at 1st poll.
coordinator.tetherOffloadClientRemove(mIpServer, clientA);
assertEquals(supportActiveSessionsMetrics ? addr1RuleCount + addr2RuleCount : 0,
coordinator.getLastMaxConnectionAndResetToCurrent());
// And the counter be updated at 2nd poll.
assertEquals(supportActiveSessionsMetrics ? addr2RuleCount : 0,
mConsumer.getCurrentConnectionCount());
assertEquals(supportActiveSessionsMetrics ? addr1RuleCount + addr2RuleCount : 0,
mConsumer.getLastMaxConnectionCount());
assertEquals(supportActiveSessionsMetrics ? addr1RuleCount + addr2RuleCount : 0,
mConsumer.getLastMaxConnectionAndResetToCurrent());
// And all counters be updated at 2nd poll.
assertConsumerCountersEquals(supportActiveSessionsMetrics ? addr2RuleCount : 0);
coordinator.getLastMaxConnectionAndResetToCurrent());
// Remove other client.
coordinator.tetherOffloadClientRemove(mIpServer, clientB);
assertEquals(0, mConsumer.getCurrentConnectionCount());
assertEquals(supportActiveSessionsMetrics ? addr2RuleCount : 0,
mConsumer.getLastMaxConnectionCount());
assertEquals(supportActiveSessionsMetrics ? addr2RuleCount : 0,
mConsumer.getLastMaxConnectionAndResetToCurrent());
// All counters reach zero at 2nd poll.
assertConsumerCountersEquals(0);
coordinator.getLastMaxConnectionAndResetToCurrent());
// Verify the counter reach zero at 2nd poll.
assertEquals(0, coordinator.getLastMaxConnectionAndResetToCurrent());
}
@FeatureFlag(name = TETHER_ACTIVE_SESSIONS_METRICS)
@ -2191,7 +2179,7 @@ public class BpfCoordinatorTest {
final BpfCoordinator coordinator = makeBpfCoordinator();
initBpfCoordinatorForRule4(coordinator);
resetNetdAndBpfMaps();
assertConsumerCountersEquals(0);
assertEquals(0, coordinator.getLastMaxConnectionAndResetToCurrent());
// Prepare the counter value.
for (int i = 0; i < 5; i++) {