Re: [Cake] Using firewall connmarks as tin selectors

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

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 :)

>> 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.

Happy to help you work out the details; but I think we'll make more
progress on this if you are driving it :)

-Toke