Re: [Cake] [PATCH net-next v19 0/8] sched: Add Common Applications Kept Enhanced (cake) qdisc

["Toke Høiland-Jørgensen" <toke@toke.dk>]

David Miller <davem@davemloft.net> writes:

> From: Toke Høiland-Jørgensen <toke@toke.dk>
> Date: Fri, 06 Jul 2018 17:37:19 +0200
>
>> This patch series adds the CAKE qdisc, and has been split up to ease
>> review.
>> 
>> I have attempted to split out each configurable feature into its own patch.
>> The first commit adds the base shaper and packet scheduler, while
>> subsequent commits add the optional features. The full userspace API and
>> most data structures are included in this commit, but options not
>> understood in the base version will be ignored.
>> 
>> The result of applying the entire series is identical to the out of tree
>> version that have seen extensive testing in previous deployments, most
>> notably as an out of tree patch to OpenWrt. However, note that I have only
>> compile tested the individual patches; so the whole series should be
>> considered as a unit.
>
> Ok, I decided to apply this even though there are still bits I'm not
> %100 happy with.

Yay, awesome, thanks! :)

> I don't like the netfilter dependency at all.
>
> You can get the NAT addresses in other ways as I've tried to suggest
> in the past. Your scheme absolutely does not work with act_nat in the
> packet scheduler, not any NAT done by XDP/eBPF programs.

Just to reiterate why we didn't go with your suggestion of recording the
pre-NAT IP in the flow dissector as the packet comes in:

- It only works on egress; on ingress (with an ifb), packets hit the
  qdisc before NAT, so we need the stateful lookup in CAKE for this
  case, which is a common deployment scenario.

- It's not needed for act_nat (for 1-to-1 NAT, hashing on the post-NAT
  IP is fine), and it won't work for XDP (which would change the packets
  before the flow dissector sees them). This means that custom NAT
  solutions in TC BPF hooks are the only ones that would benefit; and
  they can just set the classifier to achieve the same thing.

Now, I'm absolutely not opposed to having this as a fallback egress-only
mechanism. I might even be convinced to write it myself if someone
demonstrates that they really need it :)

-Toke