| Message ID | 1389627955.20467.9.camel@x41 |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Jan 13, 2014, at 10:45, Paul Bolle <pebolle@tiscali.nl> wrote: > Building verbs.o on 32 bits x86, with CONFIG_FRAME_WARN set to 1024, its > default value, triggers this GCC warning: > net/sunrpc/xprtrdma/verbs.c: In function ‘rpcrdma_register_default_external’: > net/sunrpc/xprtrdma/verbs.c:1774:1: warning: the frame size of 1040 bytes is larger than 1024 bytes [-Wframe-larger-than=] > > Silence this warning by allocating "ipb" dynamically. > > Signed-off-by: Paul Bolle <pebolle@tiscali.nl> > --- > 0) Compile tested only (on 32 bits x86). I don't have access to > Infiniband hardware. > > 1) Please note that this is not a new warning. The oldest build log I > have still available on this machine is for a v3.8 rc, and it already > showed this warning. > > 2) I do hope my choice for the GFP_KERNEL flag is correct here. > > net/sunrpc/xprtrdma/verbs.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c > index 93726560..939ccc8 100644 > --- a/net/sunrpc/xprtrdma/verbs.c > +++ b/net/sunrpc/xprtrdma/verbs.c > @@ -1736,11 +1736,14 @@ rpcrdma_register_default_external(struct rpcrdma_mr_seg *seg, > int mem_priv = (writing ? IB_ACCESS_REMOTE_WRITE : > IB_ACCESS_REMOTE_READ); > struct rpcrdma_mr_seg *seg1 = seg; > - struct ib_phys_buf ipb[RPCRDMA_MAX_DATA_SEGS]; > + struct ib_phys_buf *ipb; > int len, i, rc = 0; > > if (*nsegs > RPCRDMA_MAX_DATA_SEGS) > *nsegs = RPCRDMA_MAX_DATA_SEGS; > + ipb = kmalloc(sizeof(*ipb) * *nsegs, GFP_KERNEL); > + if (ipb == NULL) > + return -ENOMEM; > for (len = 0, i = 0; i < *nsegs;) { > rpcrdma_map_one(ia, seg, writing); > ipb[i].addr = seg->mr_dma; > @@ -1770,6 +1773,7 @@ rpcrdma_register_default_external(struct rpcrdma_mr_seg *seg, > seg1->mr_len = len; > } > *nsegs = i; > + kfree(ipb); > return rc; > } Hi Paul, Unfortunately, the above could be called from a file write back context, so we cannot use GFP_KERNEL allocations. In fact, I suspect we should only use GFP_ATOMIC, and then have the rpc_task delay and retry if the allocation fails. The problem is that it looks to me as if xprt_rdma_send_request will currently fail, if we return an error from rpcrdma_register_default_external. Chuck, will you be able to look into the above issue as part of your RDMA work? Cheers Trond -- Trond Myklebust Linux NFS client maintainer -- 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
Hi- On Jan 13, 2014, at 10:59 AM, Trond Myklebust <trond.myklebust@primarydata.com> wrote: > > On Jan 13, 2014, at 10:45, Paul Bolle <pebolle@tiscali.nl> wrote: > >> Building verbs.o on 32 bits x86, with CONFIG_FRAME_WARN set to 1024, its >> default value, triggers this GCC warning: >> net/sunrpc/xprtrdma/verbs.c: In function ‘rpcrdma_register_default_external’: >> net/sunrpc/xprtrdma/verbs.c:1774:1: warning: the frame size of 1040 bytes is larger than 1024 bytes [-Wframe-larger-than=] >> >> Silence this warning by allocating "ipb" dynamically. >> >> Signed-off-by: Paul Bolle <pebolle@tiscali.nl> >> --- >> 0) Compile tested only (on 32 bits x86). I don't have access to >> Infiniband hardware. >> >> 1) Please note that this is not a new warning. The oldest build log I >> have still available on this machine is for a v3.8 rc, and it already >> showed this warning. >> >> 2) I do hope my choice for the GFP_KERNEL flag is correct here. >> >> net/sunrpc/xprtrdma/verbs.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c >> index 93726560..939ccc8 100644 >> --- a/net/sunrpc/xprtrdma/verbs.c >> +++ b/net/sunrpc/xprtrdma/verbs.c >> @@ -1736,11 +1736,14 @@ rpcrdma_register_default_external(struct rpcrdma_mr_seg *seg, >> int mem_priv = (writing ? IB_ACCESS_REMOTE_WRITE : >> IB_ACCESS_REMOTE_READ); >> struct rpcrdma_mr_seg *seg1 = seg; >> - struct ib_phys_buf ipb[RPCRDMA_MAX_DATA_SEGS]; >> + struct ib_phys_buf *ipb; >> int len, i, rc = 0; >> >> if (*nsegs > RPCRDMA_MAX_DATA_SEGS) >> *nsegs = RPCRDMA_MAX_DATA_SEGS; >> + ipb = kmalloc(sizeof(*ipb) * *nsegs, GFP_KERNEL); >> + if (ipb == NULL) >> + return -ENOMEM; >> for (len = 0, i = 0; i < *nsegs;) { >> rpcrdma_map_one(ia, seg, writing); >> ipb[i].addr = seg->mr_dma; >> @@ -1770,6 +1773,7 @@ rpcrdma_register_default_external(struct rpcrdma_mr_seg *seg, >> seg1->mr_len = len; >> } >> *nsegs = i; >> + kfree(ipb); >> return rc; >> } > > Hi Paul, > > Unfortunately, the above could be called from a file write back context, so we cannot use GFP_KERNEL allocations. In fact, I suspect we should only use GFP_ATOMIC, and then have the rpc_task delay and retry if the allocation fails. > > The problem is that it looks to me as if xprt_rdma_send_request will currently fail, if we return an error from rpcrdma_register_default_external. > > Chuck, will you be able to look into the above issue as part of your RDMA work? I’m building a queue of NFS/RDMA work on bugzilla.kernel.org. Let’s create a defect report there to document this, and it will get prioritized with the rest. Paul, can you do that to start us off? Product “File system”, Component “NFS”. I can’t say that a warning on 32-bit x86 is going to be an especially high priority. However, the underlying issue of allocating arrays of data segments on the stack is something that needs extended attention, and is already in plan. -- Chuck Lever chuck[dot]lever[at]oracle[dot]com -- 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
On Mon, 2014-01-13 at 11:17 -0500, Chuck Lever wrote: > I’m building a queue of NFS/RDMA work on bugzilla.kernel.org. Let’s > create a defect report there to document this, and it will get > prioritized with the rest. Paul, can you do that to start us off? > Product “File system”, Component “NFS”. Sure, see https://bugzilla.kernel.org/show_bug.cgi?id=68661 . Please feel free to edit the bug's title, etc, as you see fit. > I can’t say that a warning on 32-bit x86 is going to be an especially > high priority. I see. 32-bit x86 seems to be dropping in relevance quite fast. On the other hand, this is one of the last warnings I see when currently building x86 (32-bit, that is) and it would be rather nice to see this warning gone. Since my .config is basically a Fedora 20 .config, that would help make Fedora's 32-bit x86 build (almost) warning free too. > However, the underlying issue of allocating arrays of data segments on > the stack is something that needs extended attention, and is already > in plan. Thanks, Paul Bolle -- 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
diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c index 93726560..939ccc8 100644 --- a/net/sunrpc/xprtrdma/verbs.c +++ b/net/sunrpc/xprtrdma/verbs.c @@ -1736,11 +1736,14 @@ rpcrdma_register_default_external(struct rpcrdma_mr_seg *seg, int mem_priv = (writing ? IB_ACCESS_REMOTE_WRITE : IB_ACCESS_REMOTE_READ); struct rpcrdma_mr_seg *seg1 = seg; - struct ib_phys_buf ipb[RPCRDMA_MAX_DATA_SEGS]; + struct ib_phys_buf *ipb; int len, i, rc = 0; if (*nsegs > RPCRDMA_MAX_DATA_SEGS) *nsegs = RPCRDMA_MAX_DATA_SEGS; + ipb = kmalloc(sizeof(*ipb) * *nsegs, GFP_KERNEL); + if (ipb == NULL) + return -ENOMEM; for (len = 0, i = 0; i < *nsegs;) { rpcrdma_map_one(ia, seg, writing); ipb[i].addr = seg->mr_dma; @@ -1770,6 +1773,7 @@ rpcrdma_register_default_external(struct rpcrdma_mr_seg *seg, seg1->mr_len = len; } *nsegs = i; + kfree(ipb); return rc; }
Building verbs.o on 32 bits x86, with CONFIG_FRAME_WARN set to 1024, its default value, triggers this GCC warning: net/sunrpc/xprtrdma/verbs.c: In function ‘rpcrdma_register_default_external’: net/sunrpc/xprtrdma/verbs.c:1774:1: warning: the frame size of 1040 bytes is larger than 1024 bytes [-Wframe-larger-than=] Silence this warning by allocating "ipb" dynamically. Signed-off-by: Paul Bolle <pebolle@tiscali.nl> --- 0) Compile tested only (on 32 bits x86). I don't have access to Infiniband hardware. 1) Please note that this is not a new warning. The oldest build log I have still available on this machine is for a v3.8 rc, and it already showed this warning. 2) I do hope my choice for the GFP_KERNEL flag is correct here. net/sunrpc/xprtrdma/verbs.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)