Re: [PATCH 5/7] SUNRPC: Refresh rq_pages using a bulk page allocator

[Chuck Lever III]

Mel, I can send you a tidied and tested update to this patch,
or you can drop the two NFSD patches and I can submit them via
the NFSD tree when alloc_pages_bulk() is merged.

> On Mar 12, 2021, at 1:44 PM, Alexander Duyck <alexander.duyck@xxxxxxxxx> wrote:
> 
> On Fri, Mar 12, 2021 at 7:43 AM Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx> wrote:
>> 
>> From: Chuck Lever <chuck.lever@xxxxxxxxxx>
>> 
>> Reduce the rate at which nfsd threads hammer on the page allocator.
>> This improves throughput scalability by enabling the threads to run
>> more independently of each other.
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
>> Signed-off-by: Mel Gorman <mgorman@xxxxxxxxxxxxxxxxxxx>
>> ---
>> net/sunrpc/svc_xprt.c | 43 +++++++++++++++++++++++++++++++------------
>> 1 file changed, 31 insertions(+), 12 deletions(-)
>> 
>> diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
>> index cfa7e4776d0e..38a8d6283801 100644
>> --- a/net/sunrpc/svc_xprt.c
>> +++ b/net/sunrpc/svc_xprt.c
>> @@ -642,11 +642,12 @@ static void svc_check_conn_limits(struct svc_serv *serv)
>> static int svc_alloc_arg(struct svc_rqst *rqstp)
>> {
>>        struct svc_serv *serv = rqstp->rq_server;
>> +       unsigned long needed;
>>        struct xdr_buf *arg;
>> +       struct page *page;
>>        int pages;
>>        int i;
>> 
>> -       /* now allocate needed pages.  If we get a failure, sleep briefly */
>>        pages = (serv->sv_max_mesg + 2 * PAGE_SIZE) >> PAGE_SHIFT;
>>        if (pages > RPCSVC_MAXPAGES) {
>>                pr_warn_once("svc: warning: pages=%u > RPCSVC_MAXPAGES=%lu\n",
>> @@ -654,19 +655,28 @@ static int svc_alloc_arg(struct svc_rqst *rqstp)
>>                /* use as many pages as possible */
>>                pages = RPCSVC_MAXPAGES;
>>        }
>> -       for (i = 0; i < pages ; i++)
>> -               while (rqstp->rq_pages[i] == NULL) {
>> -                       struct page *p = alloc_page(GFP_KERNEL);
>> -                       if (!p) {
>> -                               set_current_state(TASK_INTERRUPTIBLE);
>> -                               if (signalled() || kthread_should_stop()) {
>> -                                       set_current_state(TASK_RUNNING);
>> -                                       return -EINTR;
>> -                               }
>> -                               schedule_timeout(msecs_to_jiffies(500));
>> +
> 
>> +       for (needed = 0, i = 0; i < pages ; i++)
>> +               if (!rqstp->rq_pages[i])
>> +                       needed++;
> 
> I would use an opening and closing braces for the for loop since
> technically the if is a multiline statement. It will make this more
> readable.
> 
>> +       if (needed) {
>> +               LIST_HEAD(list);
>> +
>> +retry:
> 
> Rather than kind of open code a while loop why not just make this
> "while (needed)"? Then all you have to do is break out of the for loop
> and you will automatically return here instead of having to jump to
> two different labels.
> 
>> +               alloc_pages_bulk(GFP_KERNEL, needed, &list);
> 
> Rather than not using the return value would it make sense here to
> perhaps subtract it from needed? Then you would know if any of the
> allocation requests weren't fulfilled.
> 
>> +               for (i = 0; i < pages; i++) {
> 
> It is probably optimizing for the exception case, but I don't think
> you want the "i = 0" here. If you are having to stop because the list
> is empty it probably makes sense to resume where you left off. So you
> should probably be initializing i to 0 before we check for needed.
> 
>> +                       if (!rqstp->rq_pages[i]) {
> 
> It might be cleaner here to just do a "continue" if rq_pages[i] is populated.
> 
>> +                               page = list_first_entry_or_null(&list,
>> +                                                               struct page,
>> +                                                               lru);
>> +                               if (unlikely(!page))
>> +                                       goto empty_list;
> 
> I think I preferred the original code that wasn't jumping away from
> the loop here. With the change I suggested above that would switch the
> if(needed) to while(needed) you could have it just break out of the
> for loop to place itself back in the while loop.
> 
>> +                               list_del(&page->lru);
>> +                               rqstp->rq_pages[i] = page;
>> +                               needed--;
>>                        }
>> -                       rqstp->rq_pages[i] = p;
>>                }
>> +       }
>>        rqstp->rq_page_end = &rqstp->rq_pages[pages];
>>        rqstp->rq_pages[pages] = NULL; /* this might be seen in nfsd_splice_actor() */
>> 
>> @@ -681,6 +691,15 @@ static int svc_alloc_arg(struct svc_rqst *rqstp)
>>        arg->len = (pages-1)*PAGE_SIZE;
>>        arg->tail[0].iov_len = 0;
>>        return 0;
>> +
>> +empty_list:
>> +       set_current_state(TASK_INTERRUPTIBLE);
>> +       if (signalled() || kthread_should_stop()) {
>> +               set_current_state(TASK_RUNNING);
>> +               return -EINTR;
>> +       }
>> +       schedule_timeout(msecs_to_jiffies(500));
>> +       goto retry;
>> }
>> 
>> static bool
>> --
>> 2.26.2

--
Chuck Lever