[Cake] Using firewall connmarks as tin selectors

[Toke Høiland-Jørgensen]

Kevin Darbyshire-Bryant <kevin at darbyshire-bryant.me.uk> writes:

>> On 4 Mar 2019, at 16:39, Toke Høiland-Jørgensen <toke at redhat.com> wrote:
>> 
>> Kevin Darbyshire-Bryant <kevin at 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