[Cake] cake srchost/dsthost stopped working?

[Cake] cake srchost/dsthost stopped working?

[Pete Heist]

It seems like Cake's srchost and dsthost keywords may have stopped
working some time between kernel 5.4 and 5.10.

When the bug occurs, there seems to not be fairness between hosts, but
rather fairness between flows. It reproduces on any 5.10 series kernel
I've tried, and does not reproduce on any 5.4 series or lower. Here's a
standalone script to reproduce it with netns, and some sample output:

https://www.heistp.net/downloads/cake-hostfair/

It creates competition from one IP to two IPs, and from two IPs to one
IP, using the src/dsthost keywords as appropriate. It also tests
fq_codel with a tc-flow filter, and cake *dual*-(src|dst)host, which
are both unaffected by this.

Any ideas?

Pete

Re: [Cake] cake srchost/dsthost stopped working?

[Pete Heist]

One more tip, reverting this commit seems to fix it:

https://github.com/torvalds/linux/commit/b0c19ed6088ab41dd2a727b60594b7297c15d6ce

Pete

On Tue, 2021-08-03 at 18:03 +0200, Pete Heist wrote:
> It seems like Cake's srchost and dsthost keywords may have stopped
> working some time between kernel 5.4 and 5.10.
> 
> When the bug occurs, there seems to not be fairness between hosts, but
> rather fairness between flows. It reproduces on any 5.10 series kernel
> I've tried, and does not reproduce on any 5.4 series or lower. Here's a
> standalone script to reproduce it with netns, and some sample output:
> 
> https://www.heistp.net/downloads/cake-hostfair/
> 
> It creates competition from one IP to two IPs, and from two IPs to one
> IP, using the src/dsthost keywords as appropriate. It also tests
> fq_codel with a tc-flow filter, and cake *dual*-(src|dst)host, which
> are both unaffected by this.
> 
> Any ideas?
> 
> Pete
> 
> 

Re: [Cake] cake srchost/dsthost stopped working?

[Toke Høiland-Jørgensen]

Pete Heist <pete@heistp.net> writes:

> One more tip, reverting this commit seems to fix it:
>
> https://github.com/torvalds/linux/commit/b0c19ed6088ab41dd2a727b60594b7297c15d6ce

Ah, I think I see what the problem is; could you please try the patch
below?

-Toke

diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
index 951542843cab..a83c4d4326da 100644
--- a/net/sched/sch_cake.c
+++ b/net/sched/sch_cake.c
@@ -720,7 +720,7 @@ static u32 cake_hash(struct cake_tin_data *q, const struct sk_buff *skb,
 skip_hash:
        if (flow_override)
                flow_hash = flow_override - 1;
-       else if (use_skbhash)
+       else if (use_skbhash && flow_mode & CAKE_FLOW_FLOWS)
                flow_hash = skb->hash;
        if (host_override) {
                dsthost_hash = host_override - 1;

Re: [Cake] cake srchost/dsthost stopped working?

[Jonathan Morton]

On Wed, 4 Aug 2021 at 14:14, Toke Høiland-Jørgensen via Cake
<cake@lists.bufferbloat.net> wrote:
>
> Pete Heist <pete@heistp.net> writes:
>
> > One more tip, reverting this commit seems to fix it:
> >
> > https://github.com/torvalds/linux/commit/b0c19ed6088ab41dd2a727b60594b7297c15d6ce
>
> Ah, I think I see what the problem is; could you please try the patch
> below?
>
> -Toke
>
> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index 951542843cab..a83c4d4326da 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -720,7 +720,7 @@ static u32 cake_hash(struct cake_tin_data *q, const struct sk_buff *skb,
>  skip_hash:
>         if (flow_override)
>                 flow_hash = flow_override - 1;
> -       else if (use_skbhash)
> +       else if (use_skbhash && flow_mode & CAKE_FLOW_FLOWS)
>                 flow_hash = skb->hash;
>         if (host_override) {
>                 dsthost_hash = host_override - 1;

Good catch - I was going to have to wade in and remind myself how this
lump of code worked.  But shouldn't the masking operation be in
brackets, to make the precedence explicit?

 - Jonathan Morton

Re: [Cake] cake srchost/dsthost stopped working?

[Toke Høiland-Jørgensen]

Jonathan Morton <chromatix99@gmail.com> writes:

> On Wed, 4 Aug 2021 at 14:14, Toke Høiland-Jørgensen via Cake
> <cake@lists.bufferbloat.net> wrote:
>>
>> Pete Heist <pete@heistp.net> writes:
>>
>> > One more tip, reverting this commit seems to fix it:
>> >
>> > https://github.com/torvalds/linux/commit/b0c19ed6088ab41dd2a727b60594b7297c15d6ce
>>
>> Ah, I think I see what the problem is; could you please try the patch
>> below?
>>
>> -Toke
>>
>> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
>> index 951542843cab..a83c4d4326da 100644
>> --- a/net/sched/sch_cake.c
>> +++ b/net/sched/sch_cake.c
>> @@ -720,7 +720,7 @@ static u32 cake_hash(struct cake_tin_data *q, const struct sk_buff *skb,
>>  skip_hash:
>>         if (flow_override)
>>                 flow_hash = flow_override - 1;
>> -       else if (use_skbhash)
>> +       else if (use_skbhash && flow_mode & CAKE_FLOW_FLOWS)
>>                 flow_hash = skb->hash;
>>         if (host_override) {
>>                 dsthost_hash = host_override - 1;
>
> Good catch - I was going to have to wade in and remind myself how this
> lump of code worked.  But shouldn't the masking operation be in
> brackets, to make the precedence explicit?

Well, seeing as I introduced the bug, I think it's only fair that I fix
it as well ;)

I don't mind adding parenthesis; can do so when submitting a patch,
after Pete confirms that this fixes the issue...

-Toke

Re: [Cake] cake srchost/dsthost stopped working?

[Pete Heist]

Great, that seems to fix it. :) For sanity, I confirmed that I can
apply/unapply the patch to fix/break it, so all seems well.

Is there anything that you'd want to make sure still works after the
patch? I don't have any official regression tests to run, but with this
setup ready I should be able to test something easily if needed...

Pete

On Wed, 2021-08-04 at 13:14 +0200, Toke Høiland-Jørgensen wrote:
> Pete Heist <pete@heistp.net> writes:
> 
> > One more tip, reverting this commit seems to fix it:
> > 
> > https://github.com/torvalds/linux/commit/b0c19ed6088ab41dd2a727b60594b7297c15d6ce
> 
> Ah, I think I see what the problem is; could you please try the patch
> below?
> 
> -Toke
> 
> diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> index 951542843cab..a83c4d4326da 100644
> --- a/net/sched/sch_cake.c
> +++ b/net/sched/sch_cake.c
> @@ -720,7 +720,7 @@ static u32 cake_hash(struct cake_tin_data *q,
> const struct sk_buff *skb,
>  skip_hash:
>         if (flow_override)
>                 flow_hash = flow_override - 1;
> -       else if (use_skbhash)
> +       else if (use_skbhash && flow_mode & CAKE_FLOW_FLOWS)
>                 flow_hash = skb->hash;
>         if (host_override) {
>                 dsthost_hash = host_override - 1;

Re: [Cake] cake srchost/dsthost stopped working?

[Pete Heist]

I ended up testing each of the keywords, with two src IPs then two dst
IPs, and I think everything makes sense:

https://www.heistp.net/downloads/cake-hostfair/cake-hostfair-patched.txt

I'm only not sure if we can tell from this test if triple-isolate is
doing what it should...

Pete

On Wed, 2021-08-04 at 22:49 +0200, Pete Heist wrote:
> Great, that seems to fix it. :) For sanity, I confirmed that I can
> apply/unapply the patch to fix/break it, so all seems well.
> 
> Is there anything that you'd want to make sure still works after the
> patch? I don't have any official regression tests to run, but with this
> setup ready I should be able to test something easily if needed...
> 
> Pete
> 
> On Wed, 2021-08-04 at 13:14 +0200, Toke Høiland-Jørgensen wrote:
> > Pete Heist <pete@heistp.net> writes:
> > 
> > > One more tip, reverting this commit seems to fix it:
> > > 
> > > https://github.com/torvalds/linux/commit/b0c19ed6088ab41dd2a727b60594b7297c15d6ce
> > 
> > Ah, I think I see what the problem is; could you please try the patch
> > below?
> > 
> > -Toke
> > 
> > diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> > index 951542843cab..a83c4d4326da 100644
> > --- a/net/sched/sch_cake.c
> > +++ b/net/sched/sch_cake.c
> > @@ -720,7 +720,7 @@ static u32 cake_hash(struct cake_tin_data *q,
> > const struct sk_buff *skb,
> >  skip_hash:
> >         if (flow_override)
> >                 flow_hash = flow_override - 1;
> > -       else if (use_skbhash)
> > +       else if (use_skbhash && flow_mode & CAKE_FLOW_FLOWS)
> >                 flow_hash = skb->hash;
> >         if (host_override) {
> >                 dsthost_hash = host_override - 1;
> 
> 

Re: [Cake] cake srchost/dsthost stopped working?

[Jonathan Morton]

Those results look good to me as well.  The difference between the
dual modes and the pure host modes is visible in terms of looser
fairness in the latter case.

Don't worry too much about triple-isolate, as it is significantly more
complicated to test comprehensively.  I'm satisfied that if the dual
modes are working, we probably haven't broken triple-isolate either.

 - Jonathan Morton

Re: [Cake] cake srchost/dsthost stopped working?

[Pete Heist]

As for triple-isolate, I added a test Jon suggested:

"I would recommend a W configuration of flows

one of the three hosts sends to or from both of the two hosts, while
the other two send to or from only one each

this should result in the two hosts getting equal throughput each, and
equal throughput in each of their two flows"

This is the last test in the output, and I think the behavior looks
correct for both the unpatched and patched versions:

https://www.heistp.net/downloads/cake-hostfair/cake-hostfair-unpatched.txt

https://www.heistp.net/downloads/cake-hostfair/cake-hostfair-patched.txt

Thanks guys, and that, I believe, is finally it for now... :)

Pete

On Thu, 2021-08-05 at 00:52 +0200, Pete Heist wrote:
> I ended up testing each of the keywords, with two src IPs then two dst
> IPs, and I think everything makes sense:
> 
> https://www.heistp.net/downloads/cake-hostfair/cake-hostfair-patched.txt
> 
> I'm only not sure if we can tell from this test if triple-isolate is
> doing what it should...
> 
> Pete
> 
> On Wed, 2021-08-04 at 22:49 +0200, Pete Heist wrote:
> > Great, that seems to fix it. :) For sanity, I confirmed that I can
> > apply/unapply the patch to fix/break it, so all seems well.
> > 
> > Is there anything that you'd want to make sure still works after the
> > patch? I don't have any official regression tests to run, but with
> > this
> > setup ready I should be able to test something easily if needed...
> > 
> > Pete
> > 
> > On Wed, 2021-08-04 at 13:14 +0200, Toke Høiland-Jørgensen wrote:
> > > Pete Heist <pete@heistp.net> writes:
> > > 
> > > > One more tip, reverting this commit seems to fix it:
> > > > 
> > > > https://github.com/torvalds/linux/commit/b0c19ed6088ab41dd2a727b60594b7297c15d6ce
> > > 
> > > Ah, I think I see what the problem is; could you please try the
> > > patch
> > > below?
> > > 
> > > -Toke
> > > 
> > > diff --git a/net/sched/sch_cake.c b/net/sched/sch_cake.c
> > > index 951542843cab..a83c4d4326da 100644
> > > --- a/net/sched/sch_cake.c
> > > +++ b/net/sched/sch_cake.c
> > > @@ -720,7 +720,7 @@ static u32 cake_hash(struct cake_tin_data *q,
> > > const struct sk_buff *skb,
> > >  skip_hash:
> > >         if (flow_override)
> > >                 flow_hash = flow_override - 1;
> > > -       else if (use_skbhash)
> > > +       else if (use_skbhash && flow_mode & CAKE_FLOW_FLOWS)
> > >                 flow_hash = skb->hash;
> > >         if (host_override) {
> > >                 dsthost_hash = host_override - 1;
> > 
> > 
> 
> 

Re: [Cake] cake srchost/dsthost stopped working?

[Toke Høiland-Jørgensen]

Pete Heist <pete@heistp.net> writes:

> As for triple-isolate, I added a test Jon suggested:
>
> "I would recommend a W configuration of flows
>
> one of the three hosts sends to or from both of the two hosts, while
> the other two send to or from only one each
>
> this should result in the two hosts getting equal throughput each, and
> equal throughput in each of their two flows"
>
> This is the last test in the output, and I think the behavior looks
> correct for both the unpatched and patched versions:
>
> https://www.heistp.net/downloads/cake-hostfair/cake-hostfair-unpatched.txt
>
> https://www.heistp.net/downloads/cake-hostfair/cake-hostfair-patched.txt
>
> Thanks guys, and that, I believe, is finally it for now... :)

Awesome, thank you both! I'll send a proper patch upstream :)

-Toke

Re: [Cake] cake srchost/dsthost stopped working?

[Pete Heist]

Fyi I threw the test script up in a repo, so in case we need to test
the flow isolation keywords again we can just run that:

https://github.com/heistp/cake-flowiso

Pete

On Thu, 2021-08-05 at 16:56 +0200, Toke Høiland-Jørgensen wrote:
> Pete Heist <pete@heistp.net> writes:
> 
> > As for triple-isolate, I added a test Jon suggested:
> > 
> > "I would recommend a W configuration of flows
> > 
> > one of the three hosts sends to or from both of the two hosts, while
> > the other two send to or from only one each
> > 
> > this should result in the two hosts getting equal throughput each,
> > and
> > equal throughput in each of their two flows"
> > 
> > This is the last test in the output, and I think the behavior looks
> > correct for both the unpatched and patched versions:
> > 
> > https://www.heistp.net/downloads/cake-hostfair/cake-hostfair-unpatched.txt
> > 
> > https://www.heistp.net/downloads/cake-hostfair/cake-hostfair-patched.txt
> > 
> > Thanks guys, and that, I believe, is finally it for now... :)
> 
> Awesome, thank you both! I'll send a proper patch upstream :)
> 
> -Toke

Re: [Cake] cake srchost/dsthost stopped working?

[Stephen Hemminger]

On Wed, 04 Aug 2021 13:14:30 +0200
Toke Høiland-Jørgensen via Cake <cake@lists.bufferbloat.net> wrote:

> +       else if (use_skbhash && flow_mode & CAKE_FLOW_FLOWS)

I think some compilers and static checkers are going to give a warning unless
you add a parenthesis in this statement.