dscpPolicy.c - cache result-less lookups as well

It is most definitely worthwhile to cache negative lookups as well!

Test: TreeHugger, atest DscpPolicyTest
Signed-off-by: Maciej Żenczykowski <maze@google.com>
Change-Id: Iab1a57a2611a891642fef0c5897918c16e0ca540

bpf_progs/dscpPolicy.c

@@ -34,8 +34,8 @@
 #include "dscpPolicy.h"
 
 #define ECN_MASK 3
-#define IP4_OFFSET(field, header) (header + offsetof(struct iphdr, field))
-#define UPDATE_TOS(dscp, tos) (dscp << 2) | (tos & ECN_MASK)
+#define IP4_OFFSET(field, header) ((header) + offsetof(struct iphdr, field))
+#define UPDATE_TOS(dscp, tos) ((dscp) << 2) | ((tos) & ECN_MASK)
 
 DEFINE_BPF_MAP_GRW(socket_policy_cache_map, HASH, uint64_t, RuleEntry, CACHE_MAP_SIZE, AID_SYSTEM)
 
@@ -125,6 +125,7 @@ static inline __always_inline void match_policy(struct __sk_buff* skb, bool ipv4
             v6_equal(dst_ip, existing_rule->dst_ip) && skb->ifindex == existing_rule->ifindex &&
         ntohs(sport) == htons(existing_rule->src_port) &&
         ntohs(dport) == htons(existing_rule->dst_port) && protocol == existing_rule->proto) {
+        if (existing_rule->dscp_val < 0) return;
         if (ipv4) {
             uint8_t newTos = UPDATE_TOS(existing_rule->dscp_val, tos);
             bpf_l3_csum_replace(skb, IP4_OFFSET(check, l2_header_size), htons(tos), htons(newTos),
@@ -140,8 +141,8 @@ static inline __always_inline void match_policy(struct __sk_buff* skb, bool ipv4
     }
 
     // Linear scan ipv4_dscp_policies_map since no stored params match skb.
-    int best_score = -1;
-    uint32_t best_match = 0;
+    int best_score = 0;
+    int8_t new_dscp = -1;
 
     for (register uint64_t i = 0; i < MAX_POLICIES; i++) {
         int score = 0;
@@ -189,25 +190,10 @@ static inline __always_inline void match_policy(struct __sk_buff* skb, bool ipv4
         }
 
         if (score > best_score && temp_mask == policy->present_fields) {
-            best_match = i;
             best_score = score;
-        }
-    }
-
-    uint8_t new_dscp = 0;
-    if (best_score > 0) {
-        DscpPolicy* policy;
-        if (ipv4) {
-            policy = bpf_ipv4_dscp_policies_map_lookup_elem(&best_match);
-        } else {
-            policy = bpf_ipv6_dscp_policies_map_lookup_elem(&best_match);
-        }
-
-        if (policy) {
             new_dscp = policy->dscp_val;
         }
-    } else
-        return;
+    }
 
     RuleEntry value = {
         .src_ip = src_ip,
@@ -219,9 +205,11 @@ static inline __always_inline void match_policy(struct __sk_buff* skb, bool ipv4
         .dscp_val = new_dscp,
     };
 
-    // Update map with new policy.
+    // Update cache with found policy.
     bpf_socket_policy_cache_map_update_elem(&cookie, &value, BPF_ANY);
 
+    if (new_dscp < 0) return;
+
     // Need to store bytes after updating map or program will not load.
     if (ipv4) {
         uint8_t new_tos = UPDATE_TOS(new_dscp, tos);

bpf_progs/dscpPolicy.h

@@ -50,7 +50,7 @@ typedef struct {
     __be16 dst_port_start;
     __be16 dst_port_end;
     uint8_t proto;
-    uint8_t dscp_val;
+    int8_t dscp_val;  // -1 none, or 0..63 DSCP value
     uint8_t present_fields;
     uint8_t pad[3];
 } DscpPolicy;
@@ -63,7 +63,7 @@ typedef struct {
     __be16 src_port;
     __be16 dst_port;
     __u8 proto;
-    __u8 dscp_val;
+    __s8 dscp_val;  // -1 none, or 0..63 DSCP value
     __u8 pad[2];
 } RuleEntry;
 STRUCT_SIZE(RuleEntry, 2 * 16 + 1 * 4 + 2 * 2 + 2 * 1 + 2);  // 44

service/src/com/android/server/connectivity/DscpPolicyTracker.java

@@ -185,7 +185,7 @@ public class DscpPolicyTracker {
                         new DscpPolicyValue(policy.getSourceAddress(),
                             policy.getDestinationAddress(), ifIndex,
                             policy.getSourcePort(), policy.getDestinationPortRange(),
-                            (short) policy.getProtocol(), (short) policy.getDscpValue()));
+                            (short) policy.getProtocol(), (byte) policy.getDscpValue()));
             }
 
             // Add v6 policy to mBpfDscpIpv6Policies if source and destination address
@@ -196,7 +196,7 @@ public class DscpPolicyTracker {
                         new DscpPolicyValue(policy.getSourceAddress(),
                                 policy.getDestinationAddress(), ifIndex,
                                 policy.getSourcePort(), policy.getDestinationPortRange(),
-                                (short) policy.getProtocol(), (short) policy.getDscpValue()));
+                                (short) policy.getProtocol(), (byte) policy.getDscpValue()));
             }
 
             ifacePolicies.put(policy.getPolicyId(), addIndex);

service/src/com/android/server/connectivity/DscpPolicyValue.java

@@ -52,8 +52,8 @@ public class DscpPolicyValue extends Struct {
     @Field(order = 6, type = Type.U8)
     public final short proto;
 
-    @Field(order = 7, type = Type.U8)
-    public final short dscp;
+    @Field(order = 7, type = Type.S8)
+    public final byte dscp;
 
     @Field(order = 8, type = Type.U8, padding = 3)
     public final short mask;
@@ -100,7 +100,7 @@ public class DscpPolicyValue extends Struct {
             InetAddress.parseNumericAddress("::").getAddress();
 
     private short makeMask(final byte[] src46, final byte[] dst46, final int srcPort,
-            final int dstPortStart, final short proto, final short dscp) {
+            final int dstPortStart, final short proto, final byte dscp) {
         short mask = 0;
         if (src46 != EMPTY_ADDRESS_FIELD) {
             mask |= SRC_IP_MASK;
@@ -122,7 +122,7 @@ public class DscpPolicyValue extends Struct {
 
     private DscpPolicyValue(final InetAddress src46, final InetAddress dst46, final long ifIndex,
             final int srcPort, final int dstPortStart, final int dstPortEnd, final short proto,
-            final short dscp) {
+            final byte dscp) {
         this.src46 = toAddressField(src46);
         this.dst46 = toAddressField(dst46);
         this.ifIndex = ifIndex;
@@ -142,7 +142,7 @@ public class DscpPolicyValue extends Struct {
 
     public DscpPolicyValue(final InetAddress src46, final InetAddress dst46, final long ifIndex,
             final int srcPort, final Range<Integer> dstPort, final short proto,
-            final short dscp) {
+            final byte dscp) {
         this(src46, dst46, ifIndex, srcPort, dstPort != null ? dstPort.getLower() : -1,
                 dstPort != null ? dstPort.getUpper() : -1, proto, dscp);
     }
@@ -150,7 +150,7 @@ public class DscpPolicyValue extends Struct {
     public static final DscpPolicyValue NONE = new DscpPolicyValue(
             null /* src46 */, null /* dst46 */, 0 /* ifIndex */, -1 /* srcPort */,
             -1 /* dstPortStart */, -1 /* dstPortEnd */, (short) -1 /* proto */,
-            (short) 0 /* dscp */);
+            (byte) -1 /* dscp */);
 
     @Override
     public String toString() {