Re: [PATCH net-next v2 5/5] net/sched: flower: Consider the number of tags for vlan filters

[Jiri Pirko <jiri@resnulli.us>]

Tue, Apr 12, 2022 at 01:40:49PM CEST, boris.sukholitko@broadcom.com wrote:
>On Tue, Apr 12, 2022 at 01:09:35PM +0200, Jiri Pirko wrote:
>> Tue, Apr 12, 2022 at 12:02:36PM CEST, boris.sukholitko@broadcom.com wrote:
>> >Currently the existence of vlan filters is conditional on the vlan
>> >protocol being matched in the tc rule. I.e. the following rule:
>> >
>> >tc filter add dev eth1 ingress flower vlan_prio 5
>> >
>> >is illegal because we lack protocol 802.1q in the rule.
>> >
>> >Having the num_of_vlans filter configured removes this restriction. The
>> >following rule becomes ok:
>> >
>> >tc filter add dev eth1 ingress flower num_of_vlans 1 vlan_prio 5
>> >
>> >because we know that the packet is single tagged.
>> >
>> >We achieve the above by having is_vlan_key helper look at the number of
>> 
>> Sorry to be a nitpicker, but who's "we"? When I read the patch
>> description, I need to understand clearly what the patch is doing, which
>> is not this case. You suppose to command the codebase what to do.
>> I fail to see that :/
>> 
>> 
>
>What do you think of the following description? The description consists
>of two parts: the first provides motivation for the patch, the second is
>the way the motivation is implemented. I've judiciously edited out the
>"we"-word. :)
>
><description>
>
>Currently the existence of vlan filters is conditional on the vlan
>protocol being matched in the tc rule. I.e. the following rule:
>
>tc filter add dev eth1 ingress flower vlan_prio 5
>
>is illegal because vlan protocol (e.g. 802.1q) does not appear in the rule.
>
>Having the num_of_vlans filter configured removes this restriction. The
>following rule becomes ok:
>
>tc filter add dev eth1 ingress flower num_of_vlans 1 vlan_prio 5

So this is what this patch allows? You are talking about it as it is
already possible with the code before this patch being applied.


>
>because having num_of_vlans==1 implies that the packet is single tagged.
>
>To make the above possible, is_vlan_key helper is changed to look at the
>number of vlans in addition to the vlan ethertype.

What "is changed"? You should tell the codebase what to do, what toadd,
remove or change. If you did that, it would be very clear to the reader
what the patch is supposed to do.


>
>Outer tag vlan filters (e.g.  vlan_prio) require the number of vlan tags
>be greater than 0. Inner filters (e.g. cvlan_prio) require the number of
>vlan tags be greater than 1.

Again, unclear what this describes, if the current code before the patch
or the state after this patch.


>
>Number of vlans filter may cause ethertype to be set to 0.
>fl_set_key_vlan is changed to accomodate this.
>
></description>
>
>Thanks,
>Boris.
>
>> >vlans in addition to the vlan ethertype. Outer tag vlan filters (e.g.
>> >vlan_prio) require the number of vlan tags be greater than 0. Inner
>> >filters (e.g. cvlan_prio) require the number of vlan tags be greater
>> >than 1.
>> >
>> >Number of vlans filter may cause ethertype to be set to 0. Check this in
>> >fl_set_key_vlan.
>> >