| Message ID | 163027659478.7591.8897815399981483759@noble.neil.brown.name (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SUNRPC: don't pause on incomplete allocation | expand |
On Mon, Aug 30, 2021 at 08:36:34AM +1000, NeilBrown wrote: > > alloc_pages_bulk_array() attempts to allocate at least one page based on > the provided pages, and then opportunistically allocates more if that > can be done without dropping the spinlock. > > So if it returns fewer than requested, that could just mean that it > needed to drop the lock. In that case, try again immediately. > > Only pause for a time if no progress could be made. > > Reported-and-tested-by: Mike Javorski <mike.javorski@gmail.com> > Reported-and-tested-by: Lothar Paltins <lopa@mailbox.org> > Fixes: f6e70aab9dfe ("SUNRPC: refresh rq_pages using a bulk page allocator") > Signed-off-by: NeilBrown <neilb@suse.de> Acked-by: Mel Gorman <mgorman@suse.com>
For what it's worth, another positive report on this patch: https://bugzilla.kernel.org/show_bug.cgi?id=213869#c10 --b. On Mon, Aug 30, 2021 at 08:36:34AM +1000, NeilBrown wrote: > > alloc_pages_bulk_array() attempts to allocate at least one page based on > the provided pages, and then opportunistically allocates more if that > can be done without dropping the spinlock. > > So if it returns fewer than requested, that could just mean that it > needed to drop the lock. In that case, try again immediately. > > Only pause for a time if no progress could be made. > > Reported-and-tested-by: Mike Javorski <mike.javorski@gmail.com> > Reported-and-tested-by: Lothar Paltins <lopa@mailbox.org> > Fixes: f6e70aab9dfe ("SUNRPC: refresh rq_pages using a bulk page allocator") > Signed-off-by: NeilBrown <neilb@suse.de> > --- > > I decided I would resend, as I thought the for() loops could be clearer. > This patch should perform exactly the same as the previous one. > > > net/sunrpc/svc_xprt.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c > index d66a8e44a1ae..e74d5cf3cbb4 100644 > --- a/net/sunrpc/svc_xprt.c > +++ b/net/sunrpc/svc_xprt.c > @@ -662,7 +662,7 @@ static int svc_alloc_arg(struct svc_rqst *rqstp) > { > struct svc_serv *serv = rqstp->rq_server; > struct xdr_buf *arg = &rqstp->rq_arg; > - unsigned long pages, filled; > + unsigned long pages, filled, ret; > > pages = (serv->sv_max_mesg + 2 * PAGE_SIZE) >> PAGE_SHIFT; > if (pages > RPCSVC_MAXPAGES) { > @@ -672,11 +672,12 @@ static int svc_alloc_arg(struct svc_rqst *rqstp) > pages = RPCSVC_MAXPAGES; > } > > - for (;;) { > - filled = alloc_pages_bulk_array(GFP_KERNEL, pages, > - rqstp->rq_pages); > - if (filled == pages) > - break; > + for (filled = 0; filled < pages; filled = ret) { > + ret = alloc_pages_bulk_array(GFP_KERNEL, pages, > + rqstp->rq_pages); > + if (ret > filled) > + /* Made progress, don't sleep yet */ > + continue; > > set_current_state(TASK_INTERRUPTIBLE); > if (signalled() || kthread_should_stop()) { > -- > 2.32.0
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c index d66a8e44a1ae..e74d5cf3cbb4 100644 --- a/net/sunrpc/svc_xprt.c +++ b/net/sunrpc/svc_xprt.c @@ -662,7 +662,7 @@ static int svc_alloc_arg(struct svc_rqst *rqstp) { struct svc_serv *serv = rqstp->rq_server; struct xdr_buf *arg = &rqstp->rq_arg; - unsigned long pages, filled; + unsigned long pages, filled, ret; pages = (serv->sv_max_mesg + 2 * PAGE_SIZE) >> PAGE_SHIFT; if (pages > RPCSVC_MAXPAGES) { @@ -672,11 +672,12 @@ static int svc_alloc_arg(struct svc_rqst *rqstp) pages = RPCSVC_MAXPAGES; } - for (;;) { - filled = alloc_pages_bulk_array(GFP_KERNEL, pages, - rqstp->rq_pages); - if (filled == pages) - break; + for (filled = 0; filled < pages; filled = ret) { + ret = alloc_pages_bulk_array(GFP_KERNEL, pages, + rqstp->rq_pages); + if (ret > filled) + /* Made progress, don't sleep yet */ + continue; set_current_state(TASK_INTERRUPTIBLE); if (signalled() || kthread_should_stop()) {
alloc_pages_bulk_array() attempts to allocate at least one page based on the provided pages, and then opportunistically allocates more if that can be done without dropping the spinlock. So if it returns fewer than requested, that could just mean that it needed to drop the lock. In that case, try again immediately. Only pause for a time if no progress could be made. Reported-and-tested-by: Mike Javorski <mike.javorski@gmail.com> Reported-and-tested-by: Lothar Paltins <lopa@mailbox.org> Fixes: f6e70aab9dfe ("SUNRPC: refresh rq_pages using a bulk page allocator") Signed-off-by: NeilBrown <neilb@suse.de> --- I decided I would resend, as I thought the for() loops could be clearer. This patch should perform exactly the same as the previous one. net/sunrpc/svc_xprt.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)