net: rds: Don't allocate rds_sock on stack

Message ID 1408926755-12277-1-git-send-email-broonie@kernel.org
State New
Headers show

Commit Message

Mark Brown Aug. 25, 2014, 12:32 a.m.
From: Mark Brown <broonie@linaro.org>

struct rds_sock is rather large ausing the following warning in an ARM
allmodconfig:

net/rds/iw_rdma.c:200:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]

Fix this by dynamically allocating struct rds_sock in rds_iw_update_cm_id
instead of allocating it on the stack.

Signed-off-by: Mark Brown <broonie@linaro.org>
---
 net/rds/iw_rdma.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

David Miller Aug. 25, 2014, 7:57 p.m. | #1
From: Mark Brown <broonie@kernel.org>
Date: Sun, 24 Aug 2014 19:32:35 -0500

> From: Mark Brown <broonie@linaro.org>
> 
> struct rds_sock is rather large ausing the following warning in an ARM
> allmodconfig:
> 
> net/rds/iw_rdma.c:200:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 
> Fix this by dynamically allocating struct rds_sock in rds_iw_update_cm_id
> instead of allocating it on the stack.
> 
> Signed-off-by: Mark Brown <broonie@linaro.org>

I'd like you to fix this differently.  Creating pseudo instances of
objects, and partially initializing it, just to satisfy an interface
is always a really bad sign.

Create a key structure argument for rds_iw_get_device() and initialize that
and pass it in instead, update the other caller similarly.

Thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Aug. 26, 2014, 6:54 a.m. | #2
On Mon, Aug 25, 2014 at 12:57:40PM -0700, David Miller wrote:
> From: Mark Brown <broonie@kernel.org>

> > From: Mark Brown <broonie@linaro.org>

> > struct rds_sock is rather large ausing the following warning in an ARM
> > allmodconfig:

> > net/rds/iw_rdma.c:200:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]

> > Fix this by dynamically allocating struct rds_sock in rds_iw_update_cm_id
> > instead of allocating it on the stack.

> > Signed-off-by: Mark Brown <broonie@linaro.org>

> I'd like you to fix this differently.  Creating pseudo instances of
> objects, and partially initializing it, just to satisfy an interface
> is always a really bad sign.

> Create a key structure argument for rds_iw_get_device() and initialize that
> and pass it in instead, update the other caller similarly.

I agree that the existing code looks like it could be improved even more
but please bear in mind that I'm just looking for a clean build (we've
got less than 20 warnings in allmodconfig including staging at the
minute) rather than actively working on this code in particular - I've
no ability to do more than build testing here.
David Miller Aug. 26, 2014, 3:29 p.m. | #3
From: Mark Brown <broonie@kernel.org>
Date: Tue, 26 Aug 2014 07:54:09 +0100

> On Mon, Aug 25, 2014 at 12:57:40PM -0700, David Miller wrote:
>> From: Mark Brown <broonie@kernel.org>
> 
>> > From: Mark Brown <broonie@linaro.org>
> 
>> > struct rds_sock is rather large ausing the following warning in an ARM
>> > allmodconfig:
> 
>> > net/rds/iw_rdma.c:200:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]
> 
>> > Fix this by dynamically allocating struct rds_sock in rds_iw_update_cm_id
>> > instead of allocating it on the stack.
> 
>> > Signed-off-by: Mark Brown <broonie@linaro.org>
> 
>> I'd like you to fix this differently.  Creating pseudo instances of
>> objects, and partially initializing it, just to satisfy an interface
>> is always a really bad sign.
> 
>> Create a key structure argument for rds_iw_get_device() and initialize that
>> and pass it in instead, update the other caller similarly.
> 
> I agree that the existing code looks like it could be improved even more
> but please bear in mind that I'm just looking for a clean build (we've
> got less than 20 warnings in allmodconfig including staging at the
> minute) rather than actively working on this code in particular - I've
> no ability to do more than build testing here.

I understand that, but please fix this bug properly.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Aug. 27, 2014, 10:46 a.m. | #4
On Tue, Aug 26, 2014 at 08:29:06AM -0700, David Miller wrote:
> From: Mark Brown <broonie@kernel.org>

> > I agree that the existing code looks like it could be improved even more
> > but please bear in mind that I'm just looking for a clean build (we've
> > got less than 20 warnings in allmodconfig including staging at the
> > minute) rather than actively working on this code in particular - I've
> > no ability to do more than build testing here.

> I understand that, but please fix this bug properly.

Please set the expectation among the networking developers that their
code should compile cleanly on all architectures; currently the
networking code (mostly drivers rather than the core) is the single
biggest source of warnings outside of staging and if we are requring
things like this then that presents a substantial barrier to others
contributing to addressing the warnings.

Patch

diff --git a/net/rds/iw_rdma.c b/net/rds/iw_rdma.c
index a817705..cee5daa 100644
--- a/net/rds/iw_rdma.c
+++ b/net/rds/iw_rdma.c
@@ -180,22 +180,28 @@  int rds_iw_update_cm_id(struct rds_iw_device *rds_iwdev, struct rdma_cm_id *cm_i
 {
 	struct sockaddr_in *src_addr, *dst_addr;
 	struct rds_iw_device *rds_iwdev_old;
-	struct rds_sock rs;
+	struct rds_sock *rs;
 	struct rdma_cm_id *pcm_id;
 	int rc;
 
+	rs = kzalloc(sizeof(*rs), GFP_KERNEL);
+	if (!rs)
+		return -ENOMEM;
+
 	src_addr = (struct sockaddr_in *)&cm_id->route.addr.src_addr;
 	dst_addr = (struct sockaddr_in *)&cm_id->route.addr.dst_addr;
 
-	rs.rs_bound_addr = src_addr->sin_addr.s_addr;
-	rs.rs_bound_port = src_addr->sin_port;
-	rs.rs_conn_addr = dst_addr->sin_addr.s_addr;
-	rs.rs_conn_port = dst_addr->sin_port;
+	rs->rs_bound_addr = src_addr->sin_addr.s_addr;
+	rs->rs_bound_port = src_addr->sin_port;
+	rs->rs_conn_addr = dst_addr->sin_addr.s_addr;
+	rs->rs_conn_port = dst_addr->sin_port;
 
-	rc = rds_iw_get_device(&rs, &rds_iwdev_old, &pcm_id);
+	rc = rds_iw_get_device(rs, &rds_iwdev_old, &pcm_id);
 	if (rc)
 		rds_iw_remove_cm_id(rds_iwdev, cm_id);
 
+	kfree(rs);
+
 	return rds_iw_add_cm_id(rds_iwdev, cm_id);
 }