| Message ID | 20220411133100.18126-6-boris.sukholitko@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | flower: match on the number of vlan tags | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: pabeni@redhat.com |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | warning | WARNING: line length of 81 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
Mon, Apr 11, 2022 at 03:31:00PM 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. From this patch description, I'm unable to tell what the patch is doing. Tell the codebase what to do. Also, in subject line of the patches, it is customary to put prefix like: "net/sched: cls_flower:" The the first glance, the patchset looks fine to me.
On Mon, Apr 11, 2022 at 05:34:13PM +0200, Jiri Pirko wrote: > Mon, Apr 11, 2022 at 03:31:00PM 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. > > From this patch description, I'm unable to tell what the patch is doing. > Tell the codebase what to do. > I've expanded the description in v2 of the patches. > Also, in subject line of the patches, it is customary to put prefix > like: "net/sched: cls_flower:" Done in v2. > > The the first glance, the patchset looks fine to me. Thanks, Boris.
diff --git a/net/sched/cls_flower.c b/net/sched/cls_flower.c index 42dd84f5a037..464a91e64b5f 100644 --- a/net/sched/cls_flower.c +++ b/net/sched/cls_flower.c @@ -1023,8 +1023,10 @@ static void fl_set_key_vlan(struct nlattr **tb, VLAN_PRIORITY_MASK; key_mask->vlan_priority = VLAN_PRIORITY_MASK; } - key_val->vlan_tpid = ethertype; - key_mask->vlan_tpid = cpu_to_be16(~0); + if (ethertype) { + key_val->vlan_tpid = ethertype; + key_mask->vlan_tpid = cpu_to_be16(~0); + } } static void fl_set_key_flag(u32 flower_key, u32 flower_mask, @@ -1495,13 +1497,18 @@ static int fl_set_key_ct(struct nlattr **tb, } static bool is_vlan_key(struct nlattr *tb, __be16 *ethertype, - struct fl_flow_key *key, struct fl_flow_key *mask) + struct fl_flow_key *key, struct fl_flow_key *mask, + int vthresh) { - if (!tb) - return false; + const bool good_num_of_vlans = key->num_of_vlans.num_of_vlans > vthresh; + + if (!tb) { + *ethertype = 0; + return good_num_of_vlans; + } *ethertype = nla_get_be16(tb); - if (eth_type_vlan(*ethertype)) + if (good_num_of_vlans || eth_type_vlan(*ethertype)) return true; key->basic.n_proto = *ethertype; @@ -1536,12 +1543,13 @@ static int fl_set_key(struct net *net, struct nlattr **tb, TCA_FLOWER_UNSPEC, sizeof(key->num_of_vlans)); - if (is_vlan_key(tb[TCA_FLOWER_KEY_ETH_TYPE], ðertype, key, mask)) { + if (is_vlan_key(tb[TCA_FLOWER_KEY_ETH_TYPE], ðertype, key, mask, 0)) { fl_set_key_vlan(tb, ethertype, TCA_FLOWER_KEY_VLAN_ID, TCA_FLOWER_KEY_VLAN_PRIO, &key->vlan, &mask->vlan); - if (is_vlan_key(tb[TCA_FLOWER_KEY_VLAN_ETH_TYPE], ðertype, key, mask)) { + if (is_vlan_key(tb[TCA_FLOWER_KEY_VLAN_ETH_TYPE], + ðertype, key, mask, 1)) { fl_set_key_vlan(tb, ethertype, TCA_FLOWER_KEY_CVLAN_ID, TCA_FLOWER_KEY_CVLAN_PRIO,
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. Signed-off-by: Boris Sukholitko <boris.sukholitko@broadcom.com> --- net/sched/cls_flower.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)