From: Tim Gardner <hidden> Date: 2013-03-10 15:39:51
rpcrdma_register_default_external() is several frames into
the call stack which goes deeper yet. You run the risk of stack
corruption by declaring such a large automatic variable,
so dynamically allocate the array of 'struct ib_phys_buf' objects in
order to silence the frame-larger-than warning.
net/sunrpc/xprtrdma/verbs.c: In function 'rpcrdma_register_default_external':
net/sunrpc/xprtrdma/verbs.c:1774:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]
gcc version 4.6.3
Cc: Trond Myklebust <redacted>
Cc: "J. Bruce Fields" <redacted>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Tom Tucker <tom-/Yg/VP3ZvrM@public.gmane.org>
Cc: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Shani Michaeli <shanim-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Tim Gardner <redacted>
---
net/sunrpc/xprtrdma/verbs.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Tom Tucker <hidden> Date: 2013-03-10 17:48:13
This is the result of 2773395b34883fe54418de188733a63bb38e0ad6. Steve
might want to weigh in on this since it was done for performance reasons.
Tom
On 3/10/13 10:39 AM, Tim Gardner wrote:
quoted hunk
rpcrdma_register_default_external() is several frames into
the call stack which goes deeper yet. You run the risk of stack
corruption by declaring such a large automatic variable,
so dynamically allocate the array of 'struct ib_phys_buf' objects in
order to silence the frame-larger-than warning.
net/sunrpc/xprtrdma/verbs.c: In function 'rpcrdma_register_default_external':
net/sunrpc/xprtrdma/verbs.c:1774:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]
gcc version 4.6.3
Cc: Trond Myklebust <redacted>
Cc: "J. Bruce Fields" <redacted>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Tom Tucker <tom-/Yg/VP3ZvrM@public.gmane.org>
Cc: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Shani Michaeli <shanim-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Tim Gardner <redacted>
---
net/sunrpc/xprtrdma/verbs.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: J. Bruce Fields <hidden> Date: 2013-03-10 20:28:45
On Sun, Mar 10, 2013 at 09:39:13AM -0600, Tim Gardner wrote:
quoted hunk
rpcrdma_register_default_external() is several frames into
the call stack which goes deeper yet. You run the risk of stack
corruption by declaring such a large automatic variable,
so dynamically allocate the array of 'struct ib_phys_buf' objects in
order to silence the frame-larger-than warning.
net/sunrpc/xprtrdma/verbs.c: In function 'rpcrdma_register_default_external':
net/sunrpc/xprtrdma/verbs.c:1774:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]
gcc version 4.6.3
Cc: Trond Myklebust <redacted>
Cc: "J. Bruce Fields" <redacted>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Tom Tucker <tom-/Yg/VP3ZvrM@public.gmane.org>
Cc: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Shani Michaeli <shanim-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Tim Gardner <redacted>
---
net/sunrpc/xprtrdma/verbs.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
Have you checked that this occurs in a context where allocations are OK?
Checking very quickly through the callers I can't see any spinlocks or
anything, but I also don't see any other allocations.
Assuming this is just in rpciod context.... Trond's the authority, but
I think we generally try to avoid allocations here, or make them
GFP_NOFS if we must.
Would it be possible to allocate this array as part of the rpcrdma_req?
--b.
quoted hunk
+ if (!ipb)
+ return -ENOMEM;
+
if (*nsegs > RPCRDMA_MAX_DATA_SEGS)
*nsegs = RPCRDMA_MAX_DATA_SEGS;
for (len = 0, i = 0; i < *nsegs;) {
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Tim Gardner <hidden> Date: 2013-03-11 17:37:55
rpcrdma_register_default_external() is several frames into the call stack which
goes deeper yet. You run the risk of stack corruption by declaring such a large
automatic variable, so move the array of 'struct ib_phys_buf' objects into the
requestor structure 'struct rpcrdma_req' (which is dynamically allocated) in
order to silence the frame-larger-than warning.
net/sunrpc/xprtrdma/verbs.c: In function 'rpcrdma_register_default_external':
net/sunrpc/xprtrdma/verbs.c:1774:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]
gcc version 4.6.3
Cc: Trond Myklebust <redacted>
Cc: "J. Bruce Fields" <redacted>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Tom Tucker <tom-/Yg/VP3ZvrM@public.gmane.org>
Cc: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Shani Michaeli <shanim-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Tim Gardner <redacted>
---
v1 - Use kmalloc() to dynamically allocate and free the array of 'struct
ib_phys_buf' objects
v2 - Move the array of 'struct ib_phys_buf' objects into struct rpcrdma_req
and pass this request down through rpcrdma_register_external() and
rpcrdma_register_default_external(). This is less overhead then using
kmalloc() and requires no extra error checking as the allocation burden is
shifted to the transport client.
net/sunrpc/xprtrdma/rpc_rdma.c | 2 +-
net/sunrpc/xprtrdma/verbs.c | 11 ++++++-----
net/sunrpc/xprtrdma/xprt_rdma.h | 3 ++-
3 files changed, 9 insertions(+), 7 deletions(-)
@@ -192,6 +192,7 @@ struct rpcrdma_req {structib_sgerl_send_iov[4];/* for active requests */structib_sgerl_iov;/* for posting */structib_mr*rl_handle;/* handle for mem in rl_iov */+structib_phys_bufrl_ipb[RPCRDMA_MAX_DATA_SEGS];/* temp work array */charrl_base[MAX_RPCRDMAHDR];/* start of actual buffer */__u32rl_xdr_buf[0];/* start of returned rpc rq_buffer */};
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: J. Bruce Fields <hidden> Date: 2013-03-11 18:14:58
On Mon, Mar 11, 2013 at 11:37:27AM -0600, Tim Gardner wrote:
rpcrdma_register_default_external() is several frames into the call stack which
goes deeper yet. You run the risk of stack corruption by declaring such a large
automatic variable, so move the array of 'struct ib_phys_buf' objects into the
requestor structure 'struct rpcrdma_req' (which is dynamically allocated) in
order to silence the frame-larger-than warning.
net/sunrpc/xprtrdma/verbs.c: In function 'rpcrdma_register_default_external':
net/sunrpc/xprtrdma/verbs.c:1774:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]
gcc version 4.6.3
Cc: Trond Myklebust <redacted>
Cc: "J. Bruce Fields" <redacted>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Tom Tucker <tom-/Yg/VP3ZvrM@public.gmane.org>
Cc: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Shani Michaeli <shanim-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Tim Gardner <redacted>
---
v1 - Use kmalloc() to dynamically allocate and free the array of 'struct
ib_phys_buf' objects
v2 - Move the array of 'struct ib_phys_buf' objects into struct rpcrdma_req
and pass this request down through rpcrdma_register_external() and
rpcrdma_register_default_external(). This is less overhead then using
kmalloc() and requires no extra error checking as the allocation burden is
shifted to the transport client.
Oh good--so that works, and the req is the right place to put this? How
are you testing this?
(Just want to make it clear: I'm *not* an expert on the rdma code, so my
suggestion to put this in the rpcrdma_req was a suggestion for something
to look into, not a claim that it's correct.)
--b.
@@ -192,6 +192,7 @@ struct rpcrdma_req {structib_sgerl_send_iov[4];/* for active requests */structib_sgerl_iov;/* for posting */structib_mr*rl_handle;/* handle for mem in rl_iov */+structib_phys_bufrl_ipb[RPCRDMA_MAX_DATA_SEGS];/* temp work array */charrl_base[MAX_RPCRDMAHDR];/* start of actual buffer */__u32rl_xdr_buf[0];/* start of returned rpc rq_buffer */};
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Tim Gardner <hidden> Date: 2013-03-11 18:51:56
On 03/11/2013 12:14 PM, J. Bruce Fields wrote:
<snip>
quoted
v2 - Move the array of 'struct ib_phys_buf' objects into struct rpcrdma_req
and pass this request down through rpcrdma_register_external() and
rpcrdma_register_default_external(). This is less overhead then using
kmalloc() and requires no extra error checking as the allocation burden is
shifted to the transport client.
Oh good--so that works, and the req is the right place to put this? How
are you testing this?
(Just want to make it clear: I'm *not* an expert on the rdma code, so my
suggestion to put this in the rpcrdma_req was a suggestion for something
to look into, not a claim that it's correct.)
Just compile tested so far. Incidentally, I've been through the call stack:
call_transmit
xprt_transmit
xprt->ops->send_request(task)
xprt_rdma_send_request
rpcrdma_marshal_req
rpcrdma_create_chunks
rpcrdma_register_external
rpcrdma_register_default_external
It appears that the context for kmalloc() should be fine unless there is
a spinlock held around call_transmit() (which seems unlikely).
rtg
--
Tim Gardner tim.gardner-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: J. Bruce Fields <hidden> Date: 2013-03-11 19:15:54
On Mon, Mar 11, 2013 at 12:51:44PM -0600, Tim Gardner wrote:
On 03/11/2013 12:14 PM, J. Bruce Fields wrote:
<snip>
quoted
quoted
v2 - Move the array of 'struct ib_phys_buf' objects into struct rpcrdma_req
and pass this request down through rpcrdma_register_external() and
rpcrdma_register_default_external(). This is less overhead then using
kmalloc() and requires no extra error checking as the allocation burden is
shifted to the transport client.
Oh good--so that works, and the req is the right place to put this? How
are you testing this?
(Just want to make it clear: I'm *not* an expert on the rdma code, so my
suggestion to put this in the rpcrdma_req was a suggestion for something
to look into, not a claim that it's correct.)
Just compile tested so far. Incidentally, I've been through the call stack:
call_transmit
xprt_transmit
xprt->ops->send_request(task)
xprt_rdma_send_request
rpcrdma_marshal_req
rpcrdma_create_chunks
rpcrdma_register_external
rpcrdma_register_default_external
It appears that the context for kmalloc() should be fine unless there is
a spinlock held around call_transmit() (which seems unlikely).
Right, though I think it shouldn't be GFP_KERNEL--looks like writes
could wait on it.
In any case, the embedding-in-rpcrdma_req solution does look cleaner if
that's correct (e.g. if we can be sure there won't be two simultaneous
users of that array).
--b.
On Mon, 2013-03-11 at 15:15 -0400, J. Bruce Fields wrote:
On Mon, Mar 11, 2013 at 12:51:44PM -0600, Tim Gardner wrote:
quoted
On 03/11/2013 12:14 PM, J. Bruce Fields wrote:
<snip>
quoted
quoted
v2 - Move the array of 'struct ib_phys_buf' objects into struct rpcrdma_req
and pass this request down through rpcrdma_register_external() and
rpcrdma_register_default_external(). This is less overhead then using
kmalloc() and requires no extra error checking as the allocation burden is
shifted to the transport client.
Oh good--so that works, and the req is the right place to put this? How
are you testing this?
(Just want to make it clear: I'm *not* an expert on the rdma code, so my
suggestion to put this in the rpcrdma_req was a suggestion for something
to look into, not a claim that it's correct.)
Just compile tested so far. Incidentally, I've been through the call stack:
call_transmit
xprt_transmit
xprt->ops->send_request(task)
xprt_rdma_send_request
rpcrdma_marshal_req
rpcrdma_create_chunks
rpcrdma_register_external
rpcrdma_register_default_external
It appears that the context for kmalloc() should be fine unless there is
a spinlock held around call_transmit() (which seems unlikely).
Right, though I think it shouldn't be GFP_KERNEL--looks like writes
could wait on it.
Nothing inside the RPC client should be using anything heavier than
GFP_NOWAIT (unless done at setup).
In any case, the embedding-in-rpcrdma_req solution does look cleaner if
that's correct (e.g. if we can be sure there won't be two simultaneous
users of that array).
Putting it in the rpcrdma_req means that you have one copy per transport
slot. Why not rather put it in the rpcrdma_xprt?
AFAICS you only need this array at transmit time for registering memory
for RDMA, at which time the transport XPRT_LOCK guarantees that nobody
else is competing for these resources.
--
Trond Myklebust
Linux NFS client maintainer
NetApp
Trond.Myklebust-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org
www.netapp.com
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: J. Bruce Fields <hidden> Date: 2013-03-11 20:00:23
On Mon, Mar 11, 2013 at 07:48:51PM +0000, Myklebust, Trond wrote:
On Mon, 2013-03-11 at 15:15 -0400, J. Bruce Fields wrote:
quoted
On Mon, Mar 11, 2013 at 12:51:44PM -0600, Tim Gardner wrote:
quoted
On 03/11/2013 12:14 PM, J. Bruce Fields wrote:
<snip>
quoted
quoted
v2 - Move the array of 'struct ib_phys_buf' objects into struct rpcrdma_req
and pass this request down through rpcrdma_register_external() and
rpcrdma_register_default_external(). This is less overhead then using
kmalloc() and requires no extra error checking as the allocation burden is
shifted to the transport client.
Oh good--so that works, and the req is the right place to put this? How
are you testing this?
(Just want to make it clear: I'm *not* an expert on the rdma code, so my
suggestion to put this in the rpcrdma_req was a suggestion for something
to look into, not a claim that it's correct.)
Just compile tested so far. Incidentally, I've been through the call stack:
call_transmit
xprt_transmit
xprt->ops->send_request(task)
xprt_rdma_send_request
rpcrdma_marshal_req
rpcrdma_create_chunks
rpcrdma_register_external
rpcrdma_register_default_external
It appears that the context for kmalloc() should be fine unless there is
a spinlock held around call_transmit() (which seems unlikely).
Right, though I think it shouldn't be GFP_KERNEL--looks like writes
could wait on it.
Nothing inside the RPC client should be using anything heavier than
GFP_NOWAIT (unless done at setup).
quoted
In any case, the embedding-in-rpcrdma_req solution does look cleaner if
that's correct (e.g. if we can be sure there won't be two simultaneous
users of that array).
Putting it in the rpcrdma_req means that you have one copy per transport
slot. Why not rather put it in the rpcrdma_xprt?
AFAICS you only need this array at transmit time for registering memory
for RDMA, at which time the transport XPRT_LOCK guarantees that nobody
else is competing for these resources.
Oh, good. If that works, Steve might want to look back at how that
array size was chosen? I seem to recall there being some compromise due
to this array being on the stack, and that there might have been some
performance advantage to increasing it further, but I can't find the bug
right now.... (And I might be misremembering.)
--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: J. Bruce Fields <hidden> Date: 2013-03-11 21:25:42
On Mon, Mar 11, 2013 at 03:15:08PM -0600, Tim Gardner wrote:
rpcrdma_register_default_external() is several frames into the call stack which
goes deeper yet. You run the risk of stack corruption by declaring such a large
automatic variable, so move the array of 'struct ib_phys_buf' objects into the
transport structure 'struct rpcrdma_xprt' (which is dynamically allocated) in
order to silence the frame-larger-than warning. Access to each struct
rpcrdma_xprt is serialized by XPRT_LOCKED in xprt_reserve_xprt(), so there is
no danger of multiple accessors to the array of struct ib_phys_buf objects.
net/sunrpc/xprtrdma/verbs.c: In function 'rpcrdma_register_default_external':
net/sunrpc/xprtrdma/verbs.c:1774:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]
gcc version 4.6.3
Cc: Trond Myklebust <redacted>
Cc: "J. Bruce Fields" <redacted>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Tom Tucker <tom-/Yg/VP3ZvrM@public.gmane.org>
Cc: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Shani Michaeli <shanim-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Tim Gardner <redacted>
---
v1 - Use kmalloc() to dynamically allocate and free the array of 'struct
ib_phys_buf' objects
v2 - Move the array of 'struct ib_phys_buf' objects into struct rpcrdma_req
and pass this request down through rpcrdma_register_external() and
rpcrdma_register_default_external(). This is less overhead then using
kmalloc() and requires no extra error checking as the allocation burden is
shifted to the transport client.
v3 - Move the array of 'struct ib_phys_buf' objects into struct rpcrdma_xprt.
Pass a pointer to this transport structure into rpcrdma_register_default_external().
This is less overhead then using kmalloc() and requires no extra error checking
as the allocation burden is shifted to the transport client.
Looks good to me; wish we could get it tested....
In future if we do decide to also increase the size of that array we may
need to allocate it separately from struct rpcrdma_xprt itself, which
looks already fairly large without it; on x86_64:
$ gdb net/sunrpc/xprtrdma/xprtrdma.ko
...
(gdb) p sizeof(struct rpcrdma_xprt)
$1 = 2912
But that shouldn't be a big deal to do.
--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Tim Gardner <hidden> Date: 2013-03-11 21:27:43
rpcrdma_register_default_external() is several frames into the call stack which
goes deeper yet. You run the risk of stack corruption by declaring such a large
automatic variable, so move the array of 'struct ib_phys_buf' objects into the
transport structure 'struct rpcrdma_xprt' (which is dynamically allocated) in
order to silence the frame-larger-than warning. Access to each struct
rpcrdma_xprt is serialized by XPRT_LOCKED in xprt_reserve_xprt(), so there is
no danger of multiple accessors to the array of struct ib_phys_buf objects.
net/sunrpc/xprtrdma/verbs.c: In function 'rpcrdma_register_default_external':
net/sunrpc/xprtrdma/verbs.c:1774:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]
gcc version 4.6.3
Cc: Trond Myklebust <redacted>
Cc: "J. Bruce Fields" <redacted>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Tom Tucker <redacted>
Cc: Haggai Eran <redacted>
Cc: Or Gerlitz <redacted>
Cc: Shani Michaeli <redacted>
Cc: linux-nfs@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Tim Gardner <redacted>
---
v1 - Use kmalloc() to dynamically allocate and free the array of 'struct
ib_phys_buf' objects
v2 - Move the array of 'struct ib_phys_buf' objects into struct rpcrdma_req
and pass this request down through rpcrdma_register_external() and
rpcrdma_register_default_external(). This is less overhead then using
kmalloc() and requires no extra error checking as the allocation burden is
shifted to the transport client.
v3 - Move the array of 'struct ib_phys_buf' objects into struct rpcrdma_xprt.
Pass a pointer to this transport structure into rpcrdma_register_default_external().
This is less overhead then using kmalloc() and requires no extra error checking
as the allocation burden is shifted to the transport client.
net/sunrpc/xprtrdma/verbs.c | 10 ++++++----
net/sunrpc/xprtrdma/xprt_rdma.h | 5 ++++-
2 files changed, 10 insertions(+), 5 deletions(-)
From: Tom Tucker <hidden> Date: 2013-03-11 23:02:12
On 3/11/13 4:25 PM, J. Bruce Fields wrote:
On Mon, Mar 11, 2013 at 03:15:08PM -0600, Tim Gardner wrote:
quoted
rpcrdma_register_default_external() is several frames into the call stack which
goes deeper yet. You run the risk of stack corruption by declaring such a large
automatic variable, so move the array of 'struct ib_phys_buf' objects into the
transport structure 'struct rpcrdma_xprt' (which is dynamically allocated) in
order to silence the frame-larger-than warning. Access to each struct
rpcrdma_xprt is serialized by XPRT_LOCKED in xprt_reserve_xprt(), so there is
no danger of multiple accessors to the array of struct ib_phys_buf objects.
net/sunrpc/xprtrdma/verbs.c: In function 'rpcrdma_register_default_external':
net/sunrpc/xprtrdma/verbs.c:1774:1: warning: the frame size of 1056 bytes is larger than 1024 bytes [-Wframe-larger-than=]
gcc version 4.6.3
Cc: Trond Myklebust <redacted>
Cc: "J. Bruce Fields" <redacted>
Cc: "David S. Miller" <davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
Cc: Tom Tucker <tom-/Yg/VP3ZvrM@public.gmane.org>
Cc: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Shani Michaeli <shanim-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Signed-off-by: Tim Gardner <redacted>
---
v1 - Use kmalloc() to dynamically allocate and free the array of 'struct
ib_phys_buf' objects
v2 - Move the array of 'struct ib_phys_buf' objects into struct rpcrdma_req
and pass this request down through rpcrdma_register_external() and
rpcrdma_register_default_external(). This is less overhead then using
kmalloc() and requires no extra error checking as the allocation burden is
shifted to the transport client.
v3 - Move the array of 'struct ib_phys_buf' objects into struct rpcrdma_xprt.
Pass a pointer to this transport structure into rpcrdma_register_default_external().
This is less overhead then using kmalloc() and requires no extra error checking
as the allocation burden is shifted to the transport client.
Looks good to me; wish we could get it tested....
I will test it. Tim could you please send me a final version that you'd
like tested as a single message?
Would someone (like Tim maybe ... hint hint) look at tearing out all those
dead registration strategies? I don't think we need or will ever use
bounce-buffers, memory windows, or mlnx fmr. The only two that are used
and tested are all-phys and FRMR (the default).
Tom
In future if we do decide to also increase the size of that array we may
need to allocate it separately from struct rpcrdma_xprt itself, which
looks already fairly large without it; on x86_64:
$ gdb net/sunrpc/xprtrdma/xprtrdma.ko
...
(gdb) p sizeof(struct rpcrdma_xprt)
$1 = 2912
But that shouldn't be a big deal to do.
--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Tim Gardner <hidden> Date: 2013-03-12 02:53:31
On 03/11/2013 05:02 PM, Tom Tucker wrote:
On 3/11/13 4:25 PM, J. Bruce Fields wrote:
<snip>
quoted
Looks good to me; wish we could get it tested....
I will test it. Tim could you please send me a final version that you'd
like tested as a single message?
I'm a little confused about what you are asking. I think v3 of the patch
is the final version (unless you find bugs with it).
Would someone (like Tim maybe ... hint hint) look at tearing out all
those dead registration strategies? I don't think we need or will ever
use bounce-buffers, memory windows, or mlnx fmr. The only two that are
used and tested are all-phys and FRMR (the default).
Dunno if I'll get time for that. I had a one day window where I could
hack out some simple patches. Now I'm back to the usual grindstone.
rtg
--
Tim Gardner tim.gardner@canonical.com
From: Tom Tucker <hidden> Date: 2013-03-12 03:40:22
Ah....Ok
On 3/11/13 9:53 PM, Tim Gardner wrote:
On 03/11/2013 05:02 PM, Tom Tucker wrote:
quoted
On 3/11/13 4:25 PM, J. Bruce Fields wrote:
<snip>
quoted
quoted
Looks good to me; wish we could get it tested....
I will test it. Tim could you please send me a final version that you'd
like tested as a single message?
I'm a little confused about what you are asking. I think v3 of the patch
is the final version (unless you find bugs with it).
quoted
Would someone (like Tim maybe ... hint hint) look at tearing out all
those dead registration strategies? I don't think we need or will ever
use bounce-buffers, memory windows, or mlnx fmr. The only two that are
used and tested are all-phys and FRMR (the default).
Dunno if I'll get time for that. I had a one day window where I could
hack out some simple patches. Now I'm back to the usual grindstone.
rtg
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html