[PATCH v1 0/4] NFSD memory allocation optimizations

[PATCH v1 0/4] NFSD memory allocation optimizations

[Chuck Lever]

I've found a few ways to optimize the release of pages in NFSD.
Please let me know if I'm abusing the release_pages() and pagevec
APIs.

---

Chuck Lever (4):
      SUNRPC: Relocate svc_free_res_pages()
      SUNRPC: Convert svc_xprt_release() to the release_pages() API
      SUNRPC: Convert svc_tcp_restore_pages() to the release_pages() API
      SUNRPC: Be even lazier about releasing pages


 include/linux/sunrpc/svc.h | 12 +-----------
 net/sunrpc/svc.c           | 21 +++++++++++++++++++++
 net/sunrpc/svc_xprt.c      |  5 +----
 net/sunrpc/svcsock.c       | 12 ++++++------
 4 files changed, 29 insertions(+), 21 deletions(-)

--
Chuck Lever

[PATCH v1 1/4] SUNRPC: Relocate svc_free_res_pages()

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

Clean-up: There doesn't seem to be a reason why this function is
stuck in a header. One thing it prevents is the convenient addition
of tracing. Moving it to a source file also makes the rq_respages
clean-up logic easier to find.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/linux/sunrpc/svc.h |   12 +-----------
 net/sunrpc/svc.c           |   19 +++++++++++++++++++
 net/sunrpc/svc_xprt.c      |    2 +-
 3 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
index 2d31121fc2e6..762d7231e574 100644
--- a/include/linux/sunrpc/svc.h
+++ b/include/linux/sunrpc/svc.h
@@ -309,17 +309,6 @@ static inline struct sockaddr *svc_daddr(const struct svc_rqst *rqst)
 	return (struct sockaddr *) &rqst->rq_daddr;
 }
 
-static inline void svc_free_res_pages(struct svc_rqst *rqstp)
-{
-	while (rqstp->rq_next_page != rqstp->rq_respages) {
-		struct page **pp = --rqstp->rq_next_page;
-		if (*pp) {
-			put_page(*pp);
-			*pp = NULL;
-		}
-	}
-}
-
 struct svc_deferred_req {
 	u32			prot;	/* protocol (UDP or TCP) */
 	struct svc_xprt		*xprt;
@@ -424,6 +413,7 @@ struct svc_rqst *svc_rqst_alloc(struct svc_serv *serv,
 					struct svc_pool *pool, int node);
 bool		   svc_rqst_replace_page(struct svc_rqst *rqstp,
 					 struct page *page);
+void		   svc_rqst_release_pages(struct svc_rqst *rqstp);
 void		   svc_rqst_free(struct svc_rqst *);
 void		   svc_exit_thread(struct svc_rqst *);
 struct svc_serv *  svc_create_pooled(struct svc_program *, unsigned int,
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 0aa8892fad63..0fc70cc405b2 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -869,6 +869,25 @@ bool svc_rqst_replace_page(struct svc_rqst *rqstp, struct page *page)
 }
 EXPORT_SYMBOL_GPL(svc_rqst_replace_page);
 
+/**
+ * svc_rqst_release_pages - Release Reply buffer pages
+ * @rqstp: RPC transaction context
+ *
+ * Release response pages that might still be in flight after
+ * svc_send, and any spliced filesystem-owned pages.
+ */
+void svc_rqst_release_pages(struct svc_rqst *rqstp)
+{
+	while (rqstp->rq_next_page != rqstp->rq_respages) {
+		struct page **pp = --rqstp->rq_next_page;
+
+		if (*pp) {
+			put_page(*pp);
+			*pp = NULL;
+		}
+	}
+}
+
 /*
  * Called from a server thread as it's exiting. Caller must hold the "service
  * mutex" for the service.
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 36c79b718323..533e08c4f319 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -542,7 +542,7 @@ static void svc_xprt_release(struct svc_rqst *rqstp)
 	rqstp->rq_deferred = NULL;
 
 	pagevec_release(&rqstp->rq_pvec);
-	svc_free_res_pages(rqstp);
+	svc_rqst_release_pages(rqstp);
 	rqstp->rq_res.page_len = 0;
 	rqstp->rq_res.page_base = 0;
 

[PATCH v1 2/4] SUNRPC: Convert svc_xprt_release() to the release_pages() API

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

Instead of invoking put_page() one-at-a-time, pass the "response"
portion of rq_pages directly to release_pages() to reduce the number
of times each nfsd thread invokes a page allocator API.

Since svc_xprt_release() is not invoked while a client is waiting
for an RPC Reply, this is not expected to directly impact mean
request latencies on a lightly or moderately loaded server. However
as workload intensity increases, I expect somewhat better
scalability: the same number of server threads should be able to
handle more work.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/svc.c |   11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 0fc70cc405b2..b982f802f2a0 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -878,13 +878,12 @@ EXPORT_SYMBOL_GPL(svc_rqst_replace_page);
  */
 void svc_rqst_release_pages(struct svc_rqst *rqstp)
 {
-	while (rqstp->rq_next_page != rqstp->rq_respages) {
-		struct page **pp = --rqstp->rq_next_page;
+	int i, count = rqstp->rq_next_page - rqstp->rq_respages;
 
-		if (*pp) {
-			put_page(*pp);
-			*pp = NULL;
-		}
+	if (count) {
+		release_pages(rqstp->rq_respages, count);
+		for (i = 0; i < count; i++)
+			rqstp->rq_respages[i] = NULL;
 	}
 }
 

[PATCH v1 3/4] SUNRPC: Convert svc_tcp_restore_pages() to the release_pages() API

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

Instead of invoking put_page() one-at-a-time, pass the portion of
rq_pages to be released directly to release_pages() to reduce the
number of times each server thread invokes a page allocator API.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/svcsock.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 302a14dd7882..44f72b558a1c 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -812,12 +812,12 @@ static size_t svc_tcp_restore_pages(struct svc_sock *svsk,
 	if (!len)
 		return 0;
 	npages = (len + PAGE_SIZE - 1) >> PAGE_SHIFT;
-	for (i = 0; i < npages; i++) {
-		if (rqstp->rq_pages[i] != NULL)
-			put_page(rqstp->rq_pages[i]);
-		BUG_ON(svsk->sk_pages[i] == NULL);
-		rqstp->rq_pages[i] = svsk->sk_pages[i];
-		svsk->sk_pages[i] = NULL;
+	if (npages) {
+		release_pages(rqstp->rq_pages, npages);
+		for (i = 0; i < npages; i++) {
+			rqstp->rq_pages[i] = svsk->sk_pages[i];
+			svsk->sk_pages[i] = NULL;
+		}
 	}
 	rqstp->rq_arg.head[0].iov_base = page_address(rqstp->rq_pages[0]);
 	return len;

[PATCH v1 4/4] SUNRPC: Be even lazier about releasing pages

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

A single RPC transaction that touches only a couple of pages means
rq_pvec will not be even close to full in svc_xpt_release(). This is
a common case.

Instead, just leave the pages in rq_pvec until it is completely
full. This improves the efficiency of the batch release mechanism
on workloads that involve small RPC messages.

The rq_pvec is also fully emptied just before thread exit.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/svc.c      |    3 +++
 net/sunrpc/svc_xprt.c |    3 ---
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index b982f802f2a0..26367cf4c17a 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -649,6 +649,8 @@ svc_rqst_alloc(struct svc_serv *serv, struct svc_pool *pool, int node)
 	if (!rqstp)
 		return rqstp;
 
+	pagevec_init(&rqstp->rq_pvec);
+
 	__set_bit(RQ_BUSY, &rqstp->rq_flags);
 	rqstp->rq_server = serv;
 	rqstp->rq_pool = pool;
@@ -894,6 +896,7 @@ void svc_rqst_release_pages(struct svc_rqst *rqstp)
 void
 svc_rqst_free(struct svc_rqst *rqstp)
 {
+	pagevec_release(&rqstp->rq_pvec);
 	svc_release_buffer(rqstp);
 	if (rqstp->rq_scratch_page)
 		put_page(rqstp->rq_scratch_page);
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 533e08c4f319..e3952b690f54 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -541,7 +541,6 @@ static void svc_xprt_release(struct svc_rqst *rqstp)
 	kfree(rqstp->rq_deferred);
 	rqstp->rq_deferred = NULL;
 
-	pagevec_release(&rqstp->rq_pvec);
 	svc_rqst_release_pages(rqstp);
 	rqstp->rq_res.page_len = 0;
 	rqstp->rq_res.page_base = 0;
@@ -667,8 +666,6 @@ static int svc_alloc_arg(struct svc_rqst *rqstp)
 	struct xdr_buf *arg = &rqstp->rq_arg;
 	unsigned long pages, filled, ret;
 
-	pagevec_init(&rqstp->rq_pvec);
-
 	pages = (serv->sv_max_mesg + 2 * PAGE_SIZE) >> PAGE_SHIFT;
 	if (pages > RPCSVC_MAXPAGES) {
 		pr_warn_once("svc: warning: pages=%lu > RPCSVC_MAXPAGES=%lu\n",

Re: [PATCH v1 0/4] NFSD memory allocation optimizations

[Calum Mackay]

[-- Attachment #1.1: Type: text/plain, Size: 853 bytes --]

On 15/04/2023 1:17 am, Chuck Lever wrote:
> I've found a few ways to optimize the release of pages in NFSD.
> Please let me know if I'm abusing the release_pages() and pagevec
> APIs.
> 
> ---
> 
> Chuck Lever (4):
>        SUNRPC: Relocate svc_free_res_pages()
>        SUNRPC: Convert svc_xprt_release() to the release_pages() API
>        SUNRPC: Convert svc_tcp_restore_pages() to the release_pages() API
>        SUNRPC: Be even lazier about releasing pages
> 
> 
>   include/linux/sunrpc/svc.h | 12 +-----------
>   net/sunrpc/svc.c           | 21 +++++++++++++++++++++
>   net/sunrpc/svc_xprt.c      |  5 +----
>   net/sunrpc/svcsock.c       | 12 ++++++------
>   4 files changed, 29 insertions(+), 21 deletions(-)
> 
> --
> Chuck Lever
> 

Looks good to me, Chuck.

Reviewed-by: Calum Mackay <calum.mackay@oracle.com>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]