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 03:16:10PM CEST, boris.sukholitko@broadcom.com wrote:
>On Tue, Apr 12, 2022 at 02:12:15PM +0200, Jiri Pirko wrote:
>> 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?
>
>Yes.
>
>> You are talking about it as it is
>> already possible with the code before this patch being applied.
>> 
>
>Sorry for the confusion. In the updated description I try to make the
>distinction much clearer.
>
>> 
>> >
>> >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.
>> 
>
>The "changed" refers to the code of is_vlan_key function which is
>changed by this patch. Please see the updated description.
>
>> 
>> >
>> >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.
>> 
>
>What about the following:
>
><description>
>
>Before this commit the existence of vlan filters was conditional on the vlan
>protocol being matched in the tc rule. For example, the following rule:
>
>tc filter add dev eth1 ingress flower vlan_prio 5
>
>was illegal because vlan protocol (e.g. 802.1q) does not appear in the rule.
>
>This commit removes the above restriction. Having the num_of_vlans

Say rather just "Remove the above restriction. ..."


>filter configured allows further matching on vlan attributes. The
>following rule is ok now:
>
>tc filter add dev eth1 ingress flower num_of_vlans 1 vlan_prio 5
>
>because having num_of_vlans==1 implies that the packet is single tagged.
>
>To do this, this commit changes is_vlan_key helper to look at the number

"Change the is_vlan_key helper to look..."

Don't talk about "this commit".


>of vlans in addition to the vlan ethertype. Outer (e.g. vlan_prio) and
>inner (e.g. cvlan_prio) tag vlan filters require the number of vlan tags
>to be greater then 0 and 1 accordingly.
>
>As a result of this commit, the ethertype may be set to 0 when matching
>on the number of vlans. This commit changes fl_set_key_vlan to avoid
>setting key, mask vlan_tpid for the 0 ethertype.
>
></description>
>
>Is this going into the right direction?
>
>Thanks,
>Boris.