* 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 related [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-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
* 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
end of thread, other threads:[~2019-08-20 7:08 UTC | newest]
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).