* [PATCH v2] datapath: Avoid using stack larger than 1024.
@ 2017-06-28 2:29 Tonghao Zhang
2017-06-28 20:35 ` Pravin Shelar
0 siblings, 1 reply; 2+ messages in thread
From: Tonghao Zhang @ 2017-06-28 2:29 UTC (permalink / raw)
To: netdev; +Cc: pshelar, gvrose8192, Tonghao Zhang
When compiling OvS-master on 4.4.0-81 kernel,
there is a warning:
CC [M] /root/ovs/datapath/linux/datapath.o
/root/ovs/datapath/linux/datapath.c: In function
‘ovs_flow_cmd_set’:
/root/ovs/datapath/linux/datapath.c:1221:1: warning:
the frame size of 1040 bytes is larger than 1024 bytes
[-Wframe-larger-than=]
This patch factors out match-init and action-copy to avoid
"Wframe-larger-than=1024" warning. Because mask is only
used to get actions, we new a function to save some
stack space.
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
datapath/datapath.c | 73 ++++++++++++++++++++++++++++++++++++-----------------
1 file changed, 50 insertions(+), 23 deletions(-)
diff --git a/datapath/datapath.c b/datapath/datapath.c
index c85029c..fdbe314 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -1100,6 +1100,50 @@ static struct sw_flow_actions *get_flow_actions(struct net *net,
return acts;
}
+/* Factor out match-init and action-copy to avoid
+ * "Wframe-larger-than=1024" warning. Because mask is only
+ * used to get actions, we new a function to save some
+ * stack space.
+ *
+ * If there are not key and action attrs, we return 0
+ * directly. In the case, the caller will also not use the
+ * match as before. If there is action attr, we try to get
+ * actions and save them to *acts.
+ * */
+static int ovs_nla_init_match_and_action(struct net *net,
+ struct sw_flow_match *match,
+ struct sw_flow_key *key,
+ struct nlattr **a,
+ struct sw_flow_actions **acts,
+ bool log)
+{
+ struct sw_flow_mask mask;
+ int error = 0;
+
+ if (a[OVS_FLOW_ATTR_KEY]) {
+ ovs_match_init(match, key, true, &mask);
+ error = ovs_nla_get_match(net, match, a[OVS_FLOW_ATTR_KEY],
+ a[OVS_FLOW_ATTR_MASK], log);
+ if (error)
+ return error;
+ }
+
+ if (a[OVS_FLOW_ATTR_ACTIONS]) {
+ if (!a[OVS_FLOW_ATTR_KEY]) {
+ OVS_NLERR(log,
+ "Flow key attribute not present in set flow.");
+ return -EINVAL;
+ }
+
+ *acts = get_flow_actions(net, a[OVS_FLOW_ATTR_ACTIONS], key,
+ &mask, log);
+ if (IS_ERR(*acts))
+ return PTR_ERR(*acts);
+ }
+
+ return 0;
+}
+
static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
{
struct net *net = sock_net(skb->sk);
@@ -1107,7 +1151,6 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
struct ovs_header *ovs_header = info->userhdr;
struct sw_flow_key key;
struct sw_flow *flow;
- struct sw_flow_mask mask;
struct sk_buff *reply = NULL;
struct datapath *dp;
struct sw_flow_actions *old_acts = NULL, *acts = NULL;
@@ -1119,34 +1162,18 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
bool ufid_present;
ufid_present = ovs_nla_get_ufid(&sfid, a[OVS_FLOW_ATTR_UFID], log);
- if (a[OVS_FLOW_ATTR_KEY]) {
- ovs_match_init(&match, &key, true, &mask);
- error = ovs_nla_get_match(net, &match, a[OVS_FLOW_ATTR_KEY],
- a[OVS_FLOW_ATTR_MASK], log);
- } else if (!ufid_present) {
+ if (!a[OVS_FLOW_ATTR_KEY] && !ufid_present) {
OVS_NLERR(log,
"Flow set message rejected, Key attribute missing.");
- error = -EINVAL;
+ return -EINVAL;
}
+
+ error = ovs_nla_init_match_and_action(net, &match, &key, a,
+ &acts, log);
if (error)
goto error;
- /* Validate actions. */
- if (a[OVS_FLOW_ATTR_ACTIONS]) {
- if (!a[OVS_FLOW_ATTR_KEY]) {
- OVS_NLERR(log,
- "Flow key attribute not present in set flow.");
- error = -EINVAL;
- goto error;
- }
-
- acts = get_flow_actions(net, a[OVS_FLOW_ATTR_ACTIONS], &key,
- &mask, log);
- if (IS_ERR(acts)) {
- error = PTR_ERR(acts);
- goto error;
- }
-
+ if (acts) {
/* Can allocate before locking if have acts. */
reply = ovs_flow_cmd_alloc_info(acts, &sfid, info, false,
ufid_flags);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH v2] datapath: Avoid using stack larger than 1024.
2017-06-28 2:29 [PATCH v2] datapath: Avoid using stack larger than 1024 Tonghao Zhang
@ 2017-06-28 20:35 ` Pravin Shelar
0 siblings, 0 replies; 2+ messages in thread
From: Pravin Shelar @ 2017-06-28 20:35 UTC (permalink / raw)
To: Tonghao Zhang; +Cc: Linux Kernel Network Developers, Greg Rose
On Tue, Jun 27, 2017 at 7:29 PM, Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> When compiling OvS-master on 4.4.0-81 kernel,
> there is a warning:
>
> CC [M] /root/ovs/datapath/linux/datapath.o
> /root/ovs/datapath/linux/datapath.c: In function
> ‘ovs_flow_cmd_set’:
> /root/ovs/datapath/linux/datapath.c:1221:1: warning:
> the frame size of 1040 bytes is larger than 1024 bytes
> [-Wframe-larger-than=]
>
> This patch factors out match-init and action-copy to avoid
> "Wframe-larger-than=1024" warning. Because mask is only
> used to get actions, we new a function to save some
> stack space.
>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
> datapath/datapath.c | 73 ++++++++++++++++++++++++++++++++++++-----------------
> 1 file changed, 50 insertions(+), 23 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index c85029c..fdbe314 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -1100,6 +1100,50 @@ static struct sw_flow_actions *get_flow_actions(struct net *net,
> return acts;
> }
>
> +/* Factor out match-init and action-copy to avoid
> + * "Wframe-larger-than=1024" warning. Because mask is only
> + * used to get actions, we new a function to save some
> + * stack space.
> + *
> + * If there are not key and action attrs, we return 0
> + * directly. In the case, the caller will also not use the
> + * match as before. If there is action attr, we try to get
> + * actions and save them to *acts.
> + * */
> +static int ovs_nla_init_match_and_action(struct net *net,
> + struct sw_flow_match *match,
> + struct sw_flow_key *key,
> + struct nlattr **a,
> + struct sw_flow_actions **acts,
> + bool log)
> +{
> + struct sw_flow_mask mask;
> + int error = 0;
> +
> + if (a[OVS_FLOW_ATTR_KEY]) {
> + ovs_match_init(match, key, true, &mask);
> + error = ovs_nla_get_match(net, match, a[OVS_FLOW_ATTR_KEY],
> + a[OVS_FLOW_ATTR_MASK], log);
> + if (error)
> + return error;
> + }
> +
> + if (a[OVS_FLOW_ATTR_ACTIONS]) {
> + if (!a[OVS_FLOW_ATTR_KEY]) {
> + OVS_NLERR(log,
> + "Flow key attribute not present in set flow.");
> + return -EINVAL;
> + }
> +
> + *acts = get_flow_actions(net, a[OVS_FLOW_ATTR_ACTIONS], key,
> + &mask, log);
> + if (IS_ERR(*acts))
> + return PTR_ERR(*acts);
> + }
> +
> + return 0;
> +}
> +
> static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
> {
> struct net *net = sock_net(skb->sk);
> @@ -1107,7 +1151,6 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
> struct ovs_header *ovs_header = info->userhdr;
> struct sw_flow_key key;
> struct sw_flow *flow;
> - struct sw_flow_mask mask;
> struct sk_buff *reply = NULL;
> struct datapath *dp;
> struct sw_flow_actions *old_acts = NULL, *acts = NULL;
> @@ -1119,34 +1162,18 @@ static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
> bool ufid_present;
>
> ufid_present = ovs_nla_get_ufid(&sfid, a[OVS_FLOW_ATTR_UFID], log);
> - if (a[OVS_FLOW_ATTR_KEY]) {
> - ovs_match_init(&match, &key, true, &mask);
> - error = ovs_nla_get_match(net, &match, a[OVS_FLOW_ATTR_KEY],
> - a[OVS_FLOW_ATTR_MASK], log);
> - } else if (!ufid_present) {
> + if (!a[OVS_FLOW_ATTR_KEY] && !ufid_present) {
> OVS_NLERR(log,
> "Flow set message rejected, Key attribute missing.");
> - error = -EINVAL;
> + return -EINVAL;
> }
> +
> + error = ovs_nla_init_match_and_action(net, &match, &key, a,
> + &acts, log);
This looks good. But it is returning match object with dangling
reference to mask. It is fine for now as it is not referenced outside
of the function for now. But it is not pretty. We could reset the mask
pointer before returning from the function. and write comment
explaining it.
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2017-06-28 20:36 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-28 2:29 [PATCH v2] datapath: Avoid using stack larger than 1024 Tonghao Zhang
2017-06-28 20:35 ` Pravin Shelar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.