> On 4 Mar 2019, at 16:39, Toke Høiland-Jørgensen <[email protected]> wrote:
>
> Kevin Darbyshire-Bryant <[email protected]> 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'
As I said earlier I couldn't work out how m_conntrack did… anything at all to
be honest!
>
> 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:
There’s certainly been some response here that’s for sure :-)
>
>> @@ -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.
#if bit 6 0 then not been marked by cake - go & mangle the DSCP ready for cake
to find & set
#the mark.
ipt -t mangle -A PREROUTING -i $IFACE -m mark --mark 0x00/0x40000000 -g
QOS_MARK_${IFACE}
ipt -t mangle -A POSTROUTING -o $IFACE -m mark --mark 0x00/0x40000000 -g
QOS_MARK_${IFACE}
Cheers,
Kevin D-B
012C ACB2 28C6 C53E 9775 9123 B3A2 389B 9DE2 334A
_______________________________________________
Cake mailing list
[email protected]
https://lists.bufferbloat.net/listinfo/cake
