[Cake] Possible conntrack lookup improvements

[Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk>]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

Hi Everyone,

I’ve been working on act_ctinfo toward getting that upstream and it is getting closer.  Since that module along with act_connmark does its own conntrack lookups I’ve been looking at what they do and what we do in cake.

Two patches attached - one is a simple variable elimination with no functional change.  The second changes/simplifies the conntrack tuple lookup & usage.  I’ve had a play and I don’t think I’ve broken any of the host fairness BUT it could do with some more testing, that’s where you come in… probably Pete & George :-)



Cheers,

Kevin D-B

gpg: 012C ACB2 28C6 C53E 9775  9123 B3A2 389B 9DE2 334A

[-- Attachment #2: 0001-eliminate-variable-in-conntrack-lookup.patch --]
[-- Type: application/octet-stream, Size: 2800 bytes --]

From 409793741d993123ccd321344a2fbf41693ff0e0 Mon Sep 17 00:00:00 2001
From: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
Date: Fri, 3 May 2019 08:33:37 +0100
Subject: [PATCH 1/2] eliminate variable in conntrack lookup

Eliminate boolean as existing null/none null pointer can be used for
equivalent functionality.

Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
---
 sch_cake.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/sch_cake.c b/sch_cake.c
index 253cb63..f0d651e 100644
--- a/sch_cake.c
+++ b/sch_cake.c
@@ -624,10 +624,10 @@ static bool cobalt_should_drop(struct cobalt_vars *vars,
 static void cake_update_flowkeys(struct flow_keys *keys,
 				 const struct sk_buff *skb)
 {
+	const struct nf_conntrack_tuple_hash *hash = NULL;
 	const struct nf_conntrack_tuple *tuple;
 	enum ip_conntrack_info ctinfo;
 	struct nf_conn *ct;
-	bool rev = false;
 
 	if (tc_skb_protocol(skb) != htons(ETH_P_IP))
 		return;
@@ -636,7 +636,6 @@ static void cake_update_flowkeys(struct flow_keys *keys,
 	if (ct) {
 		tuple = nf_ct_tuple(ct, CTINFO2DIR(ctinfo));
 	} else {
-		const struct nf_conntrack_tuple_hash *hash;
 		struct nf_conntrack_tuple srctuple;
 
 #if KERNEL_VERSION(4, 4, 0) > LINUX_VERSION_CODE
@@ -661,31 +660,30 @@ static void cake_update_flowkeys(struct flow_keys *keys,
 		if (!hash)
 			return;
 
-		rev = true;
 		ct = nf_ct_tuplehash_to_ctrack(hash);
 		tuple = nf_ct_tuple(ct, !hash->tuple.dst.dir);
 	}
 
 #if KERNEL_VERSION(4, 2, 0) > LINUX_VERSION_CODE
-	keys->src = rev ? tuple->dst.u3.ip : tuple->src.u3.ip;
-	keys->dst = rev ? tuple->src.u3.ip : tuple->dst.u3.ip;
+	keys->src = hash ? tuple->dst.u3.ip : tuple->src.u3.ip;
+	keys->dst = hash ? tuple->src.u3.ip : tuple->dst.u3.ip;
 #else
-	keys->addrs.v4addrs.src = rev ? tuple->dst.u3.ip : tuple->src.u3.ip;
-	keys->addrs.v4addrs.dst = rev ? tuple->src.u3.ip : tuple->dst.u3.ip;
+	keys->addrs.v4addrs.src = hash ? tuple->dst.u3.ip : tuple->src.u3.ip;
+	keys->addrs.v4addrs.dst = hash ? tuple->src.u3.ip : tuple->dst.u3.ip;
 #endif
 
 #if KERNEL_VERSION(4, 2, 0) > LINUX_VERSION_CODE
 	if (keys->ports) {
-		keys->port16[0] = rev ? tuple->dst.u.all : tuple->src.u.all;
-		keys->port16[1] = rev ? tuple->src.u.all : tuple->dst.u.all;
+		keys->port16[0] = hash ? tuple->dst.u.all : tuple->src.u.all;
+		keys->port16[1] = hash ? tuple->src.u.all : tuple->dst.u.all;
 	}
 #else
 	if (keys->ports.ports) {
-		keys->ports.src = rev ? tuple->dst.u.all : tuple->src.u.all;
-		keys->ports.dst = rev ? tuple->src.u.all : tuple->dst.u.all;
+		keys->ports.src = hash ? tuple->dst.u.all : tuple->src.u.all;
+		keys->ports.dst = hash ? tuple->src.u.all : tuple->dst.u.all;
 	}
 #endif
-	if (rev)
+	if (hash)
 		nf_ct_put(ct);
 }
 #else
-- 
2.20.1 (Apple Git-117)


[-- Attachment #3: 0002-refactor-conntrack-lookup.patch --]
[-- Type: application/octet-stream, Size: 2782 bytes --]

From f19c223fb35b958c37241d0ebf70bacec056b306 Mon Sep 17 00:00:00 2001
From: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
Date: Fri, 3 May 2019 12:41:58 +0100
Subject: [PATCH 2/2] refactor conntrack lookup

In the original code we use the conntrack info contained in the skb to
lookup the conntrack entry for 'internal' ip addresses.  For egress this
works fine as the skb conntrack entry will be filled in.

Ingress is harder in that the skb ct details aren't filled in, so we
have to look ourselves into the the conntrack table deep abyss.  This
lookup was referred to by me as 'the reverse', which I think led to the
original 'rev' boolean, which isn't really required.

The 'rev' boolean also controlled which 'side' of the ct tuples we
looked at to obtain IP addresses.

The harder tuple lookup used tuple = nf_ct_tuple(ct, !hash->tuple.dst.dir)
which if looked at carefully inverts the direction of the ct tuple
lookup.  This got me thinking "why are we inverting the lookup to then
re-invert it later with our 'rev' boolean based address selection.

We can eliminate the 'rev' boolean using hash !NULL as the equivalent,
also if we don't invert our ct tuple lookup we can eliminate the address
swapping.

Signed-off-by: Kevin Darbyshire-Bryant <ldir@darbyshire-bryant.me.uk>
---
 sch_cake.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/sch_cake.c b/sch_cake.c
index f0d651e..12c641a 100644
--- a/sch_cake.c
+++ b/sch_cake.c
@@ -661,26 +661,26 @@ static void cake_update_flowkeys(struct flow_keys *keys,
 			return;
 
 		ct = nf_ct_tuplehash_to_ctrack(hash);
-		tuple = nf_ct_tuple(ct, !hash->tuple.dst.dir);
+		tuple = nf_ct_tuple(ct, hash->tuple.dst.dir);
 	}
 
 #if KERNEL_VERSION(4, 2, 0) > LINUX_VERSION_CODE
-	keys->src = hash ? tuple->dst.u3.ip : tuple->src.u3.ip;
-	keys->dst = hash ? tuple->src.u3.ip : tuple->dst.u3.ip;
+	keys->src = tuple->src.u3.ip;
+	keys->dst = tuple->dst.u3.ip;
 #else
-	keys->addrs.v4addrs.src = hash ? tuple->dst.u3.ip : tuple->src.u3.ip;
-	keys->addrs.v4addrs.dst = hash ? tuple->src.u3.ip : tuple->dst.u3.ip;
+	keys->addrs.v4addrs.src = tuple->src.u3.ip;
+	keys->addrs.v4addrs.dst = tuple->dst.u3.ip;
 #endif
 
 #if KERNEL_VERSION(4, 2, 0) > LINUX_VERSION_CODE
 	if (keys->ports) {
-		keys->port16[0] = hash ? tuple->dst.u.all : tuple->src.u.all;
-		keys->port16[1] = hash ? tuple->src.u.all : tuple->dst.u.all;
+		keys->port16[0] = tuple->src.u.all;
+		keys->port16[1] = tuple->dst.u.all;
 	}
 #else
 	if (keys->ports.ports) {
-		keys->ports.src = hash ? tuple->dst.u.all : tuple->src.u.all;
-		keys->ports.dst = hash ? tuple->src.u.all : tuple->dst.u.all;
+		keys->ports.src = tuple->src.u.all;
+		keys->ports.dst = tuple->dst.u.all;
 	}
 #endif
 	if (hash)
-- 
2.20.1 (Apple Git-117)