Re: [Cake] Using firewall connmarks as tin selectors

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

Toke Høiland-Jørgensen <toke@redhat.com> writes:

> Kevin Darbyshire-Bryant <kevin@darbyshire-bryant.me.uk> writes:
>
>>> On 4 Mar 2019, at 17:36, Toke Høiland-Jørgensen <toke@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