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:

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

This would both sidestep the whole conntrack dependency issue, and make
the same functionality available outside of CAKE (for an HTB-based
setup, for instance).

> v2 addressing some of the comments attached.  Is it best to keep the
> in progress patches here or should they be github PRs ?

Patches on the mailing list is fine by me, and it seems there are people
reading the list, but not github, so let's keep it here for now at
least. However, I'll hold off on more detailed comments on the patch
until we've resolved the point above. With one exception:

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

-Toke