| Message ID | 20250416160615.3571958-1-csander@purestorage.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | scatterlist: inline sg_next() | expand |
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
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??
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
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:
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(-)