[Cake] Update kernel version check in cake

[Cake] Update kernel version check in cake

[Kevin Darbyshire-Bryant]

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

Hi,

FYI: Updated kernel version check in latest sch_cake - openwrt wasn't
very happy when pointed at latest release

Pull request sent:  https://github.com/dtaht/sch_cake/pulls


Cheers,

Kevin



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4816 bytes --]

Re: [Cake] Update kernel version check in cake

[Dave Taht]

so nice to get some actual work done. I've been so buried in this FCC stuff.

kevin, could you take a look at

 https://github.com/dtaht/sch_cake/blob/master/sch_cake.c#L426

and figure out the right thing?

Ideally this is where I wanted to put the "squash" option as well,
wiping the non-ecn bits. squashing could be made as a new number in
the CAKE_MODE_BESTEFFORT enum, and treated basically the same
elsewhere as CAKE_MODE_BESTEFFORT.

On Mon, Sep 28, 2015 at 4:52 AM, Kevin Darbyshire-Bryant
<kevin@darbyshire-bryant.me.uk> wrote:
> Hi,
>
> FYI: Updated kernel version check in latest sch_cake - openwrt wasn't
> very happy when pointed at latest release
>
> Pull request sent:  https://github.com/dtaht/sch_cake/pulls
>
>
> Cheers,
>
> Kevin
>
>
>
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake
>



-- 
Dave Täht
Do you want faster, better, wifi? https://www.patreon.com/dtaht

Re: [Cake] Update kernel version check in cake

[Kevin Darbyshire-Bryant]

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

On 28/09/15 13:09, Dave Taht wrote:
> so nice to get some actual work done. I've been so buried in this FCC stuff.
>
> kevin, could you take a look at
>
>  https://github.com/dtaht/sch_cake/blob/master/sch_cake.c#L426
>
> and figure out the right thing?
>
> Ideally this is where I wanted to put the "squash" option as well,
> wiping the non-ecn bits. squashing could be made as a new number in
> the CAKE_MODE_BESTEFFORT enum, and treated basically the same
> elsewhere as CAKE_MODE_BESTEFFORT.
>
Hi Dave :-)

This makes me laugh so much.  Today I somehow appear to have convinced
Seb that I understand German (I don't) and you think that I understand
linux kernel network internals whereas in reality I barely know on end
of a C compiler from the other.  Maybe I should try politics? :-)

But I've had a quick look at the code and from what I glean from a bit
of googling, guesswork & intuition, mostly confirms the comment in the
code namely we don't need the 'cow'.

1) unlikely (blah blah blah) - hint the compiler that the following call
is unlikely to fail for optimisation purposes.
2) skb_cow_head copies an skb with a certain amount of buffer headroom,
the implication being we're going to stuff some more data into it.  We
actually request another sizeof(ip_hdr) bytes and for the life of me I
cannot work out why we need more space.....though see point 3.


I can't help feel this could be replaced with:

static inline unsigned int cake_get_diffserv(struct sk_buff *skb)
{
    /* borrowed from sch_dsmark */
    switch (skb->protocol) {
    case htons(ETH_P_IP):

        return ipv4_get_dsfield(ip_hdr(skb)) >> 2;

    case htons(ETH_P_IPV6):
        return ipv6_get_dsfield(ipv6_hdr(skb)) >> 2;

    default:
        /* If there is no Diffserv field, treat as bulk */
        return 0;
    };
}

Maybe if I'm daring, I'll try compiling this and see how my router blows
up :-)


3) I think the real clue is your comment that this is where you wanted
to do squashing, which sounds more like altering packets and to me much
more dangerous and also I'm beginning to guess a *LOT* more here.  I
suspect a skb_cow_head (cow=copy on write) is more likely to be needed
here, but I still fail to see the need to allocate extra space for an
ip_hdr struct...we're going to mangle an existing header.

The thought of writing to an skb fills me with terror!  I really don't
know the rules.  Some here know this stuff upside down, backwards,
inside out *and* forwards.

I'm guessing, but I'd love to know how well I guessed ;-)

Kevin 


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4816 bytes --]

Re: [Cake] Update kernel version check in cake

[Jonathan Morton]

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

I actually had the version check fix locally, but for some reason it hadn't
been picked up by a push yet.

Squashing would involve writing to the TOS byte, which is already done by
ECN marking, via a helper function.

I tried removing the cow stuff once before.  Everything promptly stopped
working until I put it back.  I have no idea why.

- Jonathan Morton

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

Re: [Cake] Update kernel version check in cake

[Kevin Darbyshire-Bryant]

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



On 29/09/15 00:01, Jonathan Morton wrote:
>
> I actually had the version check fix locally, but for some reason it
> hadn't been picked up by a push yet.
>
Yep, I have those moments.  Just when I think I've got git sussed it
goes and does something completely logical but totally unexpected :-)
>
> Squashing would involve writing to the TOS byte, which is already done
> by ECN marking, via a helper function.
>
Ah, so if I can work out how that's done I'm 50% of the way there. 
Should keep me amused for a few days.
>
> I tried removing the cow stuff once before.  Everything promptly
> stopped working until I put it back.  I have no idea why.
>
Well here's interesting.  I got brave and put my suggested change into
action.  So far: 1) router not blown up  2) tc -s still shows traffic in
different Classes (Dave, I prefer your term 'Bin' here too - I
considered 'Bucket' for a laugh) which suggests the reading of ip_hdr is
still working. 3) This hasn't been thoroughly tested in any way
whatsoever but i've tried diffserv4 & besteffort and both don't appear
broken.

Pull request: https://github.com/dtaht/sch_cake/pull/6

Kevin
>
> - Jonathan Morton
>



[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4816 bytes --]

Re: [Cake] Update kernel version check in cake

[Dave Taht]

On Tue, Sep 29, 2015 at 2:42 AM, Kevin Darbyshire-Bryant
<kevin@darbyshire-bryant.me.uk> wrote:
>
>
> On 29/09/15 00:01, Jonathan Morton wrote:
>>
>> I actually had the version check fix locally, but for some reason it
>> hadn't been picked up by a push yet.
>>
> Yep, I have those moments.  Just when I think I've got git sussed it
> goes and does something completely logical but totally unexpected :-)
>>
>> Squashing would involve writing to the TOS byte, which is already done
>> by ECN marking, via a helper function.
>>
> Ah, so if I can work out how that's done I'm 50% of the way there.
> Should keep me amused for a few days.
>>
>> I tried removing the cow stuff once before.  Everything promptly
>> stopped working until I put it back.  I have no idea why.
>>
> Well here's interesting.  I got brave and put my suggested change into
> action.  So far: 1) router not blown up  2) tc -s still shows traffic in
> different Classes (Dave, I prefer your term 'Bin' here too - I
> considered 'Bucket' for a laugh) which suggests the reading of ip_hdr is
> still working. 3) This hasn't been thoroughly tested in any way
> whatsoever but i've tried diffserv4 & besteffort and both don't appear
> broken.

I agree with bin. Wouldn't mind it if that made it into the variable
name instead of class.

> Pull request: https://github.com/dtaht/sch_cake/pull/6

It would be my hope also that killing the cow would speed it up.

Untested by me, but pulled.


> Kevin
>>
>> - Jonathan Morton
>>
>
>



-- 
Dave Täht
Do you want faster, better, wifi? https://www.patreon.com/dtaht

Re: [Cake] Update kernel version check in cake

[Dave Taht]

I added support for squashing to both sch_cake and tc-adv repos. Sort
of compile tested
only. (my laptop's kernel is 3.16 and doesn't build, my virtuals in
linode do not come with
kernel sources, snapon crashed two weeks ago, and so on)

but it was nice to write a few lines of code, knowing that someone
here will fix 'em if
I broke something.


On Tue, Sep 29, 2015 at 2:55 AM, Dave Taht <dave.taht@gmail.com> wrote:
> On Tue, Sep 29, 2015 at 2:42 AM, Kevin Darbyshire-Bryant
> <kevin@darbyshire-bryant.me.uk> wrote:
>>
>>
>> On 29/09/15 00:01, Jonathan Morton wrote:
>>>
>>> I actually had the version check fix locally, but for some reason it
>>> hadn't been picked up by a push yet.
>>>
>> Yep, I have those moments.  Just when I think I've got git sussed it
>> goes and does something completely logical but totally unexpected :-)
>>>
>>> Squashing would involve writing to the TOS byte, which is already done
>>> by ECN marking, via a helper function.
>>>
>> Ah, so if I can work out how that's done I'm 50% of the way there.
>> Should keep me amused for a few days.
>>>
>>> I tried removing the cow stuff once before.  Everything promptly
>>> stopped working until I put it back.  I have no idea why.
>>>
>> Well here's interesting.  I got brave and put my suggested change into
>> action.  So far: 1) router not blown up  2) tc -s still shows traffic in
>> different Classes (Dave, I prefer your term 'Bin' here too - I
>> considered 'Bucket' for a laugh) which suggests the reading of ip_hdr is
>> still working. 3) This hasn't been thoroughly tested in any way
>> whatsoever but i've tried diffserv4 & besteffort and both don't appear
>> broken.
>
> I agree with bin. Wouldn't mind it if that made it into the variable
> name instead of class.
>
>> Pull request: https://github.com/dtaht/sch_cake/pull/6
>
> It would be my hope also that killing the cow would speed it up.
>
> Untested by me, but pulled.
>
>
>> Kevin
>>>
>>> - Jonathan Morton
>>>
>>
>>
>
>
>
> --
> Dave Täht
> Do you want faster, better, wifi? https://www.patreon.com/dtaht



-- 
Dave Täht
Do you want faster, better, wifi? https://www.patreon.com/dtaht

Re: [Cake] Update kernel version check in cake

[Sebastian Moeller]

Hi Dave,


On Sep 29, 2015, at 13:10 , Dave Taht <dave.taht@gmail.com> wrote:

> I added support for squashing to both sch_cake and tc-adv repos. Sort
> of compile tested
> only. (my laptop's kernel is 3.16 and doesn't build, my virtuals in
> linode do not come with
> kernel sources, snapon crashed two weeks ago, and so on)
> 

	Just played a bit with the shiny new squash option. For my limited testing it seems to work. I do wonder whether the automatic default to besteffort is the ideal behavior though. For example on egress it would be nice to be able to use the internal set DSCP markings but clean them from the outgoing packets as not to leak “information” to the ISP, similarly but more contrived the same could be agued for ingress ;) 


Best Regards


> but it was nice to write a few lines of code, knowing that someone
> here will fix 'em if
> I broke something.
> 
> 
> On Tue, Sep 29, 2015 at 2:55 AM, Dave Taht <dave.taht@gmail.com> wrote:
>> On Tue, Sep 29, 2015 at 2:42 AM, Kevin Darbyshire-Bryant
>> <kevin@darbyshire-bryant.me.uk> wrote:
>>> 
>>> 
>>> On 29/09/15 00:01, Jonathan Morton wrote:
>>>> 
>>>> I actually had the version check fix locally, but for some reason it
>>>> hadn't been picked up by a push yet.
>>>> 
>>> Yep, I have those moments.  Just when I think I've got git sussed it
>>> goes and does something completely logical but totally unexpected :-)
>>>> 
>>>> Squashing would involve writing to the TOS byte, which is already done
>>>> by ECN marking, via a helper function.
>>>> 
>>> Ah, so if I can work out how that's done I'm 50% of the way there.
>>> Should keep me amused for a few days.
>>>> 
>>>> I tried removing the cow stuff once before.  Everything promptly
>>>> stopped working until I put it back.  I have no idea why.
>>>> 
>>> Well here's interesting.  I got brave and put my suggested change into
>>> action.  So far: 1) router not blown up  2) tc -s still shows traffic in
>>> different Classes (Dave, I prefer your term 'Bin' here too - I
>>> considered 'Bucket' for a laugh) which suggests the reading of ip_hdr is
>>> still working. 3) This hasn't been thoroughly tested in any way
>>> whatsoever but i've tried diffserv4 & besteffort and both don't appear
>>> broken.
>> 
>> I agree with bin. Wouldn't mind it if that made it into the variable
>> name instead of class.
>> 
>>> Pull request: https://github.com/dtaht/sch_cake/pull/6
>> 
>> It would be my hope also that killing the cow would speed it up.
>> 
>> Untested by me, but pulled.
>> 
>> 
>>> Kevin
>>>> 
>>>> - Jonathan Morton
>>>> 
>>> 
>>> 
>> 
>> 
>> 
>> --
>> Dave Täht
>> Do you want faster, better, wifi? https://www.patreon.com/dtaht
> 
> 
> 
> -- 
> Dave Täht
> Do you want faster, better, wifi? https://www.patreon.com/dtaht
> _______________________________________________
> Cake mailing list
> Cake@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/cake