udp: reduce cache pressure

PATCH Superseded Paolo Abeni 3 participants 10 messages Jun 6, 2017 – Jun 9, 2017

Subsystems (3)

Networking Network drivers UDP

lore

Share & tools

lore

Paolo Abeni

author maintainer

2017-06-06 14:24 UTC #0 lore raw

v2 cover udp: reduce cache pressure

In the most common use case, many skb fields are not used by recvmsg(), and the few ones actually accessed lays on cold cachelines, which leads to several cache miss per packet.

This patch series attempts to reduce such misses with different strategies:
* caching the interesting fields in the scratched space
* avoid accessing at all uninteresting fields
* prefetching

Tested using the udp_sink program by Jesper[1] as the receiver, an h/w l4 rx hash on the ingress nic, so that the number of ingress nic rx queues hit by the udp traffic could be controlled via ethtool -L.

The udp_sink program was bound to the first idle cpu, to get more stable numbers.

On a single numa node receiver:

nic rx queues           vanilla                 patched kernel      delta
1                       1850 kpps               1850 kpps           0%
2                       2370 kpps               2700 kpps           13.9%
16                      2000 kpps               2220 kpps           11%

[1] https://github.com/netoptimizer/network-testing/blob/master/src/udp_sink.c

v1 -> v2:
- replaced secpath_reset() with skb_release_head_state()
- changed udp_dev_scratch fields types to u{32,16} variant, replaced bitfield with bool

Paolo Abeni (3):
net: factor out a helper to decrement the skb refcount
udp: avoid a cache miss on dequeue
udp: try to avoid 2 cache miss on dequeue

 include/linux/skbuff.h |  15 +++++++
 net/core/datagram.c    |   4 +-
 net/core/skbuff.c      |  38 ++++++++++------
 net/ipv4/udp.c         | 120 ++++++++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 148 insertions(+), 29 deletions(-)

Signature · 1 line

2.9.4

Paolo Abeni

author maintainer

2017-06-06 14:24 UTC #1 lore raw

PATCH v2 1/3 net: factor out a helper to decrement the skb refcount

The same code is replicated in 3 different places; move it to a common helper.

Signed-off-by Paolo Abeni <[email protected]>

Acked-by Eric Dumazet <[email protected]>

---
 include/linux/skbuff.h | 13 +++++++++++++
 net/core/datagram.c    |  4 +---
 net/core/skbuff.c      | 14 ++++----------
 3 files changed, 18 insertions(+), 13 deletions(-)

include/linux/skbuff.h +13 -0

index 45a59c1..3c25fca 100644
@@ -867,6 +867,19 @@ static inline unsigned int skb_napi_id(const struct sk_buff *skb)
 #endif
 }
 
+/* decrement the reference count and return true if we can free the skb */
+static inline bool skb_unref(struct sk_buff *skb)
+{
+	if (unlikely(!skb))
+		return false;
+	if (likely(atomic_read(&skb->users) == 1))
+		smp_rmb();
+	else if (likely(!atomic_dec_and_test(&skb->users)))
+		return false;
+
+	return true;
+}
+
 void kfree_skb(struct sk_buff *skb);
 void kfree_skb_list(struct sk_buff *segs);
 void skb_tx_error(struct sk_buff *skb);

net/core/datagram.c +1 -3

index bc46118..e5311a7 100644
@@ -330,9 +330,7 @@ void __skb_free_datagram_locked(struct sock *sk, struct sk_buff *skb, int len)
 {
 	bool slow;
 
-	if (likely(atomic_read(&skb->users) == 1))
-		smp_rmb();
-	else if (likely(!atomic_dec_and_test(&skb->users))) {
+	if (!skb_unref(skb)) {
 		sk_peek_offset_bwd(sk, len);
 		return;
 	}

net/core/skbuff.c +4 ~~-10~~

index 780b7c1..c81b828 100644
@@ -694,12 +694,9 @@ EXPORT_SYMBOL(__kfree_skb);
  */
 void kfree_skb(struct sk_buff *skb)
 {
-	if (unlikely(!skb))
-		return;
-	if (likely(atomic_read(&skb->users) == 1))
-		smp_rmb();
-	else if (likely(!atomic_dec_and_test(&skb->users)))
+	if (!skb_unref(skb))
 		return;
+
 	trace_kfree_skb(skb, __builtin_return_address(0));
 	__kfree_skb(skb);
 }

@@ -746,12 +743,9 @@ EXPORT_SYMBOL(skb_tx_error);
  */
 void consume_skb(struct sk_buff *skb)
 {
-	if (unlikely(!skb))
-		return;
-	if (likely(atomic_read(&skb->users) == 1))
-		smp_rmb();
-	else if (likely(!atomic_dec_and_test(&skb->users)))
+	if (!skb_unref(skb))
 		return;
+
 	trace_consume_skb(skb);
 	__kfree_skb(skb);
 }

Signature · 1 line

2.9.4

Paolo Abeni

author maintainer

2017-06-06 14:24 UTC #2 lore raw

PATCH v2 2/3 udp: avoid a cache miss on dequeue

In reply to Paolo Abeni

Since UDP no more uses sk->destructor, we can clear completely the skb head state before enqueuing. Amend and use skb_release_head_state() for that.

All head states share a single cacheline, which is not normally used/accesses on dequeue. We can avoid entirely accessing such cacheline implementing and using in the UDP code a specialized skb free helper which ignores the skb head state.

This saves a cacheline miss at skb deallocation time.

v1 -> v2:
replaced secpath_reset() with skb_release_head_state()

Signed-off-by Paolo Abeni <[email protected]>

---
 include/linux/skbuff.h |  2 ++
 net/core/skbuff.c      | 24 ++++++++++++++++++++----
 net/ipv4/udp.c         |  6 +++++-
 3 files changed, 27 insertions(+), 5 deletions(-)

include/linux/skbuff.h +2 -0

index 3c25fca..ddfb964 100644
@@ -880,10 +880,12 @@ static inline bool skb_unref(struct sk_buff *skb)
 	return true;
 }
 
+void skb_release_head_state(struct sk_buff *skb);
 void kfree_skb(struct sk_buff *skb);
 void kfree_skb_list(struct sk_buff *segs);
 void skb_tx_error(struct sk_buff *skb);
 void consume_skb(struct sk_buff *skb);
+void consume_stateless_skb(struct sk_buff *skb);
 void  __kfree_skb(struct sk_buff *skb);
 extern struct kmem_cache *skbuff_head_cache;

net/core/skbuff.c +20 -4

index c81b828..786cb42 100644
@@ -643,12 +643,10 @@ static void kfree_skbmem(struct sk_buff *skb)
 	kmem_cache_free(skbuff_fclone_cache, fclones);
 }
 
-static void skb_release_head_state(struct sk_buff *skb)
+void skb_release_head_state(struct sk_buff *skb)
 {
 	skb_dst_drop(skb);
-#ifdef CONFIG_XFRM
-	secpath_put(skb->sp);
-#endif
+	secpath_reset(skb);
 	if (skb->destructor) {
 		WARN_ON(in_irq());
 		skb->destructor(skb);

@@ -751,6 +749,24 @@ void consume_skb(struct sk_buff *skb)
 }
 EXPORT_SYMBOL(consume_skb);
 
+/**
+ *	consume_stateless_skb - free an skbuff, assuming it is stateless
+ *	@skb: buffer to free
+ *
+ *	Works like consume_skb(), but this variant assumes that all the head
+ *	states have been already dropped.
+ */
+void consume_stateless_skb(struct sk_buff *skb)
+{
+	if (!skb_unref(skb))
+		return;
+
+	trace_consume_skb(skb);
+	if (likely(skb->head))
+		skb_release_data(skb);
+	kfree_skbmem(skb);
+}
+
 void __kfree_skb_flush(void)
 {
 	struct napi_alloc_cache *nc = this_cpu_ptr(&napi_alloc_cache);

net/ipv4/udp.c +5 -1

index fdcb743..d8b265f 100644
@@ -1359,7 +1359,8 @@ void skb_consume_udp(struct sock *sk, struct sk_buff *skb, int len)
 		sk_peek_offset_bwd(sk, len);
 		unlock_sock_fast(sk, slow);
 	}
-	consume_skb(skb);
+
+	consume_stateless_skb(skb);
 }
 EXPORT_SYMBOL_GPL(skb_consume_udp);

@@ -1739,6 +1740,9 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 		sk_mark_napi_id_once(sk, skb);
 	}
 
+	/* clear all pending head states while they are hot in the cache */
+	skb_release_head_state(skb);
+
 	rc = __udp_enqueue_schedule_skb(sk, skb);
 	if (rc < 0) {
 		int is_udplite = IS_UDPLITE(sk);

Signature · 1 line

2.9.4

Eric Dumazet

Acked-by on v2 2/3

2017-06-06 20:53 UTC #3 lore raw

Re: [PATCH net-next v2 2/3] udp: avoid a cache miss on dequeue

Quoted 10 lines · Paolo Abeni

Since UDP no more uses sk->destructor, we can clear completely the skb head state before enqueuing. Amend and use skb_release_head_state() for that.
All head states share a single cacheline, which is not normally used/accesses on dequeue. We can avoid entirely accessing such cacheline implementing and using in the UDP code a specialized skb free helper which ignores the skb head state.
This saves a cacheline miss at skb deallocation time.

Acked-by Eric Dumazet <[email protected]>

Paolo Abeni

author maintainer

2017-06-06 14:25 UTC #4 lore raw

PATCH v2 3/3 udp: try to avoid 2 cache miss on dequeue

In reply to Paolo Abeni

when udp_recvmsg() is executed, on x86_64 and other archs, most skb fields are on cold cachelines.
If the skb are linear and the kernel don't need to compute the udp csum, only a handful of skb fields are required by udp_recvmsg(). Since we already use skb->dev_scratch to cache hot data, and there are 32 bits unused on 64 bit archs, use such field to cache as much data as we can, and try to prefetch on dequeue the relevant fields that are left out.

This can save up to 2 cache miss per packet.

v1 -> v2:
- changed udp_dev_scratch fields types to u{32,16} variant, replaced bitfield with bool

Signed-off-by Paolo Abeni <[email protected]>

---
 net/ipv4/udp.c | 114 +++++++++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 103 insertions(+), 11 deletions(-)

net/ipv4/udp.c +103 ~~-11~~

index d8b265f..2bc638c 100644
@@ -1163,6 +1163,83 @@ int udp_sendpage(struct sock *sk, struct page *page, int offset,
 	return ret;
 }
 
+/* Copy as much information as possible into skb->dev_scratch to avoid
+ * possibly multiple cache miss on dequeue();
+ */
+#if BITS_PER_LONG == 64
+
+/* we can store multiple info here: truesize, len and the bit needed to
+ * compute skb_csum_unnecessary will be on cold cache lines at recvmsg
+ * time.
+ * skb->len can be stored on 16 bits since the udp header has been already
+ * validated and pulled.
+ */
+struct udp_dev_scratch {
+	u32 truesize;
+	u16 len;
+	bool is_linear;
+	bool csum_unnecessary;
+};
+
+static void udp_set_dev_scratch(struct sk_buff *skb)
+{
+	struct udp_dev_scratch *scratch;
+
+	BUILD_BUG_ON(sizeof(struct udp_dev_scratch) > sizeof(long));
+	scratch = (struct udp_dev_scratch *)&skb->dev_scratch;
+	scratch->truesize = skb->truesize;
+	scratch->len = skb->len;
+	scratch->csum_unnecessary = !!skb_csum_unnecessary(skb);
+	scratch->is_linear = !skb_is_nonlinear(skb);
+}
+
+static int udp_skb_truesize(struct sk_buff *skb)
+{
+	return ((struct udp_dev_scratch *)&skb->dev_scratch)->truesize;
+}
+
+static unsigned int udp_skb_len(struct sk_buff *skb)
+{
+	return ((struct udp_dev_scratch *)&skb->dev_scratch)->len;
+}
+
+static bool udp_skb_csum_unnecessary(struct sk_buff *skb)
+{
+	return ((struct udp_dev_scratch *)&skb->dev_scratch)->csum_unnecessary;
+}
+
+static bool udp_skb_is_linear(struct sk_buff *skb)
+{
+	return ((struct udp_dev_scratch *)&skb->dev_scratch)->is_linear;
+}
+
+#else
+static void udp_set_dev_scratch(struct sk_buff *skb)
+{
+	skb->dev_scratch = skb->truesize;
+}
+
+static int udp_skb_truesize(struct sk_buff *skb)
+{
+	return skb->dev_scratch;
+}
+
+static unsigned int udp_skb_len(struct sk_buff *skb)
+{
+	return skb->len;
+}
+
+static bool udp_skb_csum_unnecessary(struct sk_buff *skb)
+{
+	return skb_csum_unnecessary(skb);
+}
+
+static bool udp_skb_is_linear(struct sk_buff *skb)
+{
+	return !skb_is_nonlinear(skb);
+}
+#endif
+
 /* fully reclaim rmem/fwd memory allocated for skb */
 static void udp_rmem_release(struct sock *sk, int size, int partial,
 			     bool rx_queue_lock_held)

@@ -1213,14 +1290,16 @@ static void udp_rmem_release(struct sock *sk, int size, int partial,
  */
 void udp_skb_destructor(struct sock *sk, struct sk_buff *skb)
 {
-	udp_rmem_release(sk, skb->dev_scratch, 1, false);
+	prefetch(&skb->data);
+	udp_rmem_release(sk, udp_skb_truesize(skb), 1, false);
 }
 EXPORT_SYMBOL(udp_skb_destructor);
 
 /* as above, but the caller held the rx queue lock, too */
 static void udp_skb_dtor_locked(struct sock *sk, struct sk_buff *skb)
 {
-	udp_rmem_release(sk, skb->dev_scratch, 1, true);
+	prefetch(&skb->data);
+	udp_rmem_release(sk, udp_skb_truesize(skb), 1, true);
 }
 
 /* Idea of busylocks is to let producers grab an extra spinlock

@@ -1274,10 +1353,7 @@ int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb)
 		busy = busylock_acquire(sk);
 	}
 	size = skb->truesize;
-	/* Copy skb->truesize into skb->dev_scratch to avoid a cache line miss
-	 * in udp_skb_destructor()
-	 */
-	skb->dev_scratch = size;
+	udp_set_dev_scratch(skb);
 
 	/* we drop only if the receive buf is full and the receive
 	 * queue contains some other skb

@@ -1515,6 +1591,18 @@ struct sk_buff *__skb_recv_udp(struct sock *sk, unsigned int flags,
 }
 EXPORT_SYMBOL_GPL(__skb_recv_udp);
 
+static int copy_linear_skb(struct sk_buff *skb, int len, int off,
+			   struct iov_iter *to)
+{
+	int n, copy = len - off;
+
+	n = copy_to_iter(skb->data + off, copy, to);
+	if (n == copy)
+		return 0;
+
+	return -EFAULT;
+}
+
 /*
  * 	This should be easy, if there is something there we
  * 	return it, otherwise we block.

@@ -1541,7 +1629,7 @@ int udp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int noblock,
 	if (!skb)
 		return err;
 
-	ulen = skb->len;
+	ulen = udp_skb_len(skb);
 	copied = len;
 	if (copied > ulen - off)
 		copied = ulen - off;

@@ -1556,14 +1644,18 @@ int udp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int noblock,
 
 	if (copied < ulen || peeking ||
 	    (is_udplite && UDP_SKB_CB(skb)->partial_cov)) {
-		checksum_valid = !udp_lib_checksum_complete(skb);
+		checksum_valid = udp_skb_csum_unnecessary(skb) ||
+				!__udp_lib_checksum_complete(skb);
 		if (!checksum_valid)
 			goto csum_copy_err;
 	}
 
-	if (checksum_valid || skb_csum_unnecessary(skb))
-		err = skb_copy_datagram_msg(skb, off, msg, copied);
-	else {
+	if (checksum_valid || udp_skb_csum_unnecessary(skb)) {
+		if (udp_skb_is_linear(skb))
+			err = copy_linear_skb(skb, copied, off, &msg->msg_iter);
+		else
+			err = skb_copy_datagram_msg(skb, off, msg, copied);
+	} else {
 		err = skb_copy_and_csum_datagram_msg(skb, off, msg);
 
 		if (err == -EINVAL)

Signature · 1 line

2.9.4

Eric Dumazet

Acked-by on v2 3/3

2017-06-06 20:58 UTC #5 lore raw

Re: [PATCH net-next v2 3/3] udp: try to avoid 2 cache miss on dequeue

Quoted 8 lines · Paolo Abeni

when udp_recvmsg() is executed, on x86_64 and other archs, most skb fields are on cold cachelines.
If the skb are linear and the kernel don't need to compute the udp csum, only a handful of skb fields are required by udp_recvmsg(). Since we already use skb->dev_scratch to cache hot data, and there are 32 bits unused on 64 bit archs, use such field to cache as much data as we can, and try to prefetch on dequeue the relevant fields that are left out.

Acked-by Eric Dumazet <[email protected]>

Paolo Abeni

author maintainer

2017-06-07 07:56 UTC #6 lore raw

Re: [PATCH net-next v2 3/3] udp: try to avoid 2 cache miss on dequeue

In reply to Paolo Abeni

Hi David,

Quoted 9 lines · Paolo Abeni

when udp_recvmsg() is executed, on x86_64 and other archs, most skb fields are on cold cachelines.
If the skb are linear and the kernel don't need to compute the udp csum, only a handful of skb fields are required by udp_recvmsg(). Since we already use skb->dev_scratch to cache hot data, and there are 32 bits unused on 64 bit archs, use such field to cache as much data as we can, and try to prefetch on dequeue the relevant fields that are left out.

Quoted 7 lines

This can save up to 2 cache miss per packet.
v1 -> v2:
- changed udp_dev_scratch fields types to u{32,16} variant, replaced bitfield with bool
Signed-off-by Paolo Abeni <[email protected]>

Can you please keep on-hold this series a little time? the lkp-robot just reported a performance regression on v1 which I have still to investigate. I can't look at it really soon, but I expect the same should apply to v2.

It sounds quite weird to me, since the bisected patch touches the UDP code only and the regression is on apachebench.

Thank you,

Paolo

David Miller

maintainer

2017-06-07 14:11 UTC #7 lore raw

Re: [PATCH net-next v2 3/3] udp: try to avoid 2 cache miss on dequeue

From: Paolo Abeni <[email protected]>
Date: Wed, 07 Jun 2017 09:56:45 +0200

Quoted 20 lines

Hi David,
Quoted 9 lines · Paolo Abeni
when udp_recvmsg() is executed, on x86_64 and other archs, most skb fields are on cold cachelines.
If the skb are linear and the kernel don't need to compute the udp csum, only a handful of skb fields are required by udp_recvmsg(). Since we already use skb->dev_scratch to cache hot data, and there are 32 bits unused on 64 bit archs, use such field to cache as much data as we can, and try to prefetch on dequeue the relevant fields that are left out.
Quoted 7 lines
This can save up to 2 cache miss per packet.
v1 -> v2:
- changed udp_dev_scratch fields types to u{32,16} variant, replaced bitfield with bool
Signed-off-by Paolo Abeni <[email protected]>

Quoted 7 lines

Can you please keep on-hold this series a little time? the lkp-robot just reported a performance regression on v1 which I have still to investigate. I can't look at it really soon, but I expect the same should apply to v2.
It sounds quite weird to me, since the bisected patch touches the UDP code only and the regression is on apachebench.

Hmmm, DNS lookups?

Thanks for looking into this.

Paolo Abeni

author maintainer

2017-06-09 15:44 UTC #8 lore raw

Re: [PATCH net-next v2 3/3] udp: try to avoid 2 cache miss on dequeue

Hi,

Quoted 28 lines · David Miller

From: Paolo Abeni <[email protected]>
Date: Wed, 07 Jun 2017 09:56:45 +0200
Quoted 25 lines
Hi David,
Quoted 9 lines · Paolo Abeni
when udp_recvmsg() is executed, on x86_64 and other archs, most skb fields are on cold cachelines.
If the skb are linear and the kernel don't need to compute the udp csum, only a handful of skb fields are required by udp_recvmsg(). Since we already use skb->dev_scratch to cache hot data, and there are 32 bits unused on 64 bit archs, use such field to cache as much data as we can, and try to prefetch on dequeue the relevant fields that are left out.
Quoted 7 lines
This can save up to 2 cache miss per packet.
v1 -> v2:
- changed udp_dev_scratch fields types to u{32,16} variant, replaced bitfield with bool
Signed-off-by Paolo Abeni <[email protected]>
Can you please keep on-hold this series a little time? the lkp-robot just reported a performance regression on v1 which I have still to investigate. I can't look at it really soon, but I expect the same should apply to v2.

Quoted 6 lines

Quoted 2 lines
It sounds quite weird to me, since the bisected patch touches the UDP code only and the regression is on apachebench.
Hmmm, DNS lookups?
Thanks for looking into this.

I spent a little time trying to reproduce the regression. There are not
DNS requests during the test, because it's done against the loopback
address (verified with perf probe on UDP code).

I collected several samples for both the patched and vanilla kernels, and I measured a lot of variance (while using the same kernel) - well above 21% - and a similar results distribution when comparing vanilla to patched kernel.

I notified the lkp ML of the above, and I think this is actually a test-suite artifact.

I'll re-submit v3 unchanged, if there are no objections.

Cheers,

Paolo

David Miller

maintainer

2017-06-09 16:33 UTC #9 lore raw

Re: [PATCH net-next v2 3/3] udp: try to avoid 2 cache miss on dequeue

From: Paolo Abeni <[email protected]>
Date: Fri, 09 Jun 2017 17:44:29 +0200

Quoted 1 line

I'll re-submit v3 unchanged, if there are no objections.

No objections from me.