diff --git a/Tethering/apishim/30/com/android/networkstack/tethering/apishim/api30/BpfCoordinatorShimImpl.java b/Tethering/apishim/30/com/android/networkstack/tethering/apishim/api30/BpfCoordinatorShimImpl.java index 0df904786f..af061e455b 100644 --- a/Tethering/apishim/30/com/android/networkstack/tethering/apishim/api30/BpfCoordinatorShimImpl.java +++ b/Tethering/apishim/30/com/android/networkstack/tethering/apishim/api30/BpfCoordinatorShimImpl.java @@ -198,4 +198,13 @@ public class BpfCoordinatorShimImpl public String toString() { return "Netd used"; } + + @Override + public int getLastMaxConnectionAndResetToCurrent() { + return 0; + } + + @Override + public void clearConnectionCounters() { + } } diff --git a/Tethering/apishim/31/com/android/networkstack/tethering/apishim/api31/BpfCoordinatorShimImpl.java b/Tethering/apishim/31/com/android/networkstack/tethering/apishim/api31/BpfCoordinatorShimImpl.java index e6e99f4c2c..b460f0d0b1 100644 --- a/Tethering/apishim/31/com/android/networkstack/tethering/apishim/api31/BpfCoordinatorShimImpl.java +++ b/Tethering/apishim/31/com/android/networkstack/tethering/apishim/api31/BpfCoordinatorShimImpl.java @@ -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 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; + } } diff --git a/Tethering/apishim/common/com/android/networkstack/tethering/apishim/common/BpfCoordinatorShim.java b/Tethering/apishim/common/com/android/networkstack/tethering/apishim/common/BpfCoordinatorShim.java index 026b1c39d0..cb8bcc9126 100644 --- a/Tethering/apishim/common/com/android/networkstack/tethering/apishim/common/BpfCoordinatorShim.java +++ b/Tethering/apishim/common/com/android/networkstack/tethering/apishim/common/BpfCoordinatorShim.java @@ -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(); } diff --git a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java index 89e06da2de..75ab9eccf3 100644 --- a/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java +++ b/Tethering/src/com/android/networkstack/tethering/BpfCoordinator.java @@ -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 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() { diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java index 5d22977858..dd10cc3414 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/BpfCoordinatorTest.java @@ -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 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++) {