search

dtaht / sch_cake

Out of tree build for the new cake qdisc
100 stars 35 forks source link

cake: don't wrong skb->mac_len gso fixup on fixed kernels #107

Closed ldir-EDB0 closed 6 years ago

ldir-EDB0 commented 6 years ago

Stable kernels backport the gso fixup fix, so cake doesn't need to do it. Let's not waste cpu cycles by doing it in cake which could be really important on cpu constrained devices.

Signed-off-by: Kevin Darbyshire-Bryant ldir@darbyshire-bryant.me.uk

tohojo commented 6 years ago

Kevin D-B notifications@github.com writes:

Stable kernels backport the gso fixup fix, so cake doesn't need to do it. Let's not waste cpu cycles by doing it in cake which could be really important on cpu constrained devices.

Does this mean the upstream fix has made it into openwrt? What about for 18.06?

ldir-EDB0 commented 6 years ago

Short answer: yes. openwrt master & 18.06 bumped to fixed kernels a few minutes ago. I put in a patch for cake (the one I've just submitted here) to not do the fixup. Tested ok. Quite when 18.06.2 gets release I've no idea, but anyone self building off master/18.06 will get the tweaks

https://git.openwrt.org/?p=openwrt/openwrt.git;a=summary https://git.openwrt.org/?p=openwrt/openwrt.git;a=shortlog;h=refs/heads/openwrt-18.06

tohojo commented 6 years ago

Right, cool. However, this pull request is obviously broken; seems like you're adding a patch to the top of the .c file?

Also, I'm not sure we should be doing version checks for this; I'd rather just remove the call to skb_reset_mac_len() entirely and tell people to go upgrade their kernels...

ldir-EDB0 commented 6 years ago

How the hell did I manage that!? Oh well, fixed now.

There's anecdotal evidence that the call to skb_reset_mac_len() in the module isn't working anyway https://forum.openwrt.org/t/low-speeds-on-ipv6-with-sqm-cake/22403/8 which is all a bit odd. On the other hand, complete removal will upset those building on really ancient kernels...but then they might not be affected anyway.

So having written all that I'm now more in favour of dropping skb_reset_mac_len than keeping it.