I’m tired after the last 3 days working on $paidwork (4 jobs in 3 days) so can’t think straight, anyway...
> On 10 Mar 2019, at 23:56, Toke Høiland-Jørgensen <[email protected]> wrote: > > Kevin Darbyshire-Bryant <[email protected]> writes: > <some context snipped> >> >> Yes. I’d point out the (hopefully obvious) that the flag mask needs >> to be one bit bigger than you might immediately think. e.g. diffserv4 >> needs to store 5 values (0-4), 3 bits. 0 is being used as an implied >> ’tin is not set, fall back to DSCP’. One could store DSCP+1 of course >> and use the same logic. > > Yeah, or one can just squat on a whole byte like I do in the example ;) As do I for my ‘DSCP + set/not set flag’ :-) > >> >> The ugliness of doing the diffserv parsing is what prompted the idea >> of storing the DSCP directly and I felt the stored tin selection was >> effectively abstracting the diffserv field anyway. > > Right, but that means that the CAKE interpretation of the fwmark would > have to change from something that selects the tin, to something that is > treated as a DSCP mark. I think this was the part that I was missing > before. I don't think this is a good idea, as that means we tie the > marks to one particular use case. I did say it was incompatible with the existing tin storing idea right from day 1 and why I was so keen/worried by the fact it had already gone upstream. > >> Storing the DSCP is more compatible with differing egress v ingress >> mappings (eg. egress diffserv4, ingress diffserv3 though I can’t >> really think of a use case for that) > > I think that if someone wants to do something like that, we are way out > of "simple use case that we want to actively support" territory, and can > legitimately ask people to go write a BPF filter or something :) > >> Of course using fwmark as tin number selector in cake doesn’t preclude >> some other mechanism of storing & recovering DSCP to/from firewall >> mark e.g. the previously discussed act-connmark+dscp which would help >> anyone who wanted to do such ‘link traversing’ DSCP shenanigans. That >> of course makes you happier since cake doesn’t embed itself further >> into conntrack. > > Yeah, I definitely don't think CAKE has any business writing DSCP values > into the mark. However, as I said before, there may be a case for adding I’m going forward on the basis that *cake* ’storing DSCP within fwmark’ idea has died and will be trying to cobble something together within act_connmark as that has a wider potential client base than just cake. > an option to write the tin selection back to conntrack. Something like > the patch below would do it (with an option to control it, of course), > but it does incur a dependency on another conntrack header, so I'm not > sure if it will be acceptable to upstream. Also, we would need to figure > out how the option should work. I think before expending any further mental effort on that, the question should be asked of the kernel people. I don’t see the point in working out the semantics of something that ultimately won’t be accepted. > The alternative would be to use another mechanism; the iptables rules > replication is one example. Another could be adding a conntrack helper > to eBPF… > > -Toke > > > diff --git a/sch_cake.c b/sch_cake.c > index a8fa224..c6b7dd9 100644 > --- a/sch_cake.c > +++ b/sch_cake.c > @@ -78,6 +78,7 @@ > > #if IS_REACHABLE(CONFIG_NF_CONNTRACK) > #include <net/netfilter/nf_conntrack_core.h> > +#include <net/netfilter/nf_conntrack_ecache.h> > #include <net/netfilter/nf_conntrack_zones.h> > #include <net/netfilter/nf_conntrack.h> > #endif > @@ -1646,6 +1647,27 @@ static u8 cake_handle_diffserv(struct sk_buff *skb, > u16 wash) > } > } > > +static void cake_set_tin_connmark(struct cake_sched_data *q, > + struct sk_buff *skb, u32 tin) > +{ > +#if IS_REACHABLE(CONFIG_NF_CONNTRACK) > + enum ip_conntrack_info ctinfo; > + struct nf_conn *ct; > + u32 newmark; > + > + ct = nf_ct_get(skb, &ctinfo); > + if (ct) { > + newmark = (ct->mark & ~q->fwmark_mask); > + newmark ^= (tin << q->fwmark_shft) & q->fwmark_mask; > + > + if (ct->mark != newmark) { > + ct->mark = newmark; > + nf_conntrack_event_cache(IPCT_MARK, ct); > + } > + } > +#endif > +} > + > static struct cake_tin_data *cake_select_tin(struct Qdisc *sch, > struct sk_buff *skb) > { > @@ -1678,6 +1700,8 @@ static struct cake_tin_data *cake_select_tin(struct > Qdisc *sch, > tin = 0; > } > > + cake_set_tin_connmark(q, skb, tin); > + > return &q->tins[tin]; > } Cheers, Kevin D-B 012C ACB2 28C6 C53E 9775 9123 B3A2 389B 9DE2 334A _______________________________________________ Cake mailing list [email protected] https://lists.bufferbloat.net/listinfo/cake
