[ovs-dev] lib/tc: treat vlan id and prio as independent fields

Message ID 1536574105-5987-1-git-send-email-pieter.jansenvanvuuren@netronome.com
State Accepted
Headers show
Series
  • [ovs-dev] lib/tc: treat vlan id and prio as independent fields
Related show

Commit Message

Pieter Jansen van Vuuren Sept. 10, 2018, 10:08 a.m.
Previously the key was used to check the presence of vlan id and
prio fields instead of using the mask. Additionally the vlan id
field was considered to be present if only the prio field was set,
and vice versa. f.e. setting the following:

ovs-ofctl -OOpenFlow13,OpenFlow15 add-flow br0 \
priority=10,cookie=1,table=0,ip,dl_vlan_pcp=2,actions=output:2

Resulted in (instead of wildcarding vlan_id, filter matches 0):
filter protocol 802.1Q pref 1 flower chain 0
filter protocol 802.1Q pref 1 flower chain 0 handle 0x1
 vlan_id 0
 vlan_prio 2
 vlan_ethtype ip
 eth_type ipv4
 ip_flags nofrag
 in_hw
       action order 1: mirred (Egress Redirect to device eth1) stolen
       index 2 ref 1 bind 1 installed 5 sec used 5 sec
       Action statistics:
       Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
       backlog 0b 0p requeues 0
       cookie 47040ae7a94fff6afd7ed8aa04b11ba4

Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 lib/netdev-tc-offloads.c |  4 ++++
 lib/tc.c                 | 12 ++++++++++--
 2 files changed, 14 insertions(+), 2 deletions(-)

Comments

Simon Horman Sept. 10, 2018, 1:24 p.m. | #1
On Mon, Sep 10, 2018 at 11:08:25AM +0100, Pieter Jansen van Vuuren wrote:
> Previously the key was used to check the presence of vlan id and
> prio fields instead of using the mask. Additionally the vlan id
> field was considered to be present if only the prio field was set,
> and vice versa. f.e. setting the following:
> 
> ovs-ofctl -OOpenFlow13,OpenFlow15 add-flow br0 \
> priority=10,cookie=1,table=0,ip,dl_vlan_pcp=2,actions=output:2
> 
> Resulted in (instead of wildcarding vlan_id, filter matches 0):
> filter protocol 802.1Q pref 1 flower chain 0
> filter protocol 802.1Q pref 1 flower chain 0 handle 0x1
>  vlan_id 0
>  vlan_prio 2
>  vlan_ethtype ip
>  eth_type ipv4
>  ip_flags nofrag
>  in_hw
>        action order 1: mirred (Egress Redirect to device eth1) stolen
>        index 2 ref 1 bind 1 installed 5 sec used 5 sec
>        Action statistics:
>        Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>        backlog 0b 0p requeues 0
>        cookie 47040ae7a94fff6afd7ed8aa04b11ba4
> 
> Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>

Thanks Pieter,

this looks good to me. I will, however, wait a few days before applying to
allow others time to review this change.

Also, is this change relevant to released versions of OVS?
If so, I think some backports will need to be prepared
as this patch does not apply cleanly to branch-2.10.
Pieter Jansen van Vuuren Sept. 11, 2018, 11:23 a.m. | #2
On 10/09/2018 14:24, Simon Horman wrote:
> On Mon, Sep 10, 2018 at 11:08:25AM +0100, Pieter Jansen van Vuuren wrote:
>> Previously the key was used to check the presence of vlan id and
>> prio fields instead of using the mask. Additionally the vlan id
>> field was considered to be present if only the prio field was set,
>> and vice versa. f.e. setting the following:
>>
>> ovs-ofctl -OOpenFlow13,OpenFlow15 add-flow br0 \
>> priority=10,cookie=1,table=0,ip,dl_vlan_pcp=2,actions=output:2
>>
>> Resulted in (instead of wildcarding vlan_id, filter matches 0):
>> filter protocol 802.1Q pref 1 flower chain 0
>> filter protocol 802.1Q pref 1 flower chain 0 handle 0x1
>>  vlan_id 0
>>  vlan_prio 2
>>  vlan_ethtype ip
>>  eth_type ipv4
>>  ip_flags nofrag
>>  in_hw
>>        action order 1: mirred (Egress Redirect to device eth1) stolen
>>        index 2 ref 1 bind 1 installed 5 sec used 5 sec
>>        Action statistics:
>>        Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>>        backlog 0b 0p requeues 0
>>        cookie 47040ae7a94fff6afd7ed8aa04b11ba4
>>
>> Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
>> Reviewed-by: Simon Horman <simon.horman@netronome.com>
> 
> Thanks Pieter,
> 
> this looks good to me. I will, however, wait a few days before applying to
> allow others time to review this change.
> 
> Also, is this change relevant to released versions of OVS?

Ah yes. I believe it is relevant to the released version as well.

> If so, I think some backports will need to be prepared
> as this patch does not apply cleanly to branch-2.10.
> 

Thank you Simon, I'll prepare a backport for branch-2.10.
Simon Horman Sept. 12, 2018, 12:31 p.m. | #3
On Tue, Sep 11, 2018 at 12:23:20PM +0100, Pieter Jansen van Vuuren wrote:
> On 10/09/2018 14:24, Simon Horman wrote:
> > On Mon, Sep 10, 2018 at 11:08:25AM +0100, Pieter Jansen van Vuuren wrote:
> >> Previously the key was used to check the presence of vlan id and
> >> prio fields instead of using the mask. Additionally the vlan id
> >> field was considered to be present if only the prio field was set,
> >> and vice versa. f.e. setting the following:
> >>
> >> ovs-ofctl -OOpenFlow13,OpenFlow15 add-flow br0 \
> >> priority=10,cookie=1,table=0,ip,dl_vlan_pcp=2,actions=output:2
> >>
> >> Resulted in (instead of wildcarding vlan_id, filter matches 0):
> >> filter protocol 802.1Q pref 1 flower chain 0
> >> filter protocol 802.1Q pref 1 flower chain 0 handle 0x1
> >>  vlan_id 0
> >>  vlan_prio 2
> >>  vlan_ethtype ip
> >>  eth_type ipv4
> >>  ip_flags nofrag
> >>  in_hw
> >>        action order 1: mirred (Egress Redirect to device eth1) stolen
> >>        index 2 ref 1 bind 1 installed 5 sec used 5 sec
> >>        Action statistics:
> >>        Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
> >>        backlog 0b 0p requeues 0
> >>        cookie 47040ae7a94fff6afd7ed8aa04b11ba4
> >>
> >> Signed-off-by: Pieter Jansen van Vuuren <pieter.jansenvanvuuren@netronome.com>
> >> Reviewed-by: Simon Horman <simon.horman@netronome.com>
> > 
> > Thanks Pieter,
> > 
> > this looks good to me. I will, however, wait a few days before applying to
> > allow others time to review this change.

Thanks again Pieter, I have pushed this change to master.

> > Also, is this change relevant to released versions of OVS?
> 
> Ah yes. I believe it is relevant to the released version as well.
> 
> > If so, I think some backports will need to be prepared
> > as this patch does not apply cleanly to branch-2.10.
> > 
> 
> Thank you Simon, I'll prepare a backport for branch-2.10.

Patch

diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
index f7a175484..090662d54 100644
--- a/lib/netdev-tc-offloads.c
+++ b/lib/netdev-tc-offloads.c
@@ -1006,10 +1006,12 @@  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
             && (vid_mask || pcp_mask)) {
             if (vid_mask) {
                 flower.key.vlan_id[0] = vlan_tci_to_vid(key->vlans[0].tci);
+                flower.mask.vlan_id[0] = vlan_tci_to_vid(mask->vlans[0].tci);
                 VLOG_DBG_RL(&rl, "vlan_id[0]: %d\n", flower.key.vlan_id[0]);
             }
             if (pcp_mask) {
                 flower.key.vlan_prio[0] = vlan_tci_to_pcp(key->vlans[0].tci);
+                flower.mask.vlan_prio[0] = vlan_tci_to_pcp(mask->vlans[0].tci);
                 VLOG_DBG_RL(&rl, "vlan_prio[0]: %d\n",
                             flower.key.vlan_prio[0]);
             }
@@ -1035,10 +1037,12 @@  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
             && (vid_mask || pcp_mask)) {
             if (vid_mask) {
                 flower.key.vlan_id[1] = vlan_tci_to_vid(key->vlans[1].tci);
+                flower.mask.vlan_id[1] = vlan_tci_to_vid(mask->vlans[1].tci);
                 VLOG_DBG_RL(&rl, "vlan_id[1]: %d", flower.key.vlan_id[1]);
             }
             if (pcp_mask) {
                 flower.key.vlan_prio[1] = vlan_tci_to_pcp(key->vlans[1].tci);
+                flower.mask.vlan_prio[1] = vlan_tci_to_pcp(mask->vlans[1].tci);
                 VLOG_DBG_RL(&rl, "vlan_prio[1]: %d", flower.key.vlan_prio[1]);
             }
             flower.key.encap_eth_type[1] = flower.key.encap_eth_type[0];
diff --git a/lib/tc.c b/lib/tc.c
index adcf31604..ac8db6159 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -403,10 +403,12 @@  nl_parse_flower_vlan(struct nlattr **attrs, struct tc_flower *flower)
     if (attrs[TCA_FLOWER_KEY_VLAN_ID]) {
         flower->key.vlan_id[0] =
             nl_attr_get_u16(attrs[TCA_FLOWER_KEY_VLAN_ID]);
+        flower->mask.vlan_id[0] = 0xffff;
     }
     if (attrs[TCA_FLOWER_KEY_VLAN_PRIO]) {
         flower->key.vlan_prio[0] =
             nl_attr_get_u8(attrs[TCA_FLOWER_KEY_VLAN_PRIO]);
+        flower->mask.vlan_prio[0] = 0xff;
     }
 
     if (!attrs[TCA_FLOWER_KEY_VLAN_ETH_TYPE]) {
@@ -424,10 +426,12 @@  nl_parse_flower_vlan(struct nlattr **attrs, struct tc_flower *flower)
     if (attrs[TCA_FLOWER_KEY_CVLAN_ID]) {
         flower->key.vlan_id[1] =
             nl_attr_get_u16(attrs[TCA_FLOWER_KEY_CVLAN_ID]);
+        flower->mask.vlan_id[1] = 0xffff;
     }
     if (attrs[TCA_FLOWER_KEY_CVLAN_PRIO]) {
         flower->key.vlan_prio[1] =
             nl_attr_get_u8(attrs[TCA_FLOWER_KEY_CVLAN_PRIO]);
+        flower->mask.vlan_prio[1] = 0xff;
     }
 }
 
@@ -1789,9 +1793,11 @@  nl_msg_put_flower_options(struct ofpbuf *request, struct tc_flower *flower)
     }
 
     if (is_vlan) {
-        if (flower->key.vlan_id[0] || flower->key.vlan_prio[0]) {
+        if (flower->mask.vlan_id[0]) {
             nl_msg_put_u16(request, TCA_FLOWER_KEY_VLAN_ID,
                            flower->key.vlan_id[0]);
+        }
+        if (flower->mask.vlan_prio[0]) {
             nl_msg_put_u8(request, TCA_FLOWER_KEY_VLAN_PRIO,
                           flower->key.vlan_prio[0]);
         }
@@ -1801,9 +1807,11 @@  nl_msg_put_flower_options(struct ofpbuf *request, struct tc_flower *flower)
         }
 
         if (is_qinq) {
-            if (flower->key.vlan_id[1] || flower->key.vlan_prio[1]) {
+            if (flower->mask.vlan_id[1]) {
                 nl_msg_put_u16(request, TCA_FLOWER_KEY_CVLAN_ID,
                                flower->key.vlan_id[1]);
+            }
+            if (flower->mask.vlan_prio[1]) {
                 nl_msg_put_u8(request, TCA_FLOWER_KEY_CVLAN_PRIO,
                               flower->key.vlan_prio[1]);
             }