Re: [Cake] [PATCH net] sched: consistently handle layer3 header accesses in the presence of VLANs

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

Davide Caratti <dcaratti@redhat.com> writes:

> hello Toke,
>
> thanks for answering!
>
> On Fri, 2020-07-03 at 14:05 +0200, Toke Høiland-Jørgensen wrote:
>>   while (proto == htons(ETH_P_8021Q) || proto == htons(ETH_P_8021AD)) {
>
> maybe this line be shortened, since if_vlan.h has [1]:
>
> while (eth_type_vlan(proto)) {
>  	...
> }

Good point, missed that! Will fix and send a v2.

> If I read well, the biggest change from functional point of view is that
> now qdiscs can set the ECN bit also on non-accelerated VLAN packets and
> QinQ-tagged packets, if the IP header is the outer-most header after VLAN;
> and the same applies to almost all net/sched former users of skb->protocol 
> or tc_skb_protocol().

Yup, that's the idea.

> Question (sorry in advance because it might be a dumb one :) ):
>
> do you know why cls_flower, act_ct, act_mpls and act_connmark keep reading
> skb->protocol? is that intentional?

Hmm, no not really. I only checked for calls to tc_skb_protocol(), not
for direct uses of skb->protocol. Will fix those as well :)

> (for act_mpls that doesn't look intentional, and probably the result is
> that the BOS bit is not set correctly if someone tries to push/pop a label
> for a non-accelerated or QinQ packet. But I didn't try it experimentally
> :) )

Hmm, you're certainly right that the MPLS code should use the helper to
get consistent use between accelerated/non-accelerated VLAN usage. But I
don't know enough about MPLS to judge whether it should be skipping the
VLAN tags or not. Sounds like you're saying the right thing is to skip
the VLAN tags there as well?

Looking at the others, it looks like act_connmark and act_ct both ought
to skip VLAN tags, while act_flower should probably keep it, since it
seems it has a VLAN match type. Or?

-Toke