diff mbox series

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

Message ID 20210312154331.32229-6-mgorman@techsingularity.net (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Introduce a bulk order-0 page allocator with two in-tree users | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Mel Gorman March 12, 2021, 3:43 p.m. UTC 
From: Chuck Lever <chuck.lever@oracle.com>

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@oracle.com>
Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
---
 net/sunrpc/svc_xprt.c | 43 +++++++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 12 deletions(-)

Comments

Alexander Duyck March 12, 2021, 6:44 p.m. UTC | #1 
On Fri, Mar 12, 2021 at 7:43 AM Mel Gorman <mgorman@techsingularity.net> wrote:
>
> From: Chuck Lever <chuck.lever@oracle.com>
>
> 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@oracle.com>
> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
> ---
>  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 March 12, 2021, 7:22 p.m. UTC | #2 
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@gmail.com> wrote:
> 
> On Fri, Mar 12, 2021 at 7:43 AM Mel Gorman <mgorman@techsingularity.net> wrote:
>> 
>> From: Chuck Lever <chuck.lever@oracle.com>
>> 
>> 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@oracle.com>
>> Signed-off-by: Mel Gorman <mgorman@techsingularity.net>
>> ---
>> 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
Mel Gorman March 13, 2021, 12:59 p.m. UTC | #3 
On Fri, Mar 12, 2021 at 07:22:43PM +0000, Chuck Lever III wrote:
> 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.
> 

Send me a tidied version anyway. I'm happy enough to include them in the
series even if it ultimately gets merged via the NFSD tree. It'll need
to be kept as a separate pull request to avoid delaying unrelated NFSD
patches until Andrew merges the mm parts.
diff mbox series

Patch

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++;
+	if (needed) {
+		LIST_HEAD(list);
+
+retry:
+		alloc_pages_bulk(GFP_KERNEL, needed, &list);
+		for (i = 0; i < pages; i++) {
+			if (!rqstp->rq_pages[i]) {
+				page = list_first_entry_or_null(&list,
+								struct page,
+								lru);
+				if (unlikely(!page))
+					goto empty_list;
+				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