diff mbox series

[ovs-dev,V3,09/12] dpif-netdev: Don't use zero flow mark

Message ID 20200621111924.12397-10-elibr@mellanox.com
State Changes Requested
Headers show
Series netdev datapath offload: Support IPv6 and VXLAN encap | expand

Commit Message

Eli Britstein June 21, 2020, 11:19 a.m. UTC
Zero flow mark is used to indicate the HW to remove the mark. A packet
marked with zero mark is received in SW without a mark at all, so it
cannot be used as a valid mark. Change the pool range to fix it.

Fixes: 241bad15d99a ("dpif-netdev: associate flow with a mark id")
Signed-off-by: Eli Britstein <elibr@mellanox.com>
Reviewed-by: Roni Bar Yanai <roniba@mellanox.com>
---
 lib/dpif-netdev.c    |  4 ++--
 tests/dpif-netdev.at | 18 +++++++++---------
 2 files changed, 11 insertions(+), 11 deletions(-)

Comments

Ilya Maximets June 28, 2020, 11:47 p.m. UTC | #1
On 6/21/20 1:19 PM, Eli Britstein wrote:
> Zero flow mark is used to indicate the HW to remove the mark. A packet
> marked with zero mark is received in SW without a mark at all, so it
> cannot be used as a valid mark. Change the pool range to fix it.
> 
> Fixes: 241bad15d99a ("dpif-netdev: associate flow with a mark id")
> Signed-off-by: Eli Britstein <elibr@mellanox.com>
> Reviewed-by: Roni Bar Yanai <roniba@mellanox.com>
> ---
>  lib/dpif-netdev.c    |  4 ++--
>  tests/dpif-netdev.at | 18 +++++++++---------
>  2 files changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 87068803e..57565802a 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -2150,7 +2150,7 @@ dp_netdev_pmd_find_dpcls(struct dp_netdev_pmd_thread *pmd,
>  }
>  
>  #define MAX_FLOW_MARK       (UINT32_MAX - 1)
> -#define INVALID_FLOW_MARK   (UINT32_MAX)
> +#define INVALID_FLOW_MARK   0
>  
>  struct megaflow_to_mark_data {
>      const struct cmap_node node;
> @@ -2176,7 +2176,7 @@ flow_mark_alloc(void)
>  
>      if (!flow_mark.pool) {
>          /* Haven't initiated yet, do it here */

It might be good to add information to the comment why zero is not used here.

> -        flow_mark.pool = id_pool_create(0, MAX_FLOW_MARK);
> +        flow_mark.pool = id_pool_create(1, MAX_FLOW_MARK);
>      }
>  
>      if (id_pool_alloc_id(flow_mark.pool, &mark)) {
> diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
> index 9c0a42d00..21f0c8d24 100644
> --- a/tests/dpif-netdev.at
> +++ b/tests/dpif-netdev.at
> @@ -393,7 +393,7 @@ skb_priority(0),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),recirc
>     # Check that flow successfully offloaded.
>     OVS_WAIT_UNTIL([grep "succeed to add netdev flow" ovs-vswitchd.log])
>     AT_CHECK([filter_hw_flow_install < ovs-vswitchd.log | strip_xout], [0], [dnl
> -p1: flow put[[create]]: flow match: recirc_id=0,eth,ip,in_port=1,vlan_tci=0x0000,nw_frag=no, mark: 0
> +p1: flow put[[create]]: flow match: recirc_id=0,eth,ip,in_port=1,vlan_tci=0x0000,nw_frag=no, mark: 1
>  ])
>     # Check that datapath flow installed successfully.
>     AT_CHECK([filter_flow_install < ovs-vswitchd.log | strip_xout], [0], [dnl
> @@ -404,7 +404,7 @@ recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), a
>  
>     # Check for succesfull packet matching with installed offloaded flow.
>     AT_CHECK([filter_hw_packet_netdev_dummy < ovs-vswitchd.log | strip_xout], [0], [dnl
> -p1: packet: ip,vlan_tci=0x0000,dl_src=00:06:07:08:09:0a,dl_dst=00:01:02:03:04:05,nw_src=127.0.0.1,nw_dst=127.0.0.1,nw_proto=0,nw_tos=0,nw_ecn=0,nw_ttl=64 matches with flow: recirc_id=0,eth,ip,vlan_tci=0x0000,nw_frag=no with mark: 0
> +p1: packet: ip,vlan_tci=0x0000,dl_src=00:06:07:08:09:0a,dl_dst=00:01:02:03:04:05,nw_src=127.0.0.1,nw_dst=127.0.0.1,nw_proto=0,nw_tos=0,nw_ecn=0,nw_ttl=64 matches with flow: recirc_id=0,eth,ip,vlan_tci=0x0000,nw_frag=no with mark: 1
>  ])
>  
>     ovs-appctl revalidator/wait
> @@ -421,7 +421,7 @@ recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), p
>     # Check that flow successfully deleted from HW.
>     OVS_WAIT_UNTIL([grep "succeed to delete netdev flow" ovs-vswitchd.log])
>     AT_CHECK([filter_hw_flow_del < ovs-vswitchd.log | strip_xout], [0], [dnl
> -p1: flow del: mark: 0
> +p1: flow del: mark: 1
>  ])
>     OVS_VSWITCHD_STOP
>     AT_CLEANUP])
> @@ -460,7 +460,7 @@ packet_type(ns=0,id=0),eth(src=00:06:07:08:09:0a,dst=00:01:02:03:04:05),eth_type
>     # Check that flow successfully offloaded.
>     OVS_WAIT_UNTIL([grep "succeed to add netdev flow" ovs-vswitchd.log])
>     AT_CHECK([filter_hw_flow_install < ovs-vswitchd.log | strip_xout], [0], [dnl
> -p1: flow put[[create]]: flow match: recirc_id=0,eth,udp,in_port=1,dl_vlan=99,dl_vlan_pcp=7,nw_src=127.0.0.1,nw_frag=no,tp_dst=82, mark: 0
> +p1: flow put[[create]]: flow match: recirc_id=0,eth,udp,in_port=1,dl_vlan=99,dl_vlan_pcp=7,nw_src=127.0.0.1,nw_frag=no,tp_dst=82, mark: 1
>  ])
>     # Check that datapath flow installed successfully.
>     AT_CHECK([filter_flow_install < ovs-vswitchd.log | strip_xout], [0], [dnl
> @@ -472,7 +472,7 @@ recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x8100),vlan(vid=99,pcp=
>     # Check for succesfull packet matching with installed offloaded flow.
>     AT_CHECK([filter_hw_packet_netdev_dummy < ovs-vswitchd.log | strip_xout], [0], [dnl
>  p1: packet: udp,dl_vlan=99,dl_vlan_pcp=7,vlan_tci1=0x0000,dl_src=00:06:07:08:09:0a,dl_dst=00:01:02:03:04:05,nw_src=127.0.0.1,nw_dst=127.0.0.1,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=81,tp_dst=82 dnl
> -matches with flow: recirc_id=0,eth,udp,dl_vlan=99,dl_vlan_pcp=7,nw_src=127.0.0.1,nw_frag=no,tp_dst=82 with mark: 0
> +matches with flow: recirc_id=0,eth,udp,dl_vlan=99,dl_vlan_pcp=7,nw_src=127.0.0.1,nw_frag=no,tp_dst=82 with mark: 1
>  ])
>  
>     ovs-appctl revalidator/wait
> @@ -490,7 +490,7 @@ packets:1, bytes:64, used:0.0s, actions:set(ipv4(src=192.168.0.7)),set(udp(dst=3
>     # Check that flow successfully deleted from HW.
>     OVS_WAIT_UNTIL([grep "succeed to delete netdev flow" ovs-vswitchd.log])
>     AT_CHECK([filter_hw_flow_del < ovs-vswitchd.log | strip_xout], [0], [dnl
> -p1: flow del: mark: 0
> +p1: flow del: mark: 1
>  ])
>  
>     # Check that ip address and udp port were correctly modified in output packets.
> @@ -537,7 +537,7 @@ packet_type(ns=0,id=0),eth(src=00:06:07:08:09:0a,dst=00:01:02:03:04:05),eth_type
>     # Check that flow successfully offloaded.
>     OVS_WAIT_UNTIL([grep "succeed to add netdev flow" ovs-vswitchd.log])
>     AT_CHECK([filter_hw_flow_install < ovs-vswitchd.log | strip_xout], [0], [dnl
> -p1: flow put[[create]]: flow match: recirc_id=0,eth,arp,in_port=1,dl_vlan=99,dl_vlan_pcp=7, mark: 0
> +p1: flow put[[create]]: flow match: recirc_id=0,eth,arp,in_port=1,dl_vlan=99,dl_vlan_pcp=7, mark: 1
>  ])
>     # Check that datapath flow installed successfully.
>     AT_CHECK([filter_flow_install < ovs-vswitchd.log | strip_xout], [0], [dnl
> @@ -549,7 +549,7 @@ recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x8100),vlan(vid=99,pcp=
>     # Check for succesfull packet matching with installed offloaded flow.
>     AT_CHECK([filter_hw_packet_netdev_dummy < ovs-vswitchd.log | strip_xout], [0], [dnl
>  p1: packet: arp,dl_vlan=99,dl_vlan_pcp=7,vlan_tci1=0x0000,dl_src=00:06:07:08:09:0a,dl_dst=00:01:02:03:04:05,arp_spa=127.0.0.1,arp_tpa=127.0.0.1,arp_op=1,arp_sha=00:0b:0c:0d:0e:0f,arp_tha=00:00:00:00:00:00 dnl
> -matches with flow: recirc_id=0,eth,arp,dl_vlan=99,dl_vlan_pcp=7 with mark: 0
> +matches with flow: recirc_id=0,eth,arp,dl_vlan=99,dl_vlan_pcp=7 with mark: 1
>  ])
>  
>     ovs-appctl revalidator/wait
> @@ -567,7 +567,7 @@ packets:1, bytes:64, used:0.0s, actions:pop_vlan,push_vlan(vid=11,pcp=7),1
>     # Check that flow successfully deleted from HW.
>     OVS_WAIT_UNTIL([grep "succeed to delete netdev flow" ovs-vswitchd.log])
>     AT_CHECK([filter_hw_flow_del < ovs-vswitchd.log | strip_xout], [0], [dnl
> -p1: flow del: mark: 0
> +p1: flow del: mark: 1
>  ])
>  
>     # Check that VLAN ID was correctly modified in output packets.
>
Eli Britstein June 29, 2020, 12:10 p.m. UTC | #2
On 6/29/2020 2:47 AM, Ilya Maximets wrote:
> On 6/21/20 1:19 PM, Eli Britstein wrote:
>> Zero flow mark is used to indicate the HW to remove the mark. A packet
>> marked with zero mark is received in SW without a mark at all, so it
>> cannot be used as a valid mark. Change the pool range to fix it.
>>
>> Fixes: 241bad15d99a ("dpif-netdev: associate flow with a mark id")
>> Signed-off-by: Eli Britstein <elibr@mellanox.com>
>> Reviewed-by: Roni Bar Yanai <roniba@mellanox.com>
>> ---
>>   lib/dpif-netdev.c    |  4 ++--
>>   tests/dpif-netdev.at | 18 +++++++++---------
>>   2 files changed, 11 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 87068803e..57565802a 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -2150,7 +2150,7 @@ dp_netdev_pmd_find_dpcls(struct dp_netdev_pmd_thread *pmd,
>>   }
>>   
>>   #define MAX_FLOW_MARK       (UINT32_MAX - 1)
>> -#define INVALID_FLOW_MARK   (UINT32_MAX)
>> +#define INVALID_FLOW_MARK   0
>>   
>>   struct megaflow_to_mark_data {
>>       const struct cmap_node node;
>> @@ -2176,7 +2176,7 @@ flow_mark_alloc(void)
>>   
>>       if (!flow_mark.pool) {
>>           /* Haven't initiated yet, do it here */
> It might be good to add information to the comment why zero is not used here.
I thought in the commit message is enough. I'll add a comment here too.
>
>> -        flow_mark.pool = id_pool_create(0, MAX_FLOW_MARK);
>> +        flow_mark.pool = id_pool_create(1, MAX_FLOW_MARK);
>>       }
>>   
>>       if (id_pool_alloc_id(flow_mark.pool, &mark)) {
>>
diff mbox series

Patch

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 87068803e..57565802a 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -2150,7 +2150,7 @@  dp_netdev_pmd_find_dpcls(struct dp_netdev_pmd_thread *pmd,
 }
 
 #define MAX_FLOW_MARK       (UINT32_MAX - 1)
-#define INVALID_FLOW_MARK   (UINT32_MAX)
+#define INVALID_FLOW_MARK   0
 
 struct megaflow_to_mark_data {
     const struct cmap_node node;
@@ -2176,7 +2176,7 @@  flow_mark_alloc(void)
 
     if (!flow_mark.pool) {
         /* Haven't initiated yet, do it here */
-        flow_mark.pool = id_pool_create(0, MAX_FLOW_MARK);
+        flow_mark.pool = id_pool_create(1, MAX_FLOW_MARK);
     }
 
     if (id_pool_alloc_id(flow_mark.pool, &mark)) {
diff --git a/tests/dpif-netdev.at b/tests/dpif-netdev.at
index 9c0a42d00..21f0c8d24 100644
--- a/tests/dpif-netdev.at
+++ b/tests/dpif-netdev.at
@@ -393,7 +393,7 @@  skb_priority(0),skb_mark(0),ct_state(0),ct_zone(0),ct_mark(0),ct_label(0),recirc
    # Check that flow successfully offloaded.
    OVS_WAIT_UNTIL([grep "succeed to add netdev flow" ovs-vswitchd.log])
    AT_CHECK([filter_hw_flow_install < ovs-vswitchd.log | strip_xout], [0], [dnl
-p1: flow put[[create]]: flow match: recirc_id=0,eth,ip,in_port=1,vlan_tci=0x0000,nw_frag=no, mark: 0
+p1: flow put[[create]]: flow match: recirc_id=0,eth,ip,in_port=1,vlan_tci=0x0000,nw_frag=no, mark: 1
 ])
    # Check that datapath flow installed successfully.
    AT_CHECK([filter_flow_install < ovs-vswitchd.log | strip_xout], [0], [dnl
@@ -404,7 +404,7 @@  recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), a
 
    # Check for succesfull packet matching with installed offloaded flow.
    AT_CHECK([filter_hw_packet_netdev_dummy < ovs-vswitchd.log | strip_xout], [0], [dnl
-p1: packet: ip,vlan_tci=0x0000,dl_src=00:06:07:08:09:0a,dl_dst=00:01:02:03:04:05,nw_src=127.0.0.1,nw_dst=127.0.0.1,nw_proto=0,nw_tos=0,nw_ecn=0,nw_ttl=64 matches with flow: recirc_id=0,eth,ip,vlan_tci=0x0000,nw_frag=no with mark: 0
+p1: packet: ip,vlan_tci=0x0000,dl_src=00:06:07:08:09:0a,dl_dst=00:01:02:03:04:05,nw_src=127.0.0.1,nw_dst=127.0.0.1,nw_proto=0,nw_tos=0,nw_ecn=0,nw_ttl=64 matches with flow: recirc_id=0,eth,ip,vlan_tci=0x0000,nw_frag=no with mark: 1
 ])
 
    ovs-appctl revalidator/wait
@@ -421,7 +421,7 @@  recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(frag=no), p
    # Check that flow successfully deleted from HW.
    OVS_WAIT_UNTIL([grep "succeed to delete netdev flow" ovs-vswitchd.log])
    AT_CHECK([filter_hw_flow_del < ovs-vswitchd.log | strip_xout], [0], [dnl
-p1: flow del: mark: 0
+p1: flow del: mark: 1
 ])
    OVS_VSWITCHD_STOP
    AT_CLEANUP])
@@ -460,7 +460,7 @@  packet_type(ns=0,id=0),eth(src=00:06:07:08:09:0a,dst=00:01:02:03:04:05),eth_type
    # Check that flow successfully offloaded.
    OVS_WAIT_UNTIL([grep "succeed to add netdev flow" ovs-vswitchd.log])
    AT_CHECK([filter_hw_flow_install < ovs-vswitchd.log | strip_xout], [0], [dnl
-p1: flow put[[create]]: flow match: recirc_id=0,eth,udp,in_port=1,dl_vlan=99,dl_vlan_pcp=7,nw_src=127.0.0.1,nw_frag=no,tp_dst=82, mark: 0
+p1: flow put[[create]]: flow match: recirc_id=0,eth,udp,in_port=1,dl_vlan=99,dl_vlan_pcp=7,nw_src=127.0.0.1,nw_frag=no,tp_dst=82, mark: 1
 ])
    # Check that datapath flow installed successfully.
    AT_CHECK([filter_flow_install < ovs-vswitchd.log | strip_xout], [0], [dnl
@@ -472,7 +472,7 @@  recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x8100),vlan(vid=99,pcp=
    # Check for succesfull packet matching with installed offloaded flow.
    AT_CHECK([filter_hw_packet_netdev_dummy < ovs-vswitchd.log | strip_xout], [0], [dnl
 p1: packet: udp,dl_vlan=99,dl_vlan_pcp=7,vlan_tci1=0x0000,dl_src=00:06:07:08:09:0a,dl_dst=00:01:02:03:04:05,nw_src=127.0.0.1,nw_dst=127.0.0.1,nw_tos=0,nw_ecn=0,nw_ttl=64,tp_src=81,tp_dst=82 dnl
-matches with flow: recirc_id=0,eth,udp,dl_vlan=99,dl_vlan_pcp=7,nw_src=127.0.0.1,nw_frag=no,tp_dst=82 with mark: 0
+matches with flow: recirc_id=0,eth,udp,dl_vlan=99,dl_vlan_pcp=7,nw_src=127.0.0.1,nw_frag=no,tp_dst=82 with mark: 1
 ])
 
    ovs-appctl revalidator/wait
@@ -490,7 +490,7 @@  packets:1, bytes:64, used:0.0s, actions:set(ipv4(src=192.168.0.7)),set(udp(dst=3
    # Check that flow successfully deleted from HW.
    OVS_WAIT_UNTIL([grep "succeed to delete netdev flow" ovs-vswitchd.log])
    AT_CHECK([filter_hw_flow_del < ovs-vswitchd.log | strip_xout], [0], [dnl
-p1: flow del: mark: 0
+p1: flow del: mark: 1
 ])
 
    # Check that ip address and udp port were correctly modified in output packets.
@@ -537,7 +537,7 @@  packet_type(ns=0,id=0),eth(src=00:06:07:08:09:0a,dst=00:01:02:03:04:05),eth_type
    # Check that flow successfully offloaded.
    OVS_WAIT_UNTIL([grep "succeed to add netdev flow" ovs-vswitchd.log])
    AT_CHECK([filter_hw_flow_install < ovs-vswitchd.log | strip_xout], [0], [dnl
-p1: flow put[[create]]: flow match: recirc_id=0,eth,arp,in_port=1,dl_vlan=99,dl_vlan_pcp=7, mark: 0
+p1: flow put[[create]]: flow match: recirc_id=0,eth,arp,in_port=1,dl_vlan=99,dl_vlan_pcp=7, mark: 1
 ])
    # Check that datapath flow installed successfully.
    AT_CHECK([filter_flow_install < ovs-vswitchd.log | strip_xout], [0], [dnl
@@ -549,7 +549,7 @@  recirc_id(0),in_port(1),packet_type(ns=0,id=0),eth_type(0x8100),vlan(vid=99,pcp=
    # Check for succesfull packet matching with installed offloaded flow.
    AT_CHECK([filter_hw_packet_netdev_dummy < ovs-vswitchd.log | strip_xout], [0], [dnl
 p1: packet: arp,dl_vlan=99,dl_vlan_pcp=7,vlan_tci1=0x0000,dl_src=00:06:07:08:09:0a,dl_dst=00:01:02:03:04:05,arp_spa=127.0.0.1,arp_tpa=127.0.0.1,arp_op=1,arp_sha=00:0b:0c:0d:0e:0f,arp_tha=00:00:00:00:00:00 dnl
-matches with flow: recirc_id=0,eth,arp,dl_vlan=99,dl_vlan_pcp=7 with mark: 0
+matches with flow: recirc_id=0,eth,arp,dl_vlan=99,dl_vlan_pcp=7 with mark: 1
 ])
 
    ovs-appctl revalidator/wait
@@ -567,7 +567,7 @@  packets:1, bytes:64, used:0.0s, actions:pop_vlan,push_vlan(vid=11,pcp=7),1
    # Check that flow successfully deleted from HW.
    OVS_WAIT_UNTIL([grep "succeed to delete netdev flow" ovs-vswitchd.log])
    AT_CHECK([filter_hw_flow_del < ovs-vswitchd.log | strip_xout], [0], [dnl
-p1: flow del: mark: 0
+p1: flow del: mark: 1
 ])
 
    # Check that VLAN ID was correctly modified in output packets.