net: Fix struct sock bitfield annotation

Since commit a98b65a3 (net: annotate struct sock bitfield), we lost 8 bytes in struct sock on 64bits arches because of kmemcheck_bitfield_end(flags) misplacement.

struct good {
	int		begin_flags[0];
	unsigned char	a : 8;
	unsigned char	b;
	unsigned short	c;
	int		end_flags[0];
	int		sk_rcvbuf;
	void           *ptr;
};
struct bad {
	int		begin_flags[0];
	unsigned char	a : 8;
	int		end_flags[0];
	unsigned char	b;
	unsigned short	c;
	int		sk_rcvbuf;
	void           *ptr;
};

sizeof(struct good) = 16, sizeof(struct bad) = 24

---
 include/net/sock.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 1621935..ecfb831 100644
@@ -229,9 +229,9 @@ struct sock {
 	unsigned char		sk_shutdown : 2,
 				sk_no_check : 2,
 				sk_userlocks : 4;
-	kmemcheck_bitfield_end(flags);
 	unsigned char		sk_protocol;
 	unsigned short		sk_type;
+	kmemcheck_bitfield_end(flags);
 	int			sk_rcvbuf;
 	socket_lock_t		sk_lock;
 	/*

From: Eric Dumazet <[email protected]>
Date: Thu, 08 Oct 2009 17:16:13 +0200

...

Good catch, thanks Eric.

Applied to net-2.6

2009/10/8 Eric Dumazet <[email protected]>:

From: Vegard Nossum <[email protected]>
Date: Thu, 8 Oct 2009 23:54:02 +0200

Hmmm, indeed.

Eric I'm holding this patch off for now.

What looks wrong to me is the original commit :)

Point is we should not lose 8 bytes with kmemcheck on or off. I believe kmemcheck macros are fine as they are.

When we have a structure with

        unsigned char           sk_shutdown : 2,
                                sk_no_check : 2,
                                sk_userlocks : 4;
        unsigned char           sk_protocol;
        unsigned short          sk_type;

Its pretty clear its *logically* a bitfield aggregation, or if you prefer :

unsigned int sk_shutdown : 2,
sk_no_check : 2,
sk_userlocks : 4,
sk_protocol : 8,
sk_type : 16;

Only difference is that in the second form, you cannot use offsetof(struct sock, sk_type)

I am currently writing a tool to re-organize 'struct sock' fields, for net-next-2.6 using offsetof() macro, this is how I spot the problem.

Thanks

For networking guys, here is the actual mess with "struct sock" on x86_64, related to UDP handling (critical latencies for some people). We basically touch all cache lines, in every paths, bad effects on SMP...

- rx softirq reception
- rx recvmsg() time
- tx sendto() time
- tx completion time

sizeof(struct sk_buff) =232 (0x e8)
sizeof(struct sock) =528 (0x210)
sizeof(struct inet_sock)=680 (0x2a8)

Following offsets in hexadecimal

offsetof(struct sock, sk_refcnt) = 10 (rw by reception of udp frame, __udp4_lib_lookup())
(unavoidable hot spot unfortunatly, but not anymore touched by sock_wfree())

offsetof(struct sock, sk_hash)         = 14   (read  rx softirq )
offsetof(struct sock, sk_family)       = 18   (read   "     "    
offsetof(struct inet_sock, daddr)      =210   (read   "     "   
offsetof(struct inet_sock, rcv_saddr)  =214   (read   "     "  
offsetof(struct inet_sock, dport)      =218   (read   "     " 
offsetof(struct inet_sock, mc_list)    =240   (read   "       multicast reception
offsetof(struct sock, sk_bound_dev_if) = 1c   (read  rx softirq)

(First patch I'll submit is move daddr/rcv_saddr/dport to struct sock_common,
so that lookup() use one cache line instead of two per socket in hash chain)

offsetof(struct sock, sk_prot)         = 30   (read by sk_has_account())
offsetof(struct sock, sk_rcvbuf)       = 3c   (read)
offsetof(struct sock, sk_protocol)     = 39
offsetof(struct sock, sk_allocation)   = e0   (read at send() time)
offsetof(struct sock, sk_flags)        = f8   (read)
offsetof(struct sock, sk_lock)         = 40   (rw by udp_sendmsg())
offsetof(struct sock, sk_dst_lock)     = 90   (rw by udp_sendmsg() on connected socks)
offsetof(struct sock, sk_dst_cache)    = 78   (read by udp_sendmsg() on connected socks)
offsetof(struct sock, sk_mark)         =1d8   (read at sendto() time)
offsetof(struct sock, sk_write_queue)  = c0   (rw by sendto())
offsetof(struct inet_sock, id)         =232   (rw by sendto() on connected socks)
offsetof(struct sock, sk_wmem_alloc)   = 98   (RW, both at sendto() and tx completion handler time)
offsetof(struct sock, sk_sndbuf)       = a0   (read at tx completion time and sendto())
offsetof(struct sock, sk_sndmsg_page)  =1b8   (rw by send())
offsetof(struct sock, sk_send_head)    =1c0   (rw by send(), tcp)

offsetof(struct sock, sk_rmem_alloc)   = 94   (RW, both when frame is received by softirq and dequeued at recvmsg() time)
offsetof(struct sock, sk_receive_queue)= a8   (RW, both when frame is received by softirq and dequeued at recvmsg() time)
offsetof(struct sock, sk_forward_alloc)= dc   (rw rx softirq and recvmsg() time)
offsetof(struct sock, sk_drops)        =134   (read when udp frame is received in softirq handler)
offsetof(struct sock, sk_stamp)        =1a0   (write at recvmsg() time)
offsetof(struct sock, sk_sleep)        = 70   (read by softirq handlers (rx/tx))
offsetof(struct sock, sk_filter)       =160   (read when udp frame is received in softirq handler)
offsetof(struct sock, sk_socket)       =1a8   (read)
offsetof(struct sock, sk_callback_lock)=128   (rw at softirq time
offsetof(struct sock, sk_data_ready)   =1e8   (read)
offsetof(struct sock, sk_write_space)  =1f0   (read at TX completion time)

used by TCP
offsetof(struct sock, sk_timer) =170
offsetof(struct sock, sk_ack_backlog) =138 (listen socks)

Almost never used
offsetof(struct sock, sk_lingertime) =100
offsetof(struct sock, sk_write_pending)=1cc
offsetof(struct sock, sk_prot_creator )=120

Please keep me posted on this. I am very interested in this work.

Some simple shuffling around may do some good here.

Sure, will do, but first I want to suppress the lock_sock()/release_sock() in rx path, that was added for sk_forward_alloc thing. This really hurts, because of the backlog handling.

I have preliminary patch that restore UDP latencies we had in the past ;)

Trick is for UDP, sk_forward_alloc is not updated by tx/rx, only rx. So we can use the sk_receive_queue.lock to forbid concurrent updates.

As this lock is already hot and only used by rx, we wont have to dirty the sk_lock, that will only be used by tx path.

Then we can carefuly reorder struct sock to lower number of cache lines needed for each path.

Patch against linux-2.6 git tree

 net/core/sock.c |    9 ++++
 net/ipv4/udp.c  |   89 ++++++++++++++++++++++------------------------
 2 files changed, 51 insertions(+), 47 deletions(-)

index 7626b6a..45212d4 100644
@@ -276,6 +276,7 @@ int sock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 {
 	int err = 0;
 	int skb_len;
+	unsigned long flags;
 
 	/* Cast sk->rcvbuf to unsigned... It's pointless, but reduces
 	   number of warnings when compiling with -W --ANK

@@ -290,8 +291,12 @@ int sock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 	if (err)
 		goto out;
 
+	skb_orphan(skb);
+
+	spin_lock_irqsave(&sk->sk_receive_queue.lock, flags);
 	if (!sk_rmem_schedule(sk, skb->truesize)) {
 		err = -ENOBUFS;
+		spin_unlock_irqrestore(&sk->sk_receive_queue.lock, flags);
 		goto out;
 	}
 

@@ -305,7 +310,9 @@ int sock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 	 */
 	skb_len = skb->len;
 
-	skb_queue_tail(&sk->sk_receive_queue, skb);
+	__skb_queue_tail(&sk->sk_receive_queue, skb);
+
+	spin_unlock_irqrestore(&sk->sk_receive_queue.lock, flags);
 
 	if (!sock_flag(sk, SOCK_DEAD))
 		sk->sk_data_ready(sk, skb_len);

[RFC net-next-2.6] udp: Dont use lock_sock()/release_sock() in rx path

We added two years ago memory accounting for UDP and some people complain about increased latencies, especially on multicast.

Because sk_forward_alloc is not atomic, we duplicated the protection we used for TCP, ie use lock_sock()/release_sock() to guard sk_forward_alloc against concurrent updates.

When a frame is received by NIC, sofirq handler has to lock the socket, and eventually has to queue the frame into backlog instead of receive queue.

Then, user application also has to lock socket to dequeue a frame from receive_queue and eventually process backlog queue, leading to high latencies.

This lock is also used in tx path to guard cork structure against concurrent updates.

This cause false sharing of socket lock between several cpus that could be avoided, considering UDP touches sk_forward_alloc only on rx. (TCP use it both for rx and tx)

Instead of using socket lock, we can use the sk_receive.lock lock that we have to get anyway to queue frame at softirq time (or dequeue it by user application)

This avoids two atomic ops per packet in softirq handler, one in user app doing recvmsg(), and we dont touch backlog anymore.

This way, the socket lock is only used in tx path, as in the past, and we can improve things by reordering struct sock fields into separate rx/tx groups, in a followup patch.

---
 core/sock.c |    6 +++++-
 ipv4/udp.c  |   34 ++++++++--------------------------
 ipv6/udp.c  |   36 ++++++++++--------------------------
 3 files changed, 23 insertions(+), 53 deletions(-)

index 43ca2c9..d06b1a0 100644
@@ -292,8 +292,13 @@ int sock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 	if (err)
 		goto out;
 
+	skb_orphan(skb);
+
+	spin_lock_irqsave(&list->lock, flags);
+
 	if (!sk_rmem_schedule(sk, skb->truesize)) {
 		err = -ENOBUFS;
+		spin_unlock_irqrestore(&list->lock, flags);
 		goto out;
 	}
 

@@ -307,7 +312,6 @@ int sock_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 	 */
 	skb_len = skb->len;
 
-	spin_lock_irqsave(&list->lock, flags);
 	skb->dropcount = atomic_read(&sk->sk_drops);
 	__skb_queue_tail(list, skb);
 	spin_unlock_irqrestore(&list->lock, flags);

index ee61b3f..f62cec3 100644
@@ -855,29 +855,21 @@ out:
  */
 static unsigned int first_packet_length(struct sock *sk)
 {
-	struct sk_buff_head list_kill, *rcvq = &sk->sk_receive_queue;
+	struct sk_buff_head *rcvq = &sk->sk_receive_queue;
 	struct sk_buff *skb;
 	unsigned int res;
 
-	__skb_queue_head_init(&list_kill);
-
 	spin_lock_bh(&rcvq->lock);
 	while ((skb = skb_peek(rcvq)) != NULL &&
 		udp_lib_checksum_complete(skb)) {
 		UDP_INC_STATS_BH(sock_net(sk), UDP_MIB_INERRORS,
 				 IS_UDPLITE(sk));
 		__skb_unlink(skb, rcvq);
-		__skb_queue_tail(&list_kill, skb);
+		skb_kill_datagram(sk, skb, 0);
 	}
 	res = skb ? skb->len : 0;
 	spin_unlock_bh(&rcvq->lock);
 
-	if (!skb_queue_empty(&list_kill)) {
-		lock_sock(sk);
-		__skb_queue_purge(&list_kill);
-		sk_mem_reclaim_partial(sk);
-		release_sock(sk);
-	}
 	return res;
 }
 

@@ -1003,17 +995,17 @@ try_again:
 		err = ulen;
 
 out_free:
-	lock_sock(sk);
+	spin_lock_bh(&sk->sk_receive_queue.lock);
 	skb_free_datagram(sk, skb);
-	release_sock(sk);
+	spin_unlock_bh(&sk->sk_receive_queue.lock);
 out:
 	return err;
 
 csum_copy_err:
-	lock_sock(sk);
+	spin_lock_bh(&sk->sk_receive_queue.lock);
 	if (!skb_kill_datagram(sk, skb, flags))
-		UDP_INC_STATS_USER(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
-	release_sock(sk);
+		UDP_INC_STATS_BH(sock_net(sk), UDP_MIB_INERRORS, is_udplite);
+	spin_unlock_bh(&sk->sk_receive_queue.lock);
 
 	if (noblock)
 		return -EAGAIN;

@@ -1095,7 +1087,6 @@ drop:
 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);
 
 	/*

@@ -1175,16 +1166,7 @@ int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
 			goto drop;
 	}
 
-	rc = 0;
-
-	bh_lock_sock(sk);
-	if (!sock_owned_by_user(sk))
-		rc = __udp_queue_rcv_skb(sk, skb);
-	else
-		sk_add_backlog(sk, skb);
-	bh_unlock_sock(sk);
-
-	return rc;
+	return __udp_queue_rcv_skb(sk, skb);
 
 drop:
 	UDP_INC_STATS_BH(sock_net(sk), UDP_MIB_INERRORS, is_udplite);

index 1f8e2af..07468aa 100644
@@ -288,23 +288,23 @@ try_again:
 		err = ulen;
 
 out_free:
-	lock_sock(sk);
+	spin_lock_bh(&sk->sk_receive_queue.lock);
 	skb_free_datagram(sk, skb);
-	release_sock(sk);
+	spin_unlock_bh(&sk->sk_receive_queue.lock);
 out:
 	return err;
 
 csum_copy_err:
-	lock_sock(sk);
+	spin_lock_bh(&sk->sk_receive_queue.lock);
 	if (!skb_kill_datagram(sk, skb, flags)) {
 		if (is_udp4)
-			UDP_INC_STATS_USER(sock_net(sk),
+			UDP_INC_STATS_BH(sock_net(sk),
 					UDP_MIB_INERRORS, is_udplite);
 		else
-			UDP6_INC_STATS_USER(sock_net(sk),
+			UDP6_INC_STATS_BH(sock_net(sk),
 					UDP_MIB_INERRORS, is_udplite);
 	}
-	release_sock(sk);
+	spin_unlock_bh(&sk->sk_receive_queue.lock);
 
 	if (flags & MSG_DONTWAIT)
 		return -EAGAIN;

@@ -468,21 +468,10 @@ static int __udp6_lib_mcast_deliver(struct net *net, struct sk_buff *skb,
 	while ((sk2 = udp_v6_mcast_next(net, sk_nulls_next(sk2), uh->dest, daddr,
 					uh->source, saddr, dif))) {
 		struct sk_buff *buff = skb_clone(skb, GFP_ATOMIC);
-		if (buff) {
-			bh_lock_sock(sk2);
-			if (!sock_owned_by_user(sk2))
-				udpv6_queue_rcv_skb(sk2, buff);
-			else
-				sk_add_backlog(sk2, buff);
-			bh_unlock_sock(sk2);
-		}
+		if (buff)
+			udpv6_queue_rcv_skb(sk2, buff);
 	}
-	bh_lock_sock(sk);
-	if (!sock_owned_by_user(sk))
-		udpv6_queue_rcv_skb(sk, skb);
-	else
-		sk_add_backlog(sk, skb);
-	bh_unlock_sock(sk);
+	udpv6_queue_rcv_skb(sk, skb);
 out:
 	spin_unlock(&hslot->lock);
 	return 0;

@@ -597,12 +586,7 @@ int __udp6_lib_rcv(struct sk_buff *skb, struct udp_table *udptable,
 
 	/* deliver */
 
-	bh_lock_sock(sk);
-	if (!sock_owned_by_user(sk))
-		udpv6_queue_rcv_skb(sk, skb);
-	else
-		sk_add_backlog(sk, skb);
-	bh_unlock_sock(sk);
+	udpv6_queue_rcv_skb(sk, skb);
 	sock_put(sk);
 	return 0;
 

From: Eric Dumazet <[email protected]>
Date: Fri, 09 Oct 2009 03:07:56 +0200

I think from a practical standpoint, you are right.

But Vegard is right too, as we should be able to put the annotation right next to the ":" statements.

So if you really want why don't you put the sk_protocol and sk_type into the ":" block as you mentioned.

And then you can use Arnaldo's 'pahole' instead of the kludgy offsetof() which doesn't work with bitfields :-)

I want the 8 bytes back just like you, but seperating the annotation from the real C bitfields looks definitely wrong to me.

Let's hope nobody wants to use &sk->sk_protocol, &sk->sk_type, (or offsetof(..., sk_somefield) if that matters)

Only compile tested on 'allyesconfig' build on x86_64

[PATCH] net: Fix struct sock bitfield annotation

Since commit a98b65a3 (net: annotate struct sock bitfield), we lost 8 bytes in struct sock on 64bit arches because of kmemcheck_bitfield_end(flags) misplacement.

Fix this by putting together sk_shutdown, sk_no_check, sk_userlocks, sk_protocol and sk_type in the 'flags' 32bits bitfield

---
 include/net/sock.h |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

index 1621935..9f96394 100644
@@ -226,12 +226,12 @@ struct sock {
 #define sk_prot			__sk_common.skc_prot
 #define sk_net			__sk_common.skc_net
 	kmemcheck_bitfield_begin(flags);
-	unsigned char		sk_shutdown : 2,
-				sk_no_check : 2,
-				sk_userlocks : 4;
+	unsigned int		sk_shutdown  : 2,
+				sk_no_check  : 2,
+				sk_userlocks : 4,
+				sk_protocol  : 8,
+				sk_type      : 16;
 	kmemcheck_bitfield_end(flags);
-	unsigned char		sk_protocol;
-	unsigned short		sk_type;
 	int			sk_rcvbuf;
 	socket_lock_t		sk_lock;
 	/*

From: Eric Dumazet <[email protected]>
Date: Fri, 09 Oct 2009 10:50:25 +0200

Applied, thanks Eric.