[RFC,bpf-next,06/20] trait: Replace memmove calls with inline move

Commit Message

Arthur Fabre March 5, 2025, 2:32 p.m. UTC

From: Arthur Fabre <[e-mail]>

When inserting or deleting traits, we need to move any subsequent
traits over.

Replace it with an inline implementation to avoid the function call
overhead. This is especially expensive on AMD with SRSO.

In practice we shouldn't have too much data to move around, and we're
naturally limited to 238 bytes max, so a dumb implementation should
hopefully be fast enough.

Jesper Brouer kindly ran benchmarks on real hardware with three configs:
- Intel: E5-1650 v4
- AMD SRSO: 9684X SRSO
- AMD IBPB: 9684X SRSO=IBPB

		Intel	AMD IBPB	AMD SRSO
xdp-trait-get	5.530	3.901		9.188		(ns/op)
xdp-trait-set	7.538	4.941		10.050		(ns/op)
xdp-trait-move	14.245	8.865		14.834		(ns/op)
function call	1.319	1.359		5.703		(ns/op)
indirect call	8.922	6.251		10.329		(ns/op)

Signed-off-by: Arthur Fabre <[e-mail]>
---
 include/net/trait.h | 40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

Comments

Jesper Dangaard Brouer March 6, 2025, 10:14 a.m. UTC | #1

On 05/03/2025 15.32, [e-mail] wrote:
> From: Arthur Fabre <[e-mail]>
> 
> When inserting or deleting traits, we need to move any subsequent
> traits over.
> 
> Replace it with an inline implementation to avoid the function call
> overhead. This is especially expensive on AMD with SRSO.
> 
> In practice we shouldn't have too much data to move around, and we're
> naturally limited to 238 bytes max, so a dumb implementation should
> hopefully be fast enough.
> 
> Jesper Brouer kindly ran benchmarks on real hardware with three configs:
> - Intel: E5-1650 v4
> - AMD SRSO: 9684X SRSO
> - AMD IBPB: 9684X SRSO=IBPB
> 
> 		Intel	AMD IBPB	AMD SRSO
> xdp-trait-get	5.530	3.901		9.188		(ns/op)
> xdp-trait-set	7.538	4.941		10.050		(ns/op)
> xdp-trait-move	14.245	8.865		14.834		(ns/op)
> function call	1.319	1.359		5.703		(ns/op)
> indirect call	8.922	6.251		10.329		(ns/op)
> 

I've done extensive *micro* bechmarking documented here:
  - https://github.com/xdp-project/xdp-project/tree/main/areas/hints
  - In traits0X_* files

The latest that corresponds to this patchset is in this file:
  - 
https://github.com/xdp-project/xdp-project/blob/main/areas/hints/traits07_bench-009.org

I've not done XDP_REDIRECT testing, which would likely show the bitfield 
change in xdp_frame, that Olek pointed out.

--Jesper

Patch

diff --git a/include/net/trait.h b/include/net/trait.h
index d4581a877bd57a32e2ad032147c906764d6d37f8..111ce766345cef45fd95e562eda6a7d01c6b76da 100644
--- a/include/net/trait.h
+++ b/include/net/trait.h
@@ -75,6 +75,40 @@  static __always_inline int __trait_offset(struct __trait_hdr h, u64 key)
 	return sizeof(struct __trait_hdr) + __trait_total_length(__trait_and(h, ~(~0llu << key)));
 }
 
+/* Avoid overhead of memmove() function call when possible. */
+static __always_inline void __trait_move(void *src, int off, size_t n)
+{
+	if (n == 0)
+		return;
+
+	if (!IS_ENABLED(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) || BITS_PER_LONG != 64) {
+		memmove(src + off, src, n);
+		return;
+	}
+
+	/* Need to move in reverse to handle overlap. */
+	if (off > 0)
+		src += n;
+
+#define ___trait_move(op) do { \
+		src -= (off > 0) ? sizeof(u##op) : 0; \
+		*(u##op *)(src + off) = *(u##op *)src; \
+		src += (off < 0) ? sizeof(u##op) : 0; \
+	} while (0)
+
+	for (int w = 0; w < n / 8; w++)
+		___trait_move(64);
+
+	if (n & 4)
+		___trait_move(32);
+
+	if (n & 2)
+		___trait_move(16);
+
+	if (n & 1)
+		___trait_move(8);
+}
+
 /**
  * traits_init() - Initialize a trait store.
  * @traits: Start of trait store area.
@@ -142,8 +176,7 @@  int trait_set(void *traits, void *hard_end, u64 key, const void *val, u64 len, u
 			return -ENOSPC;
 
 		/* Memmove all the kvs after us over. */
-		if (traits_size(traits) > off)
-			memmove(traits + off + len, traits + off, traits_size(traits) - off);
+		__trait_move(traits + off, len, traits_size(traits) - off);
 	}
 
 	u64 encode_len = 0;
@@ -244,8 +277,7 @@  static __always_inline int trait_del(void *traits, u64 key)
 	int len = __trait_total_length(__trait_and(*h, (1ull << key)));
 
 	/* Memmove all the kvs after us over */
-	if (traits_size(traits) > off + len)
-		memmove(traits + off, traits + off + len, traits_size(traits) - off - len);
+	__trait_move(traits + off + len, -len, traits_size(traits) - off - len);
 
 	/* Clear our length in header */
 	h->high &= ~(1ull << key);