Kevin Darbyshire-Bryant <ke...@darbyshire-bryant.me.uk> writes: >> On 4 Mar 2019, at 16:39, Toke Høiland-Jørgensen <t...@redhat.com> wrote: >> >> Kevin Darbyshire-Bryant <ke...@darbyshire-bryant.me.uk> writes: >> >> [ ... snipping a bit of context here ... ] >> >>>>>>> +void cake_update_ct_mark(struct sk_buff *skb, u8 dscp) >>>>>>> +{ >>>>>>> + enum ip_conntrack_info ctinfo; >>>>>>> + struct nf_conn *ct; >>>>>>> + >>>>>>> + ct = nf_ct_get(skb, &ctinfo); >>>>>>> + if (!ct) >>>>>>> + return; >>>>>>> + >>>>>>> + ct->mark &= 0x80ffffff; >>>>>>> + ct->mark |= (0x40 | dscp) << 24; >>>>>> >>>>>> Right, so we *might* have an argument that putting the *tin* into the >>>>>> fwmark is CAKE's business, but copying over the dscp mark is not >>>>>> something a qdisc should be doing… >>>>> >>>>> Why ever not? It’s not the DSCP, it’s a lookup value into the cake >>>>> priority table, it just happens to look like the DSCP ;-) >>>> >>>> If it quacks like a duck… >>> >>> I honestly don’t know where to go from here. I’m clearly trying to do >>> something that the kernel doesn’t want to do. >> >> I'm not disputing that what you're trying to do (moving DSCP field into >> connmark) is useful. I'm just questioning whether CAKE is the right >> place to do this. I think it would fit better in a TC action; either >> modify act_connmark, or create a new action to sync fwmarks and DSCP >> marks… > > Interesting. Thinks out loud - Two actions - ‘act_storedscp’, > ‘act_restoredscp'
Or act_dscp with 'get' and 'set' options :) > As I said earlier I couldn't work out how m_conntrack did… anything at > all to be honest! act_connmark is pretty straight forward: https://elixir.bootlin.com/linux/latest/source/net/sched/act_connmark.c#L34 >>> @@ -1661,13 +1695,14 @@ static struct cake_tin_data *cake_select_tin(struct >>> Qdisc *sch, >>> tin = 0; >>> >>> else if (q->rate_flags & CAKE_FLAG_FWMARK && /* use fw mark */ >>> - skb->mark && >>> - skb->mark <= q->tin_cnt) >>> - tin = q->tin_order[skb->mark - 1]; >>> - >>> - else if (TC_H_MAJ(skb->priority) == sch->handle && >>> - TC_H_MIN(skb->priority) > 0 && >>> - TC_H_MIN(skb->priority) <= q->tin_cnt) >>> + skb->mark & 0x40000000) { >> >> I think there's something odd with this mask? There's only one bit set >> in it… > > I use the single bit as a flag to indicate cake has stored the DSCP > in the lower 6 bits of the byte. Otherwise with a DSCP of 0 (the > default) it’s rather difficult to know if a connection has been > through the cake ’save dscp to fwmark’ process or not. That also > indicates to user space whether it should consider mangling packets or > not e.g. Ah, right. But that breaks the previous use case where someone just wants to set fwmarks that get turned into CAKE tins? I think this definitely is leaning towards decoupling the fw-mark-to-DSCP settings to an action. And probably making the shift and mask configurable rather than hard-coding something... -Toke _______________________________________________ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake