udp: refactor memory accounting

PATCH In review Paolo Abeni 5 participants 11 messages Oct 21, 2016 – Oct 22, 2016 6 messages not in the archive

Subsystems (6)

Networking Network drivers NFS client NFS server +2 more subsystems

lore

Share & tools

lore

Paolo Abeni

2016-10-21 11:55 UTC #2 lore raw

PATCH v6 3/3 udp: use it's own memory accounting schema

In reply to a message not in the archive

Completely avoid default sock memory accounting and replace it with udp-specific accounting.

Since the new memory accounting model encapsulates completely the required locking, remove the socket lock on both enqueue and dequeue, and avoid using the backlog on enqueue.

Be sure to clean-up rx queue memory on socket destruction, using udp its own sk_destruct.

Tested using pktgen with random src port, 64 bytes packet, wire-speed on a 10G link as sender and udp_sink as the receiver, using an l4 tuple rxhash to stress the contention, and one or more udp_sink instances with reuseport.

nr readers      Kpps (vanilla)  Kpps (patched)
1               170             440
3               1250            2150
6               3000            3650
9               4200            4450
12              5700            6250

v4 -> v5:
- avoid unneeded test in first_packet_length

v3 -> v4:
- remove useless sk_rcvqueues_full() call

v2 -> v3:
- do not set the now unsed backlog_rcv callback

v1 -> v2:
- add memory pressure support
- fixed dropwatch accounting for ipv6

Acked-by Hannes Frederic Sowa <[email protected]>

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

---
 net/ipv4/udp.c        | 43 ++++++++-----------------------------------
 net/ipv6/udp.c        | 34 ++++++++--------------------------
 net/sunrpc/svcsock.c  | 20 ++++++++++++++++----
 net/sunrpc/xprtsock.c |  2 +-
 4 files changed, 33 insertions(+), 66 deletions(-)

net/ipv4/udp.c +8 ~~-35~~

index e137e1d..b38269d 100644
@@ -1307,13 +1307,7 @@ static int first_packet_length(struct sock *sk)
 	res = skb ? skb->len : -1;
 	spin_unlock_bh(&rcvq->lock);
 
-	if (!skb_queue_empty(&list_kill)) {
-		bool slow = lock_sock_fast(sk);
-
-		__skb_queue_purge(&list_kill);
-		sk_mem_reclaim_partial(sk);
-		unlock_sock_fast(sk, slow);
-	}
+	__skb_queue_purge(&list_kill);
 	return res;
 }

@@ -1362,7 +1356,6 @@ int udp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int noblock,
 	int err;
 	int is_udplite = IS_UDPLITE(sk);
 	bool checksum_valid = false;
-	bool slow;
 
 	if (flags & MSG_ERRQUEUE)
 		return ip_recv_error(sk, msg, len, addr_len);

@@ -1403,13 +1396,12 @@ int udp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int noblock,
 	}
 
 	if (unlikely(err)) {
-		trace_kfree_skb(skb, udp_recvmsg);
 		if (!peeked) {
 			atomic_inc(&sk->sk_drops);
 			UDP_INC_STATS(sock_net(sk),
 				      UDP_MIB_INERRORS, is_udplite);
 		}
-		skb_free_datagram_locked(sk, skb);
+		kfree_skb(skb);
 		return err;
 	}

@@ -1434,16 +1426,15 @@ int udp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len, int noblock,
 	if (flags & MSG_TRUNC)
 		err = ulen;
 
-	__skb_free_datagram_locked(sk, skb, peeking ? -err : err);
+	skb_consume_udp(sk, skb, peeking ? -err : err);
 	return err;
 
 csum_copy_err:
-	slow = lock_sock_fast(sk);
-	if (!skb_kill_datagram(sk, skb, flags)) {
+	if (!__sk_queue_drop_skb(sk, skb, flags)) {
 		UDP_INC_STATS(sock_net(sk), UDP_MIB_CSUMERRORS, is_udplite);
 		UDP_INC_STATS(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
 	}
-	unlock_sock_fast(sk, slow);
+	kfree_skb(skb);
 
 	/* starting over for a new packet, but check if we need to yield */
 	cond_resched();

@@ -1562,7 +1553,7 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 		sk_incoming_cpu_update(sk);
 	}
 
-	rc = __sock_queue_rcv_skb(sk, skb);
+	rc = __udp_enqueue_schedule_skb(sk, skb);
 	if (rc < 0) {
 		int is_udplite = IS_UDPLITE(sk);

@@ -1577,7 +1568,6 @@ static int __udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 	}
 
 	return 0;
-
 }
 
 static struct static_key udp_encap_needed __read_mostly;

@@ -1599,7 +1589,6 @@ void udp_encap_enable(void)
 int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 {
 	struct udp_sock *up = udp_sk(sk);
-	int rc;
 	int is_udplite = IS_UDPLITE(sk);
 
 	/*

@@ -1686,25 +1675,9 @@ int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 		goto drop;
 
 	udp_csum_pull_header(skb);
-	if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
-		__UDP_INC_STATS(sock_net(sk), UDP_MIB_RCVBUFERRORS,
-				is_udplite);
-		goto drop;
-	}
-
-	rc = 0;
 
 	ipv4_pktinfo_prepare(sk, skb);
-	bh_lock_sock(sk);
-	if (!sock_owned_by_user(sk))
-		rc = __udp_queue_rcv_skb(sk, skb);
-	else if (sk_add_backlog(sk, skb, sk->sk_rcvbuf)) {
-		bh_unlock_sock(sk);
-		goto drop;
-	}
-	bh_unlock_sock(sk);
-
-	return rc;
+	return __udp_queue_rcv_skb(sk, skb);
 
 csum_error:
 	__UDP_INC_STATS(sock_net(sk), UDP_MIB_CSUMERRORS, is_udplite);

@@ -2314,13 +2287,13 @@ struct proto udp_prot = {
 	.connect	   = ip4_datagram_connect,
 	.disconnect	   = udp_disconnect,
 	.ioctl		   = udp_ioctl,
+	.init		   = udp_init_sock,
 	.destroy	   = udp_destroy_sock,
 	.setsockopt	   = udp_setsockopt,
 	.getsockopt	   = udp_getsockopt,
 	.sendmsg	   = udp_sendmsg,
 	.recvmsg	   = udp_recvmsg,
 	.sendpage	   = udp_sendpage,
-	.backlog_rcv	   = __udp_queue_rcv_skb,
 	.release_cb	   = ip4_datagram_release_cb,
 	.hash		   = udp_lib_hash,
 	.unhash		   = udp_lib_unhash,

net/ipv6/udp.c +8 ~~-26~~

index 9aa7c1c..71963b2 100644
@@ -334,7 +334,6 @@ int udpv6_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 	int is_udplite = IS_UDPLITE(sk);
 	bool checksum_valid = false;
 	int is_udp4;
-	bool slow;
 
 	if (flags & MSG_ERRQUEUE)
 		return ipv6_recv_error(sk, msg, len, addr_len);

@@ -378,7 +377,6 @@ int udpv6_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 			goto csum_copy_err;
 	}
 	if (unlikely(err)) {
-		trace_kfree_skb(skb, udpv6_recvmsg);
 		if (!peeked) {
 			atomic_inc(&sk->sk_drops);
 			if (is_udp4)

@@ -388,7 +386,7 @@ int udpv6_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 				UDP6_INC_STATS(sock_net(sk), UDP_MIB_INERRORS,
 					       is_udplite);
 		}
-		skb_free_datagram_locked(sk, skb);
+		kfree_skb(skb);
 		return err;
 	}
 	if (!peeked) {

@@ -437,12 +435,11 @@ int udpv6_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 	if (flags & MSG_TRUNC)
 		err = ulen;
 
-	__skb_free_datagram_locked(sk, skb, peeking ? -err : err);
+	skb_consume_udp(sk, skb, peeking ? -err : err);
 	return err;
 
 csum_copy_err:
-	slow = lock_sock_fast(sk);
-	if (!skb_kill_datagram(sk, skb, flags)) {
+	if (!__sk_queue_drop_skb(sk, skb, flags)) {
 		if (is_udp4) {
 			UDP_INC_STATS(sock_net(sk),
 				      UDP_MIB_CSUMERRORS, is_udplite);

@@ -455,7 +452,7 @@ int udpv6_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
 				       UDP_MIB_INERRORS, is_udplite);
 		}
 	}
-	unlock_sock_fast(sk, slow);
+	kfree_skb(skb);
 
 	/* starting over for a new packet, but check if we need to yield */
 	cond_resched();

@@ -523,7 +520,7 @@ static int __udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 		sk_incoming_cpu_update(sk);
 	}
 
-	rc = __sock_queue_rcv_skb(sk, skb);
+	rc = __udp_enqueue_schedule_skb(sk, skb);
 	if (rc < 0) {
 		int is_udplite = IS_UDPLITE(sk);

@@ -535,6 +532,7 @@ static int __udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 		kfree_skb(skb);
 		return -1;
 	}
+
 	return 0;
 }

@@ -556,7 +554,6 @@ void udpv6_encap_enable(void)
 int udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 {
 	struct udp_sock *up = udp_sk(sk);
-	int rc;
 	int is_udplite = IS_UDPLITE(sk);
 
 	if (!xfrm6_policy_check(sk, XFRM_POLICY_IN, skb))

@@ -622,25 +619,10 @@ int udpv6_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 		goto drop;
 
 	udp_csum_pull_header(skb);
-	if (sk_rcvqueues_full(sk, sk->sk_rcvbuf)) {
-		__UDP6_INC_STATS(sock_net(sk),
-				 UDP_MIB_RCVBUFERRORS, is_udplite);
-		goto drop;
-	}
 
 	skb_dst_drop(skb);
 
-	bh_lock_sock(sk);
-	rc = 0;
-	if (!sock_owned_by_user(sk))
-		rc = __udpv6_queue_rcv_skb(sk, skb);
-	else if (sk_add_backlog(sk, skb, sk->sk_rcvbuf)) {
-		bh_unlock_sock(sk);
-		goto drop;
-	}
-	bh_unlock_sock(sk);
-
-	return rc;
+	return __udpv6_queue_rcv_skb(sk, skb);
 
 csum_error:
 	__UDP6_INC_STATS(sock_net(sk), UDP_MIB_CSUMERRORS, is_udplite);

@@ -1433,12 +1415,12 @@ struct proto udpv6_prot = {
 	.connect	   = ip6_datagram_connect,
 	.disconnect	   = udp_disconnect,
 	.ioctl		   = udp_ioctl,
+	.init		   = udp_init_sock,
 	.destroy	   = udpv6_destroy_sock,
 	.setsockopt	   = udpv6_setsockopt,
 	.getsockopt	   = udpv6_getsockopt,
 	.sendmsg	   = udpv6_sendmsg,
 	.recvmsg	   = udpv6_recvmsg,
-	.backlog_rcv	   = __udpv6_queue_rcv_skb,
 	.release_cb	   = ip6_datagram_release_cb,
 	.hash		   = udp_lib_hash,
 	.unhash		   = udp_lib_unhash,

net/sunrpc/svcsock.c +16 -4

index 57625f6..e2a55dc 100644
@@ -39,6 +39,7 @@
 #include <net/checksum.h>
 #include <net/ip.h>
 #include <net/ipv6.h>
+#include <net/udp.h>
 #include <net/tcp.h>
 #include <net/tcp_states.h>
 #include <asm/uaccess.h>

@@ -129,6 +130,18 @@ static void svc_release_skb(struct svc_rqst *rqstp)
 	}
 }
 
+static void svc_release_udp_skb(struct svc_rqst *rqstp)
+{
+	struct sk_buff *skb = rqstp->rq_xprt_ctxt;
+
+	if (skb) {
+		rqstp->rq_xprt_ctxt = NULL;
+
+		dprintk("svc: service %p, releasing skb %p\n", rqstp, skb);
+		consume_skb(skb);
+	}
+}
+
 union svc_pktinfo_u {
 	struct in_pktinfo pkti;
 	struct in6_pktinfo pkti6;

@@ -575,7 +588,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
 			goto out_free;
 		}
 		local_bh_enable();
-		skb_free_datagram_locked(svsk->sk_sk, skb);
+		consume_skb(skb);
 	} else {
 		/* we can use it in-place */
 		rqstp->rq_arg.head[0].iov_base = skb->data;

@@ -602,8 +615,7 @@ static int svc_udp_recvfrom(struct svc_rqst *rqstp)
 
 	return len;
 out_free:
-	trace_kfree_skb(skb, svc_udp_recvfrom);
-	skb_free_datagram_locked(svsk->sk_sk, skb);
+	kfree_skb(skb);
 	return 0;
 }

@@ -660,7 +672,7 @@ static struct svc_xprt *svc_udp_create(struct svc_serv *serv,
 	.xpo_create = svc_udp_create,
 	.xpo_recvfrom = svc_udp_recvfrom,
 	.xpo_sendto = svc_udp_sendto,
-	.xpo_release_rqst = svc_release_skb,
+	.xpo_release_rqst = svc_release_udp_skb,
 	.xpo_detach = svc_sock_detach,
 	.xpo_free = svc_sock_free,
 	.xpo_prep_reply_hdr = svc_udp_prep_reply_hdr,

net/sunrpc/xprtsock.c +1 -1

index 0137af1..1758665 100644
@@ -1083,7 +1083,7 @@ static void xs_udp_data_receive(struct sock_xprt *transport)
 		skb = skb_recv_datagram(sk, 0, 1, &err);
 		if (skb != NULL) {
 			xs_udp_data_read_skb(&transport->xprt, sk, skb);
-			skb_free_datagram_locked(sk, skb);
+			consume_skb(skb);
 			continue;
 		}
 		if (!test_and_clear_bit(XPRT_SOCK_DATA_READY, &transport->sock_state))

Signature · 6 lines

1.8.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Paolo Abeni

author maintainer

2016-10-21 11:55 UTC #0 lore raw

v6 cover udp: refactor memory accounting

This patch series refactor the udp memory accounting, replacing the generic implementation with a custom one, in order to remove the needs for locking the socket on the enqueue and dequeue operations. The socket backlog usage is dropped, as well.

The first patch factor out pieces of some queue and memory management socket helpers, so that they can later be used by the udp memory accounting functions.
The second patch adds the memory account helpers, without using them. The third patch replacse the old rx memory accounting path for udp over ipv4 and udp over ipv6. In kernel UDP users are updated, as well.

The memory accounting schema is described in detail in the individual patch commit message.

The performance gain depends on the specific scenario; with few flows (and little contention in the original code) the differences are in the noise range, while with several flows contending the same socket, the measured speed-up is relevant (e.g. even over 100% in case of extreme contention)

Many thanks to Eric Dumazet for the reiterated reviews and suggestions.

v5 -> v6:
- do not orphan the skb on enqueue, skb_steal_sock() already did the work for us

v4 -> v5:
- use the receive queue spin lock to protect the memory accounting
- several minor clean-up

v3 -> v4:
- simplified the locking schema, always use a plain spinlock

v2 -> v3:
- do not set the now unsed backlog_rcv callback

v1 -> v2:
- changed slighly the memory accounting schema, we now perform lazy reclaim
- fixed forward_alloc updating issue
- fixed memory counter integer overflows

Paolo Abeni (3):
net/socket: factor out helpers for memory and queue manipulation
udp: implement memory accounting helpers
udp: use it's own memory accounting schema

 include/net/sock.h    |   4 ++
 include/net/udp.h     |   4 ++
 net/core/datagram.c   |  36 +++++++-----
 net/core/sock.c       |  64 +++++++++++++++-------
 net/ipv4/udp.c        | 149 ++++++++++++++++++++++++++++++++++++++------------
 net/ipv6/udp.c        |  34 +++---------
 net/sunrpc/svcsock.c  |  20 +++++--
 net/sunrpc/xprtsock.c |   2 +-
 8 files changed, 214 insertions(+), 99 deletions(-)

Signature · 1 line

1.8.3.1

Paolo Abeni

author maintainer

2016-10-21 11:55 UTC #6 lore raw

PATCH v6 1/3 net/socket: factor out helpers for memory and queue manipulation

Basic sock operations that udp code can use with its own memory accounting schema. No functional change is introduced in the existing APIs.

v4 -> v5:
- avoid whitespace changes

v2 -> v4:
- avoid exporting __sock_enqueue_skb

v1 -> v2:
- avoid export sock_rmem_free

Acked-by Hannes Frederic Sowa <[email protected]>

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

---
 include/net/sock.h  |  4 ++++
 net/core/datagram.c | 36 ++++++++++++++++++------------
 net/core/sock.c     | 64 +++++++++++++++++++++++++++++++++++++----------------
 3 files changed, 71 insertions(+), 33 deletions(-)

include/net/sock.h +4 -0

index ebf75db..2764895 100644
@@ -1274,7 +1274,9 @@ static inline struct inode *SOCK_INODE(struct socket *socket)
 /*
  * Functions for memory accounting
  */
+int __sk_mem_raise_allocated(struct sock *sk, int size, int amt, int kind);
 int __sk_mem_schedule(struct sock *sk, int size, int kind);
+void __sk_mem_reduce_allocated(struct sock *sk, int amount);
 void __sk_mem_reclaim(struct sock *sk, int amount);
 
 #define SK_MEM_QUANTUM ((int)PAGE_SIZE)

@@ -1950,6 +1952,8 @@ void sk_reset_timer(struct sock *sk, struct timer_list *timer,
 
 void sk_stop_timer(struct sock *sk, struct timer_list *timer);
 
+int __sk_queue_drop_skb(struct sock *sk, struct sk_buff *skb,
+			unsigned int flags);
 int __sock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb);
 int sock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb);

net/core/datagram.c +22 ~~-14~~

index b7de71f..bfb973a 100644
@@ -323,6 +323,27 @@ void __skb_free_datagram_locked(struct sock *sk, struct sk_buff *skb, int len)
 }
 EXPORT_SYMBOL(__skb_free_datagram_locked);
 
+int __sk_queue_drop_skb(struct sock *sk, struct sk_buff *skb,
+			unsigned int flags)
+{
+	int err = 0;
+
+	if (flags & MSG_PEEK) {
+		err = -ENOENT;
+		spin_lock_bh(&sk->sk_receive_queue.lock);
+		if (skb == skb_peek(&sk->sk_receive_queue)) {
+			__skb_unlink(skb, &sk->sk_receive_queue);
+			atomic_dec(&skb->users);
+			err = 0;
+		}
+		spin_unlock_bh(&sk->sk_receive_queue.lock);
+	}
+
+	atomic_inc(&sk->sk_drops);
+	return err;
+}
+EXPORT_SYMBOL(__sk_queue_drop_skb);
+
 /**
  *	skb_kill_datagram - Free a datagram skbuff forcibly
  *	@sk: socket

@@ -346,23 +367,10 @@ void __skb_free_datagram_locked(struct sock *sk, struct sk_buff *skb, int len)
 
 int skb_kill_datagram(struct sock *sk, struct sk_buff *skb, unsigned int flags)
 {
-	int err = 0;
-
-	if (flags & MSG_PEEK) {
-		err = -ENOENT;
-		spin_lock_bh(&sk->sk_receive_queue.lock);
-		if (skb == skb_peek(&sk->sk_receive_queue)) {
-			__skb_unlink(skb, &sk->sk_receive_queue);
-			atomic_dec(&skb->users);
-			err = 0;
-		}
-		spin_unlock_bh(&sk->sk_receive_queue.lock);
-	}
+	int err = __sk_queue_drop_skb(sk, skb, flags);
 
 	kfree_skb(skb);
-	atomic_inc(&sk->sk_drops);
 	sk_mem_reclaim_partial(sk);
-
 	return err;
 }
 EXPORT_SYMBOL(skb_kill_datagram);

net/core/sock.c +45 ~~-19~~

102 lines not on this page · raw message

Signature · 1 line

1.8.3.1

Paolo Abeni

author maintainer

2016-10-21 11:56 UTC #9 lore raw

PATCH v6 2/3 udp: implement memory accounting helpers

In reply to Paolo Abeni

Avoid using the generic helpers.
Use the receive queue spin lock to protect the memory accounting operation, both on enqueue and on dequeue.

On dequeue perform partial memory reclaiming, trying to leave a quantum of forward allocated memory.

On enqueue use a custom helper, to allow some optimizations:
- use a plain spin_lock() variant instead of the slightly costly spin_lock_irqsave(),
- avoid dst_force check, since the calling code has already dropped the skb dst
- avoid orphaning the skb, since skb_steal_sock() already did the work for us

The above needs custom memory reclaiming on shutdown, provided by the udp_destruct_sock().

v5 -> v6:
- don't orphan the skb on enqueue

v4 -> v5:
- replace the mem_lock with the receive queue spin lock
- ensure that the bh is always allowed to enqueue at least a skb, even if sk_rcvbuf is exceeded

v3 -> v4:
- reworked memory accunting, simplifying the schema
- provide an helper for both memory scheduling and enqueuing

v1 -> v2:
- use a udp specific destrctor to perform memory reclaiming
- remove a couple of helpers, unneeded after the above cleanup
- do not reclaim memory on dequeue if not under memory pressure
- reworked the fwd accounting schema to avoid potential integer overflow

Acked-by Hannes Frederic Sowa <[email protected]>

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

---
 include/net/udp.h |   4 +++
 net/ipv4/udp.c    | 106 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 110 insertions(+)

include/net/udp.h +4 -0

index ea53a87..18f1e6b 100644
@@ -246,6 +246,9 @@ static inline __be16 udp_flow_src_port(struct net *net, struct sk_buff *skb,
 }
 
 /* net/ipv4/udp.c */
+void skb_consume_udp(struct sock *sk, struct sk_buff *skb, int len);
+int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb);
+
 void udp_v4_early_demux(struct sk_buff *skb);
 int udp_get_port(struct sock *sk, unsigned short snum,
 		 int (*saddr_cmp)(const struct sock *,

@@ -258,6 +261,7 @@ int udp_get_port(struct sock *sk, unsigned short snum,
 void udp4_hwcsum(struct sk_buff *skb, __be32 src, __be32 dst);
 int udp_rcv(struct sk_buff *skb);
 int udp_ioctl(struct sock *sk, int cmd, unsigned long arg);
+int udp_init_sock(struct sock *sk);
 int udp_disconnect(struct sock *sk, int flags);
 unsigned int udp_poll(struct file *file, struct socket *sock, poll_table *wait);
 struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb,

net/ipv4/udp.c +106 -0

112 lines not on this page · raw message

Signature · 1 line

1.8.3.1

Eric Dumazet

2016-10-21 12:24 UTC #11 lore raw

Re: [PATCH net-next v6 2/3] udp: implement memory accounting helpers

In reply to a message not in the archive

Quoted 27 lines · Paolo Abeni

Avoid using the generic helpers.
Use the receive queue spin lock to protect the memory accounting operation, both on enqueue and on dequeue.
On dequeue perform partial memory reclaiming, trying to leave a quantum of forward allocated memory.
On enqueue use a custom helper, to allow some optimizations:
- use a plain spin_lock() variant instead of the slightly costly spin_lock_irqsave(),
- avoid dst_force check, since the calling code has already dropped the skb dst
- avoid orphaning the skb, since skb_steal_sock() already did the work for us
net/ipv4/udp.c +11 -0
+static void udp_rmem_release(struct sock *sk, int size, int partial)
+{
+	int amt;
+
+	atomic_sub(size, &sk->sk_rmem_alloc);
+
+	spin_lock_bh(&sk->sk_receive_queue.lock);
+	sk->sk_forward_alloc += size;
+	amt = (sk->sk_forward_alloc - partial) & ~(SK_MEM_QUANTUM - 1);
+	sk->sk_forward_alloc -= amt;
+	spin_unlock_bh(&sk->sk_receive_queue.lock);

net/ipv4/udp.c int udp_sendpage(struct sock *sk, struct page *page, int offset, · line 1195

Quoted 10 lines

net/ipv4/udp.c +10 -0

+
+	if (amt)
+		__sk_mem_reduce_allocated(sk, amt >> SK_MEM_QUANTUM_SHIFT);
+}
+
+static void udp_rmem_free(struct sk_buff *skb)
+{
+	udp_rmem_release(skb->sk, skb->truesize, 1);
+}
+

It looks like you are acquiring/releasing sk_receive_queue.lock twice per packet in recvmsg() (the second time in the destructor above)

We could do slightly better if :

We do not set skb->destructor at all, and manage sk_rmem_alloc/sk_forward_alloc changes at the time we dequeue skb
(if !MSG_PEEK), before copy to user space.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Paolo Abeni

2016-10-21 13:34 UTC #13 lore raw

Re: [PATCH net-next v6 2/3] udp: implement memory accounting helpers

In reply to a message not in the archive

Quoted 43 lines · Eric Dumazet

Quoted 27 lines · Paolo Abeni
Avoid using the generic helpers.
Use the receive queue spin lock to protect the memory accounting operation, both on enqueue and on dequeue.
On dequeue perform partial memory reclaiming, trying to leave a quantum of forward allocated memory.
On enqueue use a custom helper, to allow some optimizations:
- use a plain spin_lock() variant instead of the slightly costly spin_lock_irqsave(),
- avoid dst_force check, since the calling code has already dropped the skb dst
- avoid orphaning the skb, since skb_steal_sock() already did the work for us
net/ipv4/udp.c +11 -0
+static void udp_rmem_release(struct sock *sk, int size, int partial)
+{
+	int amt;
+
+	atomic_sub(size, &sk->sk_rmem_alloc);
+
+	spin_lock_bh(&sk->sk_receive_queue.lock);
+	sk->sk_forward_alloc += size;
+	amt = (sk->sk_forward_alloc - partial) & ~(SK_MEM_QUANTUM - 1);
+	sk->sk_forward_alloc -= amt;
+	spin_unlock_bh(&sk->sk_receive_queue.lock);
net/ipv4/udp.c int udp_sendpage(struct sock *sk, struct page *page, int offset, · line 1195
Quoted 10 lines
net/ipv4/udp.c +10 -0
+
+	if (amt)
+		__sk_mem_reduce_allocated(sk, amt >> SK_MEM_QUANTUM_SHIFT);
+}
+
+static void udp_rmem_free(struct sk_buff *skb)
+{
+	udp_rmem_release(skb->sk, skb->truesize, 1);
+}
+
It looks like you are acquiring/releasing sk_receive_queue.lock twice per packet in recvmsg() (the second time in the destructor above)

Quoted 5 lines

We could do slightly better if :
We do not set skb->destructor at all, and manage sk_rmem_alloc/sk_forward_alloc changes at the time we dequeue skb
(if !MSG_PEEK), before copy to user space.

Thank you again for reviewing this!

Updating sk_rmem_alloc would still need an atomic operation, because it is touched also by the error queue path: we will end up adding an atomic operation (or two, when reclaiming the fwd allocated memory) inside the critical section. The contention will likely increase.

The above is going to be quite intrusive: we need to pass an additional argument all the way up to __skb_try_recv_datagram() and change the function behavior according to its value.

If you are otherwise satisfied with the series in the current shape, can't we instead build incrementally on top of it ? it will simplify both reviews and re-factor tasks.

Thank you,

Paolo

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Dumazet

2016-10-21 13:41 UTC #15 lore raw

Re: [PATCH net-next v6 2/3] udp: implement memory accounting helpers

In reply to a message not in the archive

Quoted 6 lines · Paolo Abeni

Updating sk_rmem_alloc would still need an atomic operation, because it is touched also by the error queue path: we will end up adding an atomic operation (or two, when reclaiming the fwd allocated memory) inside the critical section. The contention will likely increase.

Quoted 7 lines

The above is going to be quite intrusive: we need to pass an additional argument all the way up to __skb_try_recv_datagram() and change the function behavior according to its value.
If you are otherwise satisfied with the series in the current shape, can't we instead build incrementally on top of it ? it will simplify both reviews and re-factor tasks.

I certainly can provide an incremental patch, it might be easier for both of us ;)

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Dumazet

Acked-by

2016-10-21 21:13 UTC #8 lore raw

Re: [PATCH net-next v6 1/3] net/socket: factor out helpers for memory and queue manipulation

In reply to a message not in the archive

Quoted 10 lines · Paolo Abeni

Basic sock operations that udp code can use with its own memory accounting schema. No functional change is introduced in the existing APIs.
v4 -> v5:
- avoid whitespace changes
v2 -> v4:
- avoid exporting __sock_enqueue_skb

Quoted 6 lines

v1 -> v2:
- avoid export sock_rmem_free
Acked-by Hannes Frederic Sowa <[email protected]>
Signed-off-by Paolo Abeni <[email protected]>
---

Acked-by Eric Dumazet <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Dumazet

Acked-by on v6 2/3

2016-10-21 21:13 UTC #16 lore raw

Re: [PATCH net-next v6 2/3] udp: implement memory accounting helpers

In reply to Paolo Abeni

Quoted 15 lines · Paolo Abeni

Avoid using the generic helpers.
Use the receive queue spin lock to protect the memory accounting operation, both on enqueue and on dequeue.
On dequeue perform partial memory reclaiming, trying to leave a quantum of forward allocated memory.
On enqueue use a custom helper, to allow some optimizations:
- use a plain spin_lock() variant instead of the slightly costly spin_lock_irqsave(),
- avoid dst_force check, since the calling code has already dropped the skb dst
- avoid orphaning the skb, since skb_steal_sock() already did the work for us

Quoted 2 lines

The above needs custom memory reclaiming on shutdown, provided by the udp_destruct_sock().

Acked-by Eric Dumazet <[email protected]>

Eric Dumazet

Acked-by

2016-10-21 21:14 UTC #4 lore raw

Re: [PATCH net-next v6 3/3] udp: use it's own memory accounting schema

In reply to a message not in the archive

Quoted 10 lines · Paolo Abeni

Completely avoid default sock memory accounting and replace it with udp-specific accounting.
Since the new memory accounting model encapsulates completely the required locking, remove the socket lock on both enqueue and dequeue, and avoid using the backlog on enqueue.
Be sure to clean-up rx queue memory on socket destruction, using udp its own sk_destruct.

Quoted 7 lines

Tested using pktgen with random src port, 64 bytes packet, wire-speed on a 10G link as sender and udp_sink as the receiver, using an l4 tuple rxhash to stress the contention, and one or more udp_sink instances with reuseport.
nr readers      Kpps (vanilla)  Kpps (patched)
1               170             440

Acked-by Eric Dumazet <[email protected]>

Thanks a lot guys.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

David Miller

2016-10-22 20:50 UTC #5 lore raw

In reply to a message not in the archive

From: Paolo Abeni <[email protected]>
Date: Fri, 21 Oct 2016 13:55:44 +0200

Quoted 4 lines

This patch series refactor the udp memory accounting, replacing the generic implementation with a custom one, in order to remove the needs for locking the socket on the enqueue and dequeue operations. The socket backlog usage is dropped, as well.

...

Series applied, thank you.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html