diff mbox

[ovs-dev,1/2] datapath: Avoid using stack larger than 1024.

Message ID 1499047080-89457-1-git-send-email-xiangxia.m.yue@gmail.com
State Accepted
Delegated to: Joe Stringer
Headers show

Commit Message

Tonghao Zhang July 3, 2017, 1:57 a.m. UTC 
Upstream commit:
    commit 9cc9a5cb176ccb4f2cda5ac34da5a659926f125f
    Author: Tonghao Zhang <xiangxia.m.yue@gmail.com>
    Date:   Thu Jun 29 17:27:44 2017 -0700

    datapath: Avoid using stack larger than 1024.

    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>
    Acked-by: Pravin B Shelar <pshelar@ovn.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>

Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
 datapath/datapath.c | 81 ++++++++++++++++++++++++++++++++++++++---------------
 1 file changed, 58 insertions(+), 23 deletions(-)

Comments

Joe Stringer July 21, 2017, 10:32 p.m. UTC | #1 
On 2 July 2017 at 18:57, Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> Upstream commit:
>     commit 9cc9a5cb176ccb4f2cda5ac34da5a659926f125f
>     Author: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>     Date:   Thu Jun 29 17:27:44 2017 -0700
>
>     datapath: Avoid using stack larger than 1024.
>
>     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>
>     Acked-by: Pravin B Shelar <pshelar@ovn.org>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>

Thanks for the backport!

I plan to roll this into a series that will bring the datapath pretty
much up to date, inserting in the same order as the upstream patches
were applied. I'll add my signoff and no further action should be
necessary from your part for this to land upstream to master.

Cheers,
Joe
diff mbox

Patch

diff --git a/datapath/datapath.c b/datapath/datapath.c
index c85029c..fb9f114 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -1100,6 +1100,58 @@  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. Before returning from
+ * the function, we reset the match->mask pointer. Because
+ * we should not to return match object with dangling reference
+ * to mask.
+ * */
+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)
+			goto 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)) {
+			error = PTR_ERR(*acts);
+			goto error;
+		}
+	}
+
+	/* On success, error is 0. */
+error:
+	match->mask = NULL;
+	return error;
+}
+
 static int ovs_flow_cmd_set(struct sk_buff *skb, struct genl_info *info)
 {
 	struct net *net = sock_net(skb->sk);
@@ -1107,7 +1159,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 +1170,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);