Netdev Archive on lore.kernel.org
 help / color / Atom feed
* Re: [PATCH net-next] net: openvswitch: Set OvS recirc_id from tc chain
@ 2019-08-18 16:00 Paul Blakey
  2019-08-19  6:20 ` Paul Blakey
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Paul Blakey @ 2019-08-18 16:00 UTC (permalink / raw)
  To: Pravin B Shelar, netdev, David S. Miller, Justin Pettit,
	Simon Horman, Marcelo Ricardo Leitner, Vlad Buslov, Paul Blakey
  Cc: Jiri Pirko, Roi Dayan, Yossi Kuperman, Rony Efraim, Oz Shlomo

What do you guys say about the following diff on top of the last one?
Use static key, and also have OVS_DP_CMD_SET command probe/enable the feature.

This will allow userspace to probe the feature, and selectivly enable it via the
OVS_DP_CMD_SET command.

Thansk,
Paul.


---
 include/uapi/linux/openvswitch.h |  3 +++
 net/openvswitch/datapath.c       | 29 +++++++++++++++++++++++++----
 net/openvswitch/datapath.h       |  2 ++
 net/openvswitch/flow.c           |  6 ++++--
 4 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index f271f1e..1887a45 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -123,6 +123,9 @@ struct ovs_vport_stats {
 /* Allow datapath to associate multiple Netlink PIDs to each vport */
 #define OVS_DP_F_VPORT_PIDS	(1 << 1)
 
+/* Allow tc offload recirc sharing */
+#define OVS_DP_F_TC_RECIRC_SHARING	(1 << 2)
+
 /* Fixed logical ports. */
 #define OVSP_LOCAL      ((__u32)0)
 
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 892287d..589b4f1 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1541,10 +1541,27 @@ static void ovs_dp_reset_user_features(struct sk_buff *skb, struct genl_info *in
 	dp->user_features = 0;
 }
 
-static void ovs_dp_change(struct datapath *dp, struct nlattr *a[])
+DEFINE_STATIC_KEY_FALSE(tc_recirc_sharing_support);
+
+static int ovs_dp_change(struct datapath *dp, struct nlattr *a[])
 {
+	u32 user_features;
+
 	if (a[OVS_DP_ATTR_USER_FEATURES])
-		dp->user_features = nla_get_u32(a[OVS_DP_ATTR_USER_FEATURES]);
+		user_features = nla_get_u32(a[OVS_DP_ATTR_USER_FEATURES]);
+
+#if !IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
+	if (user_features & OVS_DP_F_TC_RECIRC_SHARING)
+		return -EOPNOTSUPP;
+#endif
+	dp->user_features = user_features;
+
+	if (dp->user_features & OVS_DP_F_TC_RECIRC_SHARING)
+		static_branch_enable(&tc_recirc_sharing_support);
+	else
+		static_branch_disable(&tc_recirc_sharing_support);
+
+	return 0;
 }
 
 static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info)
@@ -1606,7 +1623,9 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info)
 	parms.port_no = OVSP_LOCAL;
 	parms.upcall_portids = a[OVS_DP_ATTR_UPCALL_PID];
 
-	ovs_dp_change(dp, a);
+	err = ovs_dp_change(dp, a);
+	if (err)
+		goto err_destroy_meters;
 
 	/* So far only local changes have been made, now need the lock. */
 	ovs_lock();
@@ -1732,7 +1751,9 @@ static int ovs_dp_cmd_set(struct sk_buff *skb, struct genl_info *info)
 	if (IS_ERR(dp))
 		goto err_unlock_free;
 
-	ovs_dp_change(dp, info->attrs);
+	err = ovs_dp_change(dp, info->attrs);
+	if (err)
+		goto err_unlock_free;
 
 	err = ovs_dp_cmd_fill_info(dp, reply, info->snd_portid,
 				   info->snd_seq, 0, OVS_DP_CMD_SET);
diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h
index 751d34a..81e85dd 100644
--- a/net/openvswitch/datapath.h
+++ b/net/openvswitch/datapath.h
@@ -218,6 +218,8 @@ static inline struct datapath *get_dp(struct net *net, int dp_ifindex)
 extern struct notifier_block ovs_dp_device_notifier;
 extern struct genl_family dp_vport_genl_family;
 
+DECLARE_STATIC_KEY_FALSE(tc_recirc_sharing_support);
+
 void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key);
 void ovs_dp_detach_port(struct vport *);
 int ovs_dp_upcall(struct datapath *, struct sk_buff *,
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 0287ead..c0ac7c9 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -853,8 +853,10 @@ int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info,
 	key->mac_proto = res;
 
 #if IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
-	tc_ext = skb_ext_find(skb, TC_SKB_EXT);
-	key->recirc_id = tc_ext ? tc_ext->chain : 0;
+	if (static_branch_unlikely(&tc_recirc_sharing_support)) {
+		tc_ext = skb_ext_find(skb, TC_SKB_EXT);
+		key->recirc_id = tc_ext ? tc_ext->chain : 0;
+	}
 #else
 	key->recirc_id = 0;
 #endif
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next] net: openvswitch: Set OvS recirc_id from tc chain
  2019-08-18 16:00 [PATCH net-next] net: openvswitch: Set OvS recirc_id from tc chain Paul Blakey
@ 2019-08-19  6:20 ` Paul Blakey
  2019-08-19 17:42 ` Marcelo Ricardo Leitner
  2019-08-20  5:52 ` Pravin Shelar
  2 siblings, 0 replies; 6+ messages in thread
From: Paul Blakey @ 2019-08-19  6:20 UTC (permalink / raw)
  To: Pravin B Shelar, netdev, David S. Miller, Justin Pettit,
	Simon Horman, Marcelo Ricardo Leitner, Vlad Buslov
  Cc: Jiri Pirko, Roi Dayan, Yossi Kuperman, Rony Efraim, Oz Shlomo


On 8/18/2019 7:00 PM, Paul Blakey wrote:
> What do you guys say about the following diff on top of the last one?
> Use static key, and also have OVS_DP_CMD_SET command probe/enable the feature.
>
> This will allow userspace to probe the feature, and selectivly enable it via the
> OVS_DP_CMD_SET command.
>
> Thansk,
> Paul.
>
>
> ---
>   include/uapi/linux/openvswitch.h |  3 +++
>   net/openvswitch/datapath.c       | 29 +++++++++++++++++++++++++----
>   net/openvswitch/datapath.h       |  2 ++
>   net/openvswitch/flow.c           |  6 ++++--
>   4 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
> index f271f1e..1887a45 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -123,6 +123,9 @@ struct ovs_vport_stats {
>   /* Allow datapath to associate multiple Netlink PIDs to each vport */
>   #define OVS_DP_F_VPORT_PIDS	(1 << 1)
>   
> +/* Allow tc offload recirc sharing */
> +#define OVS_DP_F_TC_RECIRC_SHARING	(1 << 2)
> +
>   /* Fixed logical ports. */
>   #define OVSP_LOCAL      ((__u32)0)
>   
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 892287d..589b4f1 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -1541,10 +1541,27 @@ static void ovs_dp_reset_user_features(struct sk_buff *skb, struct genl_info *in
>   	dp->user_features = 0;
>   }
>   
> -static void ovs_dp_change(struct datapath *dp, struct nlattr *a[])
> +DEFINE_STATIC_KEY_FALSE(tc_recirc_sharing_support);
> +
> +static int ovs_dp_change(struct datapath *dp, struct nlattr *a[])
>   {
> +	u32 user_features;
> +
>   	if (a[OVS_DP_ATTR_USER_FEATURES])
> -		dp->user_features = nla_get_u32(a[OVS_DP_ATTR_USER_FEATURES]);
> +		user_features = nla_get_u32(a[OVS_DP_ATTR_USER_FEATURES]);
> +
> +#if !IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
> +	if (user_features & OVS_DP_F_TC_RECIRC_SHARING)
> +		return -EOPNOTSUPP;
> +#endif
> +	dp->user_features = user_features;
> +
> +	if (dp->user_features & OVS_DP_F_TC_RECIRC_SHARING)
> +		static_branch_enable(&tc_recirc_sharing_support);
> +	else
> +		static_branch_disable(&tc_recirc_sharing_support);
> +
> +	return 0;
>   }
>   
>   static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info)
> @@ -1606,7 +1623,9 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct genl_info *info)
>   	parms.port_no = OVSP_LOCAL;
>   	parms.upcall_portids = a[OVS_DP_ATTR_UPCALL_PID];
>   
> -	ovs_dp_change(dp, a);
> +	err = ovs_dp_change(dp, a);
> +	if (err)
> +		goto err_destroy_meters;
>   
>   	/* So far only local changes have been made, now need the lock. */
>   	ovs_lock();
> @@ -1732,7 +1751,9 @@ static int ovs_dp_cmd_set(struct sk_buff *skb, struct genl_info *info)
>   	if (IS_ERR(dp))
>   		goto err_unlock_free;
>   
> -	ovs_dp_change(dp, info->attrs);
> +	err = ovs_dp_change(dp, info->attrs);
> +	if (err)
> +		goto err_unlock_free;
>   
>   	err = ovs_dp_cmd_fill_info(dp, reply, info->snd_portid,
>   				   info->snd_seq, 0, OVS_DP_CMD_SET);
> diff --git a/net/openvswitch/datapath.h b/net/openvswitch/datapath.h
> index 751d34a..81e85dd 100644
> --- a/net/openvswitch/datapath.h
> +++ b/net/openvswitch/datapath.h
> @@ -218,6 +218,8 @@ static inline struct datapath *get_dp(struct net *net, int dp_ifindex)
>   extern struct notifier_block ovs_dp_device_notifier;
>   extern struct genl_family dp_vport_genl_family;
>   
> +DECLARE_STATIC_KEY_FALSE(tc_recirc_sharing_support);
> +
>   void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key);
>   void ovs_dp_detach_port(struct vport *);
>   int ovs_dp_upcall(struct datapath *, struct sk_buff *,
> diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
> index 0287ead..c0ac7c9 100644
> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -853,8 +853,10 @@ int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info,
>   	key->mac_proto = res;
>   
>   #if IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
> -	tc_ext = skb_ext_find(skb, TC_SKB_EXT);
> -	key->recirc_id = tc_ext ? tc_ext->chain : 0;
> +	if (static_branch_unlikely(&tc_recirc_sharing_support)) {
> +		tc_ext = skb_ext_find(skb, TC_SKB_EXT);
> +		key->recirc_id = tc_ext ? tc_ext->chain : 0;
> +	}

Here should be

else

       key->recirc_id = 0

>   #else
>   	key->recirc_id = 0;
>   #endif

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next] net: openvswitch: Set OvS recirc_id from tc chain
  2019-08-18 16:00 [PATCH net-next] net: openvswitch: Set OvS recirc_id from tc chain Paul Blakey
  2019-08-19  6:20 ` Paul Blakey
@ 2019-08-19 17:42 ` Marcelo Ricardo Leitner
  2019-08-20  5:50   ` Pravin Shelar
  2019-08-20  5:52 ` Pravin Shelar
  2 siblings, 1 reply; 6+ messages in thread
From: Marcelo Ricardo Leitner @ 2019-08-19 17:42 UTC (permalink / raw)
  To: Paul Blakey
  Cc: Pravin B Shelar, netdev, David S. Miller, Justin Pettit,
	Simon Horman, Vlad Buslov, Jiri Pirko, Roi Dayan, Yossi Kuperman,
	Rony Efraim, Oz Shlomo

On Sun, Aug 18, 2019 at 07:00:59PM +0300, Paul Blakey wrote:
> What do you guys say about the following diff on top of the last one?
> Use static key, and also have OVS_DP_CMD_SET command probe/enable the feature.
> 
> This will allow userspace to probe the feature, and selectivly enable it via the
> OVS_DP_CMD_SET command.

I'm not convinced yet that we need something like this. Been
wondering, skb_ext_find() below is not that expensive if not in use.
It's just a bit check and that's it, it returns NULL.

And drivers will only be setting this if they have tc-offloading
enabled (assuming they won't be seeing it for chain 0 all the time).
On which case, with tc offloading, we need this in order to work
properly.

Is the bit checking really that worrysome?

> --- a/net/openvswitch/flow.c
> +++ b/net/openvswitch/flow.c
> @@ -853,8 +853,10 @@ int ovs_flow_key_extract(const struct ip_tunnel_info *tun_info,
>  	key->mac_proto = res;
>  
>  #if IS_ENABLED(CONFIG_NET_TC_SKB_EXT)
> -	tc_ext = skb_ext_find(skb, TC_SKB_EXT);
> -	key->recirc_id = tc_ext ? tc_ext->chain : 0;
> +	if (static_branch_unlikely(&tc_recirc_sharing_support)) {
> +		tc_ext = skb_ext_find(skb, TC_SKB_EXT);
> +		key->recirc_id = tc_ext ? tc_ext->chain : 0;
> +	}
>  #else
>  	key->recirc_id = 0;
>  #endif
> -- 
> 1.8.3.1
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next] net: openvswitch: Set OvS recirc_id from tc chain
  2019-08-19 17:42 ` Marcelo Ricardo Leitner
@ 2019-08-20  5:50   ` Pravin Shelar
  2019-08-20  7:08     ` Paul Blakey
  0 siblings, 1 reply; 6+ messages in thread
From: Pravin Shelar @ 2019-08-20  5:50 UTC (permalink / raw)
  To: Marcelo Ricardo Leitner
  Cc: Paul Blakey, Linux Kernel Network Developers, David S. Miller,
	Justin Pettit, Simon Horman, Vlad Buslov, Jiri Pirko, Roi Dayan,
	Yossi Kuperman, Rony Efraim, Oz Shlomo

On Mon, Aug 19, 2019 at 10:42 AM Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
>
> On Sun, Aug 18, 2019 at 07:00:59PM +0300, Paul Blakey wrote:
> > What do you guys say about the following diff on top of the last one?
> > Use static key, and also have OVS_DP_CMD_SET command probe/enable the feature.
> >
> > This will allow userspace to probe the feature, and selectivly enable it via the
> > OVS_DP_CMD_SET command.
>
> I'm not convinced yet that we need something like this. Been
> wondering, skb_ext_find() below is not that expensive if not in use.
> It's just a bit check and that's it, it returns NULL.
>
> And drivers will only be setting this if they have tc-offloading
> enabled (assuming they won't be seeing it for chain 0 all the time).
> On which case, with tc offloading, we need this in order to work
> properly.
>
> Is the bit checking really that worrysome?
>
Point is this would be completely unnecessary check for software only
cases, that is what static key is used for, when you have a feature in
datapath that is not used by majority of users. So I do not see any
downside of having this static key.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next] net: openvswitch: Set OvS recirc_id from tc chain
  2019-08-18 16:00 [PATCH net-next] net: openvswitch: Set OvS recirc_id from tc chain Paul Blakey
  2019-08-19  6:20 ` Paul Blakey
  2019-08-19 17:42 ` Marcelo Ricardo Leitner
@ 2019-08-20  5:52 ` Pravin Shelar
  2 siblings, 0 replies; 6+ messages in thread
From: Pravin Shelar @ 2019-08-20  5:52 UTC (permalink / raw)
  To: Paul Blakey
  Cc: Linux Kernel Network Developers, David S. Miller, Justin Pettit,
	Simon Horman, Marcelo Ricardo Leitner, Vlad Buslov, Jiri Pirko,
	Roi Dayan, Yossi Kuperman, Rony Efraim, Oz Shlomo

On Sun, Aug 18, 2019 at 9:01 AM Paul Blakey <paulb@mellanox.com> wrote:
>
> What do you guys say about the following diff on top of the last one?
> Use static key, and also have OVS_DP_CMD_SET command probe/enable the feature.
>
> This will allow userspace to probe the feature, and selectivly enable it via the
> OVS_DP_CMD_SET command.
>

This approach looks good to me.

Thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next] net: openvswitch: Set OvS recirc_id from tc chain
  2019-08-20  5:50   ` Pravin Shelar
@ 2019-08-20  7:08     ` Paul Blakey
  0 siblings, 0 replies; 6+ messages in thread
From: Paul Blakey @ 2019-08-20  7:08 UTC (permalink / raw)
  To: Pravin Shelar, Marcelo Ricardo Leitner
  Cc: Linux Kernel Network Developers, David S. Miller, Justin Pettit,
	Simon Horman, Vlad Buslov, Jiri Pirko, Roi Dayan, Yossi Kuperman,
	Rony Efraim, Oz Shlomo


On 8/20/2019 8:50 AM, Pravin Shelar wrote:
> On Mon, Aug 19, 2019 at 10:42 AM Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
>> On Sun, Aug 18, 2019 at 07:00:59PM +0300, Paul Blakey wrote:
>>> What do you guys say about the following diff on top of the last one?
>>> Use static key, and also have OVS_DP_CMD_SET command probe/enable the feature.
>>>
>>> This will allow userspace to probe the feature, and selectivly enable it via the
>>> OVS_DP_CMD_SET command.
>> I'm not convinced yet that we need something like this. Been
>> wondering, skb_ext_find() below is not that expensive if not in use.
>> It's just a bit check and that's it, it returns NULL.
>>
>> And drivers will only be setting this if they have tc-offloading
>> enabled (assuming they won't be seeing it for chain 0 all the time).
>> On which case, with tc offloading, we need this in order to work
>> properly.
>>
>> Is the bit checking really that worrysome?
>>
> Point is this would be completely unnecessary check for software only
> cases, that is what static key is used for, when you have a feature in
> datapath that is not used by majority of users. So I do not see any
> downside of having this static key.

Also it's good that I can now probe kernel support via OVS_DP_CMD_SET.

Since user can disable the kernel support or even use a different 
version of the datapath module, it would silently bug on misses from tc.

Now I can set the recirc sharing feature in user_feature via SET op, and 
check the returned new user_features or an error code.

If no error, and user_features contains the flag (won't be there for 
older DPs that won't return error), then DP supports this feature and

kernel config is enabled.




^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-18 16:00 [PATCH net-next] net: openvswitch: Set OvS recirc_id from tc chain Paul Blakey
2019-08-19  6:20 ` Paul Blakey
2019-08-19 17:42 ` Marcelo Ricardo Leitner
2019-08-20  5:50   ` Pravin Shelar
2019-08-20  7:08     ` Paul Blakey
2019-08-20  5:52 ` Pravin Shelar

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox