[Cake] Possible conntrack lookup improvements

[Cake] Possible conntrack lookup improvements

[Kevin Darbyshire-Bryant]

[-- 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)

Re: [Cake] Possible conntrack lookup improvements

[Jonathan Morton]

> On 3 May, 2019, at 4:55 pm, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
> 
> 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… 

Looks like sound logic, as long as it does actually work.  It could be a useful speedup for those small CPE devices which need NAT and host-fairness working.

 - Jonathan Morton

Re: [Cake] Possible conntrack lookup improvements

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

> 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 :-)

Seems reasonable. But please fold these two patches into one; changing
everything, then immediately changing it again does not help
readability... And the explanation makes a lot more sense if you just
change the whole thing in one patch :)

-Toke

Re: [Cake] Possible conntrack lookup improvements

[Kevin Darbyshire-Bryant]

> On 3 May 2019, at 15:22, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> 
> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
> 
>> 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 :-)
> 
> Seems reasonable. But please fold these two patches into one; changing
> everything, then immediately changing it again does not help
> readability... And the explanation makes a lot more sense if you just
> change the whole thing in one patch :)
> 
> -Toke

Yeah, when I do the PR after testing confirms I haven’t totally screwed up host fairness in the process I’ll of course squash them together :-) The 1st patch is a no brainer, the second should be a no brainer but it needs more testing than I have given it.

I went down this path as a result of my act_ctinfo work which in the latest version is able to restore DSCP & skb->marks from conntrack.  I had an idea to restore the ct info as well, so CAKE didn’t have to do its ‘look harder’ lookup.  Then I noticed how cake sort of does the harder lookup backwards.


Cheers,

Kevin D-B

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

Re: [Cake] Possible conntrack lookup improvements

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

>> On 3 May 2019, at 15:22, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> 
>> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
>> 
>>> 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 :-)
>> 
>> Seems reasonable. But please fold these two patches into one; changing
>> everything, then immediately changing it again does not help
>> readability... And the explanation makes a lot more sense if you just
>> change the whole thing in one patch :)
>> 
>> -Toke
>
> Yeah, when I do the PR after testing confirms I haven’t totally
> screwed up host fairness in the process I’ll of course squash them
> together :-)

Cool :)

-Toke

Re: [Cake] Possible conntrack lookup improvements

[Kevin Darbyshire-Bryant]

> On 3 May 2019, at 15:16, Jonathan Morton <chromatix99@gmail.com> wrote:
> 
>> On 3 May, 2019, at 4:55 pm, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
>> 
>> 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… 
> 
> Looks like sound logic, as long as it does actually work.  It could be a useful speedup for those small CPE devices which need NAT and host-fairness working.

It’s interesting you bring that up - are we sure that ingress host NAT fairness works in the upstream kernel version of CAKE anyway?  I’m looking at cake_update_flowkeys(…) and thinking half of it is missing?

> 
> - Jonathan Morton


Cheers,

Kevin D-B

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

Re: [Cake] Possible conntrack lookup improvements

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:

>> On 3 May 2019, at 15:16, Jonathan Morton <chromatix99@gmail.com> wrote:
>> 
>>> On 3 May, 2019, at 4:55 pm, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
>>> 
>>> 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… 
>> 
>> Looks like sound logic, as long as it does actually work.  It could be a useful speedup for those small CPE devices which need NAT and host-fairness working.
>
> It’s interesting you bring that up - are we sure that ingress host NAT
> fairness works in the upstream kernel version of CAKE anyway?  I’m
> looking at cake_update_flowkeys(…) and thinking half of it is missing?

No, it's just moved into nf_conntrack_get_tuple_skb(); this was part of
the work we did to ensure sch_cake could load without a dependency on
the conntrack module...

It does carry over the 'nf_ct_tuple(ct, !hash->tuple.dst.dir);' and the
subsequent reversion, though, but I think the logic fits what's in the
out-of-tree version?

-Toke

Re: [Cake] Possible conntrack lookup improvements

[Kevin Darbyshire-Bryant]

> On 3 May 2019, at 20:13, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> 
> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
> 
>>> On 3 May 2019, at 15:16, Jonathan Morton <chromatix99@gmail.com> wrote:
>>> 
>>>> On 3 May, 2019, at 4:55 pm, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
>>>> 
>>>> 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… 
>>> 
>>> Looks like sound logic, as long as it does actually work.  It could be a useful speedup for those small CPE devices which need NAT and host-fairness working.
>> 
>> It’s interesting you bring that up - are we sure that ingress host NAT
>> fairness works in the upstream kernel version of CAKE anyway?  I’m
>> looking at cake_update_flowkeys(…) and thinking half of it is missing?
> 
> No, it's just moved into nf_conntrack_get_tuple_skb(); this was part of
> the work we did to ensure sch_cake could load without a dependency on
> the conntrack module...
> 
> It does carry over the 'nf_ct_tuple(ct, !hash->tuple.dst.dir);' and the
> subsequent reversion, though, but I think the logic fits what's in the
> out-of-tree version?
> 
> -Toke

Ahh! yes I see, thanks. - elixir or my ability to operate elixir was failing earlier.

Yes and agree the logic follows the out-of-tree…and I can see how my change to it would be applied, assuming it does actually work.


Cheers,

Kevin D-B

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

Re: [Cake] Possible conntrack lookup improvements

[Kevin Darbyshire-Bryant]

> On 3 May 2019, at 20:23, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
> 
> 
> 
>> On 3 May 2019, at 20:13, Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> 
>> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
>> 
>>>> On 3 May 2019, at 15:16, Jonathan Morton <chromatix99@gmail.com> wrote:
>>>> 
>>>>> On 3 May, 2019, at 4:55 pm, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
>>>>> 
>>>>> 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… 
>>>> 
>>>> Looks like sound logic, as long as it does actually work.  It could be a useful speedup for those small CPE devices which need NAT and host-fairness working.
>>> 
>>> It’s interesting you bring that up - are we sure that ingress host NAT
>>> fairness works in the upstream kernel version of CAKE anyway?  I’m
>>> looking at cake_update_flowkeys(…) and thinking half of it is missing?
>> 
>> No, it's just moved into nf_conntrack_get_tuple_skb(); this was part of
>> the work we did to ensure sch_cake could load without a dependency on
>> the conntrack module...
>> 
>> It does carry over the 'nf_ct_tuple(ct, !hash->tuple.dst.dir);' and the
>> subsequent reversion, though, but I think the logic fits what's in the
>> out-of-tree version?
>> 
>> -Toke
> 
> Ahh! yes I see, thanks. - elixir or my ability to operate elixir was failing earlier.
> 
> Yes and agree the logic follows the out-of-tree…and I can see how my change to it would be applied, assuming it does actually work.
> 

My testing strategy was flawed, the 2nd patch does not work correctly.  Will think again.

> 


Cheers,

Kevin D-B

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