Toke Høiland-Jørgensen <t...@redhat.com> writes: > Kevin Darbyshire-Bryant <ke...@darbyshire-bryant.me.uk> writes: > >>> On 4 Mar 2019, at 17:36, Toke Høiland-Jørgensen <t...@redhat.com> wrote: >>> < snips > >>> >>> 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 >> >> Oh bloody hell I’m an idiot - I was looking in user space tc code for >> something that obviously lives in kernel land. Yes *now* I can see >> what it does. > > No comment ;) > >>>>>> @@ -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? >> >> Yes, which is why I was hoping for upstream to bounce it, not least >> because of the unmasked use of fwmark field. Personally I’d like to >> see that change reverted before we get trapped into something we’ll >> regret. > > Well, we have plenty of time to try out things; the whole point of the > rc cycle is testing. But sure, if we don't settle on something, we can > just revert it :)
And, well, the simple thing to do is just keep the current behaviour, or add a mask of 0xff, and use the tc action for everything that's more advanced than this... >>> 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… >> >> I think so too though I think the mechanism of copying the DSCP bits >> and adding a ‘I did this’ flag bit should be retained so that other >> user space tools (iptables etc) can detect when a connmark based DSCP >> has been set/applied. > > I guess this could be an option as well? > >> I think cake ‘fwmark’ should have the smarts to look for the act_dscp >> DSCP value if nothing else so we don’t have to have the overhead of >> act_dscp set restoring DSCP to all the packets if we don’t want to. > > Not sure what you mean here? > >> I’m right at the limit of my coding ability with what I’ve sent in so >> far - the kernel space bits of act_connmark leave me mostly confused - >> really not sure where to start with act_dscp! > > I think I would start with `cp act_connmark.c act_dscp.c`, adding the > new file to the Makefile and Kconfig, and working from there. Then rip > out everything not needed, and copy over what you already added to > cake. Or even simpler, just add new options to act_connmark... -Toke _______________________________________________ Cake mailing list Cake@lists.bufferbloat.net https://lists.bufferbloat.net/listinfo/cake