[PATCH] scatterlist: inline sg_next()

[PATCH] scatterlist: inline sg_next()

[Caleb Sander Mateos]

sg_next() is a short function called frequently in I/O paths. Define it
in the header file so it can be inlined into its callers.

Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>
---
Is it a concern that this would break kernel modules built against old headers?
If so, I could update the patch to continue compiling and exporting sg_next() in
scatterlist.c.

 include/linux/scatterlist.h | 23 ++++++++++++++++++++++-
 lib/scatterlist.c           | 23 -----------------------
 2 files changed, 22 insertions(+), 24 deletions(-)

diff --git a/include/linux/scatterlist.h b/include/linux/scatterlist.h
index 138e2f1bd08f..0cdbfc42f153 100644
--- a/include/linux/scatterlist.h
+++ b/include/linux/scatterlist.h
@@ -92,10 +92,32 @@ static inline bool sg_is_chain(struct scatterlist *sg)
 static inline bool sg_is_last(struct scatterlist *sg)
 {
 	return __sg_flags(sg) & SG_END;
 }
 
+/**
+ * sg_next - return the next scatterlist entry in a list
+ * @sg:		The current sg entry
+ *
+ * Description:
+ *   Usually the next entry will be @sg@ + 1, but if this sg element is part
+ *   of a chained scatterlist, it could jump to the start of a new
+ *   scatterlist array.
+ *
+ **/
+static inline struct scatterlist *sg_next(struct scatterlist *sg)
+{
+	if (sg_is_last(sg))
+		return NULL;
+
+	sg++;
+	if (unlikely(sg_is_chain(sg)))
+		sg = sg_chain_ptr(sg);
+
+	return sg;
+}
+
 /**
  * sg_assign_page - Assign a given page to an SG entry
  * @sg:		    SG entry
  * @page:	    The page
  *
@@ -416,11 +438,10 @@ static inline void sg_init_marker(struct scatterlist *sgl,
 	sg_mark_end(&sgl[nents - 1]);
 }
 
 int sg_nents(struct scatterlist *sg);
 int sg_nents_for_len(struct scatterlist *sg, u64 len);
-struct scatterlist *sg_next(struct scatterlist *);
 struct scatterlist *sg_last(struct scatterlist *s, unsigned int);
 void sg_init_table(struct scatterlist *, unsigned int);
 void sg_init_one(struct scatterlist *, const void *, unsigned int);
 int sg_split(struct scatterlist *in, const int in_mapped_nents,
 	     const off_t skip, const int nb_splits,
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index b58d5ef1a34b..7582dfab7fe3 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -11,33 +11,10 @@
 #include <linux/kmemleak.h>
 #include <linux/bvec.h>
 #include <linux/uio.h>
 #include <linux/folio_queue.h>
 
-/**
- * sg_next - return the next scatterlist entry in a list
- * @sg:		The current sg entry
- *
- * Description:
- *   Usually the next entry will be @sg@ + 1, but if this sg element is part
- *   of a chained scatterlist, it could jump to the start of a new
- *   scatterlist array.
- *
- **/
-struct scatterlist *sg_next(struct scatterlist *sg)
-{
-	if (sg_is_last(sg))
-		return NULL;
-
-	sg++;
-	if (unlikely(sg_is_chain(sg)))
-		sg = sg_chain_ptr(sg);
-
-	return sg;
-}
-EXPORT_SYMBOL(sg_next);
-
 /**
  * sg_nents - return total count of entries in scatterlist
  * @sg:		The scatterlist
  *
  * Description:
-- 
2.45.2

Re: [PATCH] scatterlist: inline sg_next()

[Eric Biggers]

On Wed, Apr 16, 2025 at 10:06:13AM -0600, Caleb Sander Mateos wrote:
> sg_next() is a short function called frequently in I/O paths. Define it
> in the header file so it can be inlined into its callers.
> 
> Signed-off-by: Caleb Sander Mateos <csander@purestorage.com>

Reviewed-by: Eric Biggers <ebiggers@kernel.org>

I had been thinking about doing this too.  Besides the benefits for storage,
this will also reduce the overhead in some parts of the crypto subsystem.

> Is it a concern that this would break kernel modules built against old headers?
> If so, I could update the patch to continue compiling and exporting sg_next() in
> scatterlist.c.

No, that's not a concern.

- Eric

Re: [PATCH] scatterlist: inline sg_next()

[Andrew Morton]

On Wed, 16 Apr 2025 10:06:13 -0600 Caleb Sander Mateos <csander@purestorage.com> wrote:

> sg_next() is a short function called frequently in I/O paths. Define it
> in the header file so it can be inlined into its callers.

Does this actually make anything faster?

net/ceph/messenger_v2.c has four calls to sg_next().  x86_64 defconfig:

x1:/usr/src/25> size net/ceph/messenger_v2.o
   text	   data	    bss	    dec	    hex	filename
  31486	   2212	      0	  33698	   83a2	net/ceph/messenger_v2.o

after:

  31742	   2212	      0	  33954	   84a2	net/ceph/messenger_v2.o

More text means more cache misses.  Possibly the patch slows things down??
 

Re: [PATCH] scatterlist: inline sg_next()

[Caleb Sander Mateos]

On Wed, Apr 16, 2025 at 3:05 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Wed, 16 Apr 2025 10:06:13 -0600 Caleb Sander Mateos <csander@purestorage.com> wrote:
>
> > sg_next() is a short function called frequently in I/O paths. Define it
> > in the header file so it can be inlined into its callers.
>
> Does this actually make anything faster?
>
> net/ceph/messenger_v2.c has four calls to sg_next().  x86_64 defconfig:

Hmm, I count 7 calls in the source code. And that excludes possible
functions defined in included header files that also call sg_next().
And the functions which call sg_next() could themselves be inlined,
resulting in even more calls. The object file looks to have 7 calls to
sg_next():
$ readelf -r net/ceph/messenger_v2.o | grep -c sg_next
7

>
> x1:/usr/src/25> size net/ceph/messenger_v2.o
>    text    data     bss     dec     hex filename
>   31486    2212       0   33698    83a2 net/ceph/messenger_v2.o
>
> after:
>
>   31742    2212       0   33954    84a2 net/ceph/messenger_v2.o
>
> More text means more cache misses.  Possibly the patch slows things down??

Yes, it's true that inlining doesn't necessarily improve performance.
For reference, the workload I am looking at is issuing 32 KB NVMe
reads, which results in calling sg_next() from nvme_pci_setup_prps().
About 0.5% of the CPU time is spent in sg_next() itself (not counting
the cost of calling into it).
Inlining the function could help save the cost of the call + return,
as well as improve branch prediction rates for the if (sg_is_last(sg))
check by creating a separate copy of the branch in each caller.
My guess is that most workloads (like mine) don't call sg_next() from
all that many places. So even though inlining would duplicate the code
into all callers, not all the callers are hot. The number of locations
actually loaded into the instruction cache are likely to be relatively
few, so the increase in cached instructions wouldn't be as steep as
the text size suggests.
That's all to say: the costs and benefits are workload-dependent. And
in all likelihood, they will be pretty small either way.

Best,
Caleb