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 | expand |
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.
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.
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.
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]); }