[PATCH net-next v2 1/1] net: openvswitch: Use on stack sw_flow_key in ovs_packet_cmd_execute

[PATCH net-next v2 1/1] net: openvswitch: Use on stack sw_flow_key in ovs_packet_cmd_execute

[Eddy Tao]

Use on stack sw_flow_key in ovs_packet_cmd_execute

Reason: As key function in slow-path, ovs_packet_cmd_execute and
        ovs_flow_cmd_new allocate transient memory for sw_flow
        and frees it at the end of function.
        The procedure is not efficient in 2 aspects
        1. actuall sw_flow_key is what the function need
        2. free/alloc involves kmem_cache operations
        when system under frequent slow path operation

        Existing code in ovs_flow_cmd_new/set/get use stack
        to store sw_flow_mask and sw_flow_key deliberately

Performance benefit:
        ovs_packet_cmd_execute efficiency improved
        Avoid 2 calls to kmem_cache alloc
        Avoid memzero of 200 bytes
        6% less instructions

Testing topology
            +-----+
      nic1--|     |--nic1
      nic2--|     |--nic2
VM1(16cpus) | ovs |   VM2(16 cpus)
      nic3--|4cpus|--nic3
      nic4--|     |--nic4
            +-----+
   2 threads on each vnic with affinity set on client side

netperf -H $peer -p $((port+$i)) -t UDP_RR  -l 60 -- -R 1 -r 8K,8K
netperf -H $peer -p $((port+$i)) -t TCP_RR  -l 60 -- -R 1 -r 120,240
netperf -H $peer -p $((port+$i)) -t TCP_CRR -l 60 -- -R 1 -r 120,240

Before the fix
      Mode Iterations   Variance    Average
    UDP_RR         10      %1.31      46724
    TCP_RR         10      %6.26      77188
   TCP_CRR         10      %0.10      20505
UDP_STREAM         10      %4.55      19907
TCP_STREAM         10      %9.93      28942

After the fix
      Mode Iterations   Variance    Average
    UDP_RR         10      %1.51      49097
    TCP_RR         10      %5.58      78540
   TCP_CRR         10      %0.14      20542
UDP_STREAM         10     %11.17      22532
TCP_STREAM         10     %11.14      28579

Signed-off-by: Eddy Tao <taoyuan_eddy@hotmail.com>
---
 V1 -> V2: Further reduce memory usage by using sw_flow_key instead
           of sw_flow, revise description of change and provide data

 net/openvswitch/datapath.c | 30 +++++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index fcee6012293b..ae3146d51079 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -596,8 +596,7 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
 	struct nlattr **a = info->attrs;
 	struct sw_flow_actions *acts;
 	struct sk_buff *packet;
-	struct sw_flow *flow;
-	struct sw_flow_actions *sf_acts;
+	struct sw_flow_key key;
 	struct datapath *dp;
 	struct vport *input_vport;
 	u16 mru = 0;
@@ -636,24 +635,20 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	/* Build an sw_flow for sending this packet. */
-	flow = ovs_flow_alloc();
-	err = PTR_ERR(flow);
-	if (IS_ERR(flow))
-		goto err_kfree_skb;
+	memset(&key, 0, sizeof(key));
 
 	err = ovs_flow_key_extract_userspace(net, a[OVS_PACKET_ATTR_KEY],
-					     packet, &flow->key, log);
+					     packet, &key, log);
 	if (err)
-		goto err_flow_free;
+		goto err_kfree_skb;
 
 	err = ovs_nla_copy_actions(net, a[OVS_PACKET_ATTR_ACTIONS],
-				   &flow->key, &acts, log);
+				   &key, &acts, log);
 	if (err)
-		goto err_flow_free;
+		goto err_kfree_skb;
 
-	rcu_assign_pointer(flow->sf_acts, acts);
-	packet->priority = flow->key.phy.priority;
-	packet->mark = flow->key.phy.skb_mark;
+	packet->priority = key.phy.priority;
+	packet->mark = key.phy.skb_mark;
 
 	rcu_read_lock();
 	dp = get_dp_rcu(net, ovs_header->dp_ifindex);
@@ -661,7 +656,7 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
 	if (!dp)
 		goto err_unlock;
 
-	input_vport = ovs_vport_rcu(dp, flow->key.phy.in_port);
+	input_vport = ovs_vport_rcu(dp, key.phy.in_port);
 	if (!input_vport)
 		input_vport = ovs_vport_rcu(dp, OVSP_LOCAL);
 
@@ -670,20 +665,17 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
 
 	packet->dev = input_vport->dev;
 	OVS_CB(packet)->input_vport = input_vport;
-	sf_acts = rcu_dereference(flow->sf_acts);
 
 	local_bh_disable();
-	err = ovs_execute_actions(dp, packet, sf_acts, &flow->key);
+	err = ovs_execute_actions(dp, packet, acts, &key);
 	local_bh_enable();
 	rcu_read_unlock();
 
-	ovs_flow_free(flow, false);
+	ovs_nla_free_flow_actions(acts);
 	return err;
 
 err_unlock:
 	rcu_read_unlock();
-err_flow_free:
-	ovs_flow_free(flow, false);
 err_kfree_skb:
 	kfree_skb(packet);
 err:
-- 
2.27.0

Re: [PATCH net-next v2 1/1] net: openvswitch: Use on stack sw_flow_key in ovs_packet_cmd_execute

[Eddy Tao]

Sorry, there is a typo in the mail, i will resend shortly, please ignore 
it for now

On 2023/2/23 20:21, Eddy Tao wrote:
> Use on stack sw_flow_key in ovs_packet_cmd_execute
>
> Reason: As key function in slow-path, ovs_packet_cmd_execute and
>          ovs_flow_cmd_new allocate transient memory for sw_flow
>          and frees it at the end of function.
>          The procedure is not efficient in 2 aspects
>          1. actuall sw_flow_key is what the function need
>          2. free/alloc involves kmem_cache operations
>          when system under frequent slow path operation
>
>          Existing code in ovs_flow_cmd_new/set/get use stack
>          to store sw_flow_mask and sw_flow_key deliberately
>
> Performance benefit:
>          ovs_packet_cmd_execute efficiency improved
>          Avoid 2 calls to kmem_cache alloc
>          Avoid memzero of 200 bytes
>          6% less instructions
>
> Testing topology
>              +-----+
>        nic1--|     |--nic1
>        nic2--|     |--nic2
> VM1(16cpus) | ovs |   VM2(16 cpus)
>        nic3--|4cpus|--nic3
>        nic4--|     |--nic4
>              +-----+
>     2 threads on each vnic with affinity set on client side
>
> netperf -H $peer -p $((port+$i)) -t UDP_RR  -l 60 -- -R 1 -r 8K,8K
> netperf -H $peer -p $((port+$i)) -t TCP_RR  -l 60 -- -R 1 -r 120,240
> netperf -H $peer -p $((port+$i)) -t TCP_CRR -l 60 -- -R 1 -r 120,240
>
> Before the fix
>        Mode Iterations   Variance    Average
>      UDP_RR         10      %1.31      46724
>      TCP_RR         10      %6.26      77188
>     TCP_CRR         10      %0.10      20505
> UDP_STREAM         10      %4.55      19907
> TCP_STREAM         10      %9.93      28942
>
> After the fix
>        Mode Iterations   Variance    Average
>      UDP_RR         10      %1.51      49097
>      TCP_RR         10      %5.58      78540
>     TCP_CRR         10      %0.14      20542
> UDP_STREAM         10     %11.17      22532
> TCP_STREAM         10     %11.14      28579
>
> Signed-off-by: Eddy Tao <taoyuan_eddy@hotmail.com>
> ---
>   V1 -> V2: Further reduce memory usage by using sw_flow_key instead
>             of sw_flow, revise description of change and provide data
>
>   net/openvswitch/datapath.c | 30 +++++++++++-------------------
>   1 file changed, 11 insertions(+), 19 deletions(-)
>
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index fcee6012293b..ae3146d51079 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -596,8 +596,7 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
>   	struct nlattr **a = info->attrs;
>   	struct sw_flow_actions *acts;
>   	struct sk_buff *packet;
> -	struct sw_flow *flow;
> -	struct sw_flow_actions *sf_acts;
> +	struct sw_flow_key key;
>   	struct datapath *dp;
>   	struct vport *input_vport;
>   	u16 mru = 0;
> @@ -636,24 +635,20 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
>   	}
>   
>   	/* Build an sw_flow for sending this packet. */
> -	flow = ovs_flow_alloc();
> -	err = PTR_ERR(flow);
> -	if (IS_ERR(flow))
> -		goto err_kfree_skb;
> +	memset(&key, 0, sizeof(key));
>   
>   	err = ovs_flow_key_extract_userspace(net, a[OVS_PACKET_ATTR_KEY],
> -					     packet, &flow->key, log);
> +					     packet, &key, log);
>   	if (err)
> -		goto err_flow_free;
> +		goto err_kfree_skb;
>   
>   	err = ovs_nla_copy_actions(net, a[OVS_PACKET_ATTR_ACTIONS],
> -				   &flow->key, &acts, log);
> +				   &key, &acts, log);
>   	if (err)
> -		goto err_flow_free;
> +		goto err_kfree_skb;
>   
> -	rcu_assign_pointer(flow->sf_acts, acts);
> -	packet->priority = flow->key.phy.priority;
> -	packet->mark = flow->key.phy.skb_mark;
> +	packet->priority = key.phy.priority;
> +	packet->mark = key.phy.skb_mark;
>   
>   	rcu_read_lock();
>   	dp = get_dp_rcu(net, ovs_header->dp_ifindex);
> @@ -661,7 +656,7 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
>   	if (!dp)
>   		goto err_unlock;
>   
> -	input_vport = ovs_vport_rcu(dp, flow->key.phy.in_port);
> +	input_vport = ovs_vport_rcu(dp, key.phy.in_port);
>   	if (!input_vport)
>   		input_vport = ovs_vport_rcu(dp, OVSP_LOCAL);
>   
> @@ -670,20 +665,17 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
>   
>   	packet->dev = input_vport->dev;
>   	OVS_CB(packet)->input_vport = input_vport;
> -	sf_acts = rcu_dereference(flow->sf_acts);
>   
>   	local_bh_disable();
> -	err = ovs_execute_actions(dp, packet, sf_acts, &flow->key);
> +	err = ovs_execute_actions(dp, packet, acts, &key);
>   	local_bh_enable();
>   	rcu_read_unlock();
>   
> -	ovs_flow_free(flow, false);
> +	ovs_nla_free_flow_actions(acts);
>   	return err;
>   
>   err_unlock:
>   	rcu_read_unlock();
> -err_flow_free:
> -	ovs_flow_free(flow, false);
>   err_kfree_skb:
>   	kfree_skb(packet);
>   err:

Re: [PATCH net-next v2 1/1] net: openvswitch: Use on stack sw_flow_key in ovs_packet_cmd_execute

[Simon Horman]

On Thu, Feb 23, 2023 at 08:24:50PM +0800, Eddy Tao wrote:
> Sorry, there is a typo in the mail, i will resend shortly, please ignore it
> for now


net-next is now closed.

You'll need to repost this patch after v6.3-rc1 has been tagged.
Or post it as an RFC.

Ref: https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

Re: [PATCH net-next v2 1/1] net: openvswitch: Use on stack sw_flow_key in ovs_packet_cmd_execute

[Paolo Abeni]

On Thu, 2023-02-23 at 20:24 +0800, Eddy Tao wrote:
> Sorry, there is a typo in the mail, i will resend shortly,

please, don't do that.

# Form letter - net-next is closed

The merge window for v6.3 has begun and therefore net-next is closed
for new drivers, features, code refactoring and optimizations.
We are currently accepting bug fixes only.

Please repost when net-next reopens after Mar 6th.

RFC patches sent for review only are obviously welcome at any time.

Re: [PATCH net-next v2 1/1] net: openvswitch: Use on stack sw_flow_key in ovs_packet_cmd_execute

[缘 陶]

Sure, will redo the post when window open
Have a great day
eddy

________________________________________
From: Paolo Abeni <pabeni@redhat.com>
Sent: Thursday, February 23, 2023 12:41
To: Eddy Tao; netdev@vger.kernel.org
Cc: Pravin B Shelar; David S. Miller; Eric Dumazet; Jakub Kicinski; dev@openvswitch.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next v2 1/1] net: openvswitch: Use on stack sw_flow_key in ovs_packet_cmd_execute

On Thu, 2023-02-23 at 20:24 +0800, Eddy Tao wrote:
> Sorry, there is a typo in the mail, i will resend shortly,

please, don't do that.

# Form letter - net-next is closed

The merge window for v6.3 has begun and therefore net-next is closed
for new drivers, features, code refactoring and optimizations.
We are currently accepting bug fixes only.

Please repost when net-next reopens after Mar 6th.

RFC patches sent for review only are obviously welcome at any time.