[Cake] allocate_src allocate_dst

[Cake] allocate_src allocate_dst

[Dave Taht]

there is no place in the current code base where these are not both
true or both false. Thus redundant.

?

-- 

Dave Täht
CEO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-669-226-2619

Re: [Cake] allocate_src allocate_dst

[Kevin Darbyshire-Bryant]

> On 25 Nov 2017, at 03:59, Dave Taht <dave.taht@gmail.com> wrote:
> 
> there is no place in the current code base where these are not both
> true or both false. Thus redundant.
> 
> ?

Yes, interesting that.  Been there since introduced in 5a6da2ba Add triple-isolation support.  *EXPERIMENTAL*

Have done a PR https://github.com/dtaht/sch_cake/pull/72 with a minor cleanup for that - been running on my router with no explosions :-)

> 
> -- 
> 
> Dave Täht
> CEO, TekLibre, LLC
> http://www.teklibre.com
> Tel: 1-669-226-2619
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] allocate_src allocate_dst

[Jonathan Morton]

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

I think I originally intended to make them individually conditional on the
src/dest host flags, but that idea evidently got forgotten.

- Jonathan Morton

[-- Attachment #2: Type: text/html, Size: 193 bytes --]

Re: [Cake] allocate_src allocate_dst

[Pete Heist]

> On Nov 25, 2017, at 3:08 PM, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
> 
>> On 25 Nov 2017, at 03:59, Dave Taht <dave.taht@gmail.com> wrote:
>> 
>> there is no place in the current code base where these are not both
>> true or both false. Thus redundant.
>> 
>> ?
> 
> Yes, interesting that.  Been there since introduced in 5a6da2ba Add triple-isolation support.  *EXPERIMENTAL*
> 
> Have done a PR https://github.com/dtaht/sch_cake/pull/72 with a minor cleanup for that - been running on my router with no explosions :-)

Woah, nice find. I’d been working through that function (looking at setting srchost_hash and dsthost_hash to a packet mark value for custom isolation) and between the gotos and ifdefs for kernel version support, it takes some work to look at it.

Could this function be refactored a bit for readability? I’d offer to do it, but this is an important piece of code with little room for error and more ways to plant hard to find problems, better played with by someone who’s more familiar with it...

Re: [Cake] allocate_src allocate_dst

[Jonathan Morton]

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

Part of making an upstream patch involves collapsing those ifdefs to match
the current kernel version.  That by itself should considerably simplify
that function.

- Jonathan Morton

[-- Attachment #2: Type: text/html, Size: 225 bytes --]

Re: [Cake] allocate_src allocate_dst

[Kevin Darbyshire-Bryant]

> On 25 Nov 2017, at 15:19, Jonathan Morton <chromatix99@gmail.com> wrote:
> 
> I think I originally intended to make them individually conditional on the src/dest host flags, but that idea evidently got forgotten.
> 
> - Jonathan Morton

Ahh, is this https://github.com/ldir-EDB0/sch_cake/commit/53953e073e26c45ac67dd5677340a74f75a6e1ae more what you were thinking Jonathan?

It compiles… family using network so not yet dared run it ;-)

Re: [Cake] allocate_src allocate_dst

[Jonathan Morton]

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

Not quite.  The "dual" enums have more than one bit set, so you have to
test them for equality.

- Jonathan Morton

[-- Attachment #2: Type: text/html, Size: 168 bytes --]

Re: [Cake] allocate_src allocate_dst

[Kevin Darbyshire-Bryant]

> On 25 Nov 2017, at 16:41, Jonathan Morton <chromatix99@gmail.com> wrote:
> 
> Not quite.  The "dual" enums have more than one bit set, so you have to test them for equality.
> 
> - Jonathan Morton

Oh dammit, yes they are dual bit flags, rats.  good catch.  v2 shortly he he :-) 

Re: [Cake] allocate_src allocate_dst

[Dave Taht]

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

I have a for_upstream_4.16 branch which you might want to pull and
take a look at.

It no longer compiles out of tree, so you can apply it against the
current net-next kernel, with patchset for that is attached. I'm
compiling this version now (which takes a while), and going out to do
laundry.

[-- Attachment #2: upstream.tgz --]
[-- Type: application/x-gzip, Size: 23409 bytes --]

Re: [Cake] allocate_src allocate_dst

[Dave Taht]

On Sat, Nov 25, 2017 at 8:43 AM, Kevin Darbyshire-Bryant
<kevin@darbyshire-bryant.me.uk> wrote:
>
>
>> On 25 Nov 2017, at 16:41, Jonathan Morton <chromatix99@gmail.com> wrote:
>>
>> Not quite.  The "dual" enums have more than one bit set, so you have to test them for equality.
>>
>> - Jonathan Morton
>
> Oh dammit, yes they are dual bit flags, rats.  good catch.  v2 shortly he he :-)

Grump.




-- 

Dave Täht
CEO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-669-226-2619

Re: [Cake] allocate_src allocate_dst

[Dave Taht]

On Sat, Nov 25, 2017 at 8:28 AM, Pete Heist <peteheist@gmail.com> wrote:
>
>> On Nov 25, 2017, at 3:08 PM, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
>>
>>> On 25 Nov 2017, at 03:59, Dave Taht <dave.taht@gmail.com> wrote:
>>>
>>> there is no place in the current code base where these are not both
>>> true or both false. Thus redundant.
>>>
>>> ?
>>
>> Yes, interesting that.  Been there since introduced in 5a6da2ba Add triple-isolation support.  *EXPERIMENTAL*
>>
>> Have done a PR https://github.com/dtaht/sch_cake/pull/72 with a minor cleanup for that - been running on my router with no explosions :-)
>
> Woah, nice find. I’d been working through that function (looking at setting srchost_hash and dsthost_hash to a packet mark value for custom isolation) and between the gotos and ifdefs for kernel version support, it takes some work to look at it.
>
> Could this function be refactored a bit for readability? I’d offer to do it, but this is an important piece of code with little room for error and more ways to plant hard to find problems, better played with by someone who’s more familiar with it...

I am unfond of the gotos as well. But I'm gonna go do laundry. :) If
someone else beats me to it?



-- 

Dave Täht
CEO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-669-226-2619

Re: [Cake] allocate_src allocate_dst

[Dave Taht]

Please ignore that patchset. I'll regenerate it later.

On Sat, Nov 25, 2017 at 8:45 AM, Dave Taht <dave.taht@gmail.com> wrote:
> I have a for_upstream_4.16 branch which you might want to pull and
> take a look at.
>
> It no longer compiles out of tree, so you can apply it against the
> current net-next kernel, with patchset for that is attached. I'm
> compiling this version now (which takes a while), and going out to do
> laundry.



-- 

Dave Täht
CEO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-669-226-2619

Re: [Cake] allocate_src allocate_dst

[Dave Taht]

On Sat, Nov 25, 2017 at 8:52 AM, Dave Taht <dave.taht@gmail.com> wrote:
> Please ignore that patchset. I'll regenerate it later.

The tarball, anyway. (I'd generated it before I ripped out perturb)

The branch looks ok to my bleeding eyes thus far.

>
> On Sat, Nov 25, 2017 at 8:45 AM, Dave Taht <dave.taht@gmail.com> wrote:
>> I have a for_upstream_4.16 branch which you might want to pull and
>> take a look at.
>>
>> It no longer compiles out of tree, so you can apply it against the
>> current net-next kernel, with patchset for that is attached. I'm
>> compiling this version now (which takes a while), and going out to do
>> laundry.
>
>
>
> --
>
> Dave Täht
> CEO, TekLibre, LLC
> http://www.teklibre.com
> Tel: 1-669-226-2619



-- 

Dave Täht
CEO, TekLibre, LLC
http://www.teklibre.com
Tel: 1-669-226-2619

Re: [Cake] allocate_src allocate_dst

[Kevin Darbyshire-Bryant]

> On 25 Nov 2017, at 16:41, Jonathan Morton <chromatix99@gmail.com> wrote:
> 
> Not quite.  The "dual" enums have more than one bit set, so you have to test them for equality.
> 
> - Jonathan Morton

Here you go https://github.com/ldir-EDB0/sch_cake/commit/795db1d1df9f9a400d8c4861eeff59b85994ccfb  - rebased onto current cobalt.

Compiles, have built firmware… internet in use so still not actually run.

(KDB secretly likes the gotos )

Re: [Cake] allocate_src allocate_dst

[Pete Heist]

> On Nov 25, 2017, at 5:48 PM, Dave Taht <dave.taht@gmail.com> wrote:
> 
> I am unfond of the gotos as well. But I'm gonna go do laundry. :) If
> someone else beats me to it?

To me the very occasional goto can be nice as an intellectual mint on your pillow- ooh, I didn’t expect to find that there, kind of thing. Seen every day though, well, not my preference either, but expression is a matter of opinion… :)

It’s nice to see the improvements starting, thank you! At least if I try to review the existing code some more I can offer about half an eyeball.

Re: [Cake] allocate_src allocate_dst

[Jonathan Morton]

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

Gotos are fairly common in kernel code, chiefly for exception handling.
Obviously structured code is still preferred where it makes sense, but
there are cases where it would actually confuse matters.

I hope to be able to spend most of tomorrow going over the code as it
currently stands.  I even have an up-to-date net-next tree on one of my
machines to build against.

- Jonathan Morton

[-- Attachment #2: Type: text/html, Size: 451 bytes --]

Re: [Cake] allocate_src allocate_dst

[George Amanakis]

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

I think we missed an "allocate_host" in cake_hash(), line ~810:

if (allocate_host) {
             srchost_idx = srchost_hash % CAKE_QUEUES;
             inner_hash = srchost_idx % CAKE_SET_WAYS;
             outer_hash = srchost_idx - inner_hash;
             for (i = 0, k = inner_hash; i < CAKE_SET_WAYS;
                 i++, k = (k + 1) % CAKE_SET_WAYS) {
                 if (q->hosts[outer_hash + k].srchost_tag ==
                     srchost_hash)
                     goto found_src;
}


On 11/25/2017 2:04 PM, Jonathan Morton wrote:
>
> Gotos are fairly common in kernel code, chiefly for exception 
> handling.  Obviously structured code is still preferred where it makes 
> sense, but there are cases where it would actually confuse matters.
>
> I hope to be able to spend most of tomorrow going over the code as it 
> currently stands.  I even have an up-to-date net-next tree on one of 
> my machines to build against.
>
> - Jonathan Morton
>
>
>
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake


[-- Attachment #2: Type: text/html, Size: 2027 bytes --]

Re: [Cake] allocate_src allocate_dst

[Kevin Darbyshire-Bryant]

Ooops, sorry, dodge merge on my part from my work tree back to cobalt.  A fix & PR will be with you shortly…. apologies.

Kevin

> On 25 Nov 2017, at 20:03, George Amanakis <gamanakis@gmail.com> wrote:
> 
> I think we missed an "allocate_host" in cake_hash(), line ~810:
> 
> if (allocate_host) {
>             srchost_idx = srchost_hash % CAKE_QUEUES;
>             inner_hash = srchost_idx % CAKE_SET_WAYS;
>             outer_hash = srchost_idx - inner_hash;
>             for (i = 0, k = inner_hash; i < CAKE_SET_WAYS;
>                 i++, k = (k + 1) % CAKE_SET_WAYS) {
>                 if (q->hosts[outer_hash + k].srchost_tag ==
>                     srchost_hash)
>                     goto found_src;
> }
> 
> On 11/25/2017 2:04 PM, Jonathan Morton wrote:
>> Gotos are fairly common in kernel code, chiefly for exception handling.  Obviously structured code is still preferred where it makes sense, but there are cases where it would actually confuse matters.
>> 
>> I hope to be able to spend most of tomorrow going over the code as it currently stands.  I even have an up-to-date net-next tree on one of my machines to build against.
>> 
>> - Jonathan Morton
>> 
>> 
>> _______________________________________________
>> Cake mailing list
>> 
>> Cake@lists.bufferbloat.net
>> https://lists.bufferbloat.net/listinfo/cake
> 
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] allocate_src allocate_dst

[Kevin Darbyshire-Bryant]

https://github.com/dtaht/sch_cake/pull/74

Once again, sorry about the screwup.

> On 25 Nov 2017, at 20:49, Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> wrote:
> 
> [This sender failed our fraud detection checks and may not be who they appear to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing]
> 
> Ooops, sorry, dodge merge on my part from my work tree back to cobalt.  A fix & PR will be with you shortly…. apologies.
> 
> Kevin
> 
>> On 25 Nov 2017, at 20:03, George Amanakis <gamanakis@gmail.com> wrote:
>> 
>> I think we missed an "allocate_host" in cake_hash(), line ~810:
>> 
>> if (allocate_host) {
>>            srchost_idx = srchost_hash % CAKE_QUEUES;
>>            inner_hash = srchost_idx % CAKE_SET_WAYS;
>>            outer_hash = srchost_idx - inner_hash;
>>            for (i = 0, k = inner_hash; i < CAKE_SET_WAYS;
>>                i++, k = (k + 1) % CAKE_SET_WAYS) {
>>                if (q->hosts[outer_hash + k].srchost_tag ==
>>                    srchost_hash)
>>                    goto found_src;
>> }
>> 
>> On 11/25/2017 2:04 PM, Jonathan Morton wrote:
>>> Gotos are fairly common in kernel code, chiefly for exception handling.  Obviously structured code is still preferred where it makes sense, but there are cases where it would actually confuse matters.
>>> 
>>> I hope to be able to spend most of tomorrow going over the code as it currently stands.  I even have an up-to-date net-next tree on one of my machines to build against.
>>> 
>>> - Jonathan Morton
>>> 
>>> 
>>> _______________________________________________
>>> Cake mailing list
>>> 
>>> Cake@lists.bufferbloat.net
>>> https://lists.bufferbloat.net/listinfo/cake
>> 
>> _______________________________________________
>> Cake mailing list
>> Cake@lists.bufferbloat.net
>> https://lists.bufferbloat.net/listinfo/cake
> 
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake

Re: [Cake] allocate_src allocate_dst

[Kevin Darbyshire-Bryant]

Have to say, my perception is that I’ve a small cpu usage improvement for my use case with these changes.  Then I would expect that ‘cos I use ‘dual-srchost’ on egress and ‘dual-dsthost’ on ingress, so I’m saving running the dst/src host allocation code on egress/ingress respectively.

Of course for the standard ‘triple-isolate’ things will be slightly slower ‘cos of the conditional flag setting.

Cheers,

Kevin