Skip to contentSkip to conversation

memory barrier cleanups

RFC In review Ralf Baechle 2 participants 3 messages Dec 6, 2006 – Dec 7, 2006

Subsystems (4)
Networking Network drivers TCP Packet sockets
lore
Share & tools
lore

Ralf Baechle

author
#0 lore raw

RFC 1/1 memory barrier cleanups

I believe all the below memory barriers only matter on SMP so therefore the smp_* variant of the barrier should be used.

I'm wondering if the barrier in net/ipv4/inet_timewait_sock.c should be dropped entirely. schedule_work's implementation currently implies a memory barrier and I think sane semantics of schedule_work() should imply a memory barrier, as needed so the caller shouldn't have to worry. It's not quite obvious why the barrier in net/packet/af_packet.c is needed; maybe it should be implied through flush_dcache_page?

Ralf

Signed-off-by Ralf Baechle <[email protected]>
net/core/wireless.c +2 -2
index cb1b872..f69ab7b 100644
@@ -2130,7 +2130,7 @@ int iw_handler_set_spy(struct net_device
 	 * The rtnl_lock() make sure we don't race with the other iw_handlers.
 	 * This make sure wireless_spy_update() "see" that the spy list
 	 * is temporarily disabled. */
-	wmb();
+	smp_wmb();
 
 	/* Are there are addresses to copy? */
 	if(wrqu->data.length > 0) {

@@ -2159,7 +2159,7 @@ #endif	/* WE_SPY_DEBUG */
 	}
 
 	/* Make sure above is updated before re-enabling */
-	wmb();
+	smp_wmb();
 
 	/* Enable addresses */
 	spydata->spy_number = wrqu->data.length;
net/ipv4/inet_timewait_sock.c +1 -1
index cdd8053..1a852c2 100644
@@ -178,7 +178,7 @@ void inet_twdr_hangman(unsigned long dat
 	need_timer = 0;
 	if (inet_twdr_do_twkill_work(twdr, twdr->slot)) {
 		twdr->thread_slots |= (1 << twdr->slot);
-		mb();
+		smp_mb();
 		schedule_work(&twdr->twkill_work);
 		need_timer = 1;
 	} else {
net/ipv4/tcp_input.c +2 -2
index 9304034..c701f6a 100644
@@ -4235,7 +4235,7 @@ static int tcp_rcv_synsent_state_process
 		 * Change state from SYN-SENT only after copied_seq
 		 * is initialized. */
 		tp->copied_seq = tp->rcv_nxt;
-		mb();
+		smp_mb();
 		tcp_set_state(sk, TCP_ESTABLISHED);
 
 		security_inet_conn_established(sk, skb);

@@ -4483,7 +4483,7 @@ int tcp_rcv_state_process(struct sock *s
 		case TCP_SYN_RECV:
 			if (acceptable) {
 				tp->copied_seq = tp->rcv_nxt;
-				mb();
+				smp_mb();
 				tcp_set_state(sk, TCP_ESTABLISHED);
 				sk->sk_state_change(sk);
net/packet/af_packet.c +1 -1
index 08e68b6..da73e8a 100644
@@ -660,7 +660,7 @@ static int tpacket_rcv(struct sk_buff *s
 	sll->sll_ifindex = dev->ifindex;
 
 	h->tp_status = status;
-	mb();
+	smp_mb();
 
 	{
 		struct page *p_start, *p_end;
maintainer
#1 lore raw

From: Ralf Baechle <[email protected]>
Date: Wed, 6 Dec 2006 21:49:46 +0000

Quoted 10 lines

I believe all the below memory barriers only matter on SMP so therefore the smp_* variant of the barrier should be used.

I'm wondering if the barrier in net/ipv4/inet_timewait_sock.c should be dropped entirely. schedule_work's implementation currently implies a memory barrier and I think sane semantics of schedule_work() should imply a memory barrier, as needed so the caller shouldn't have to worry. It's not quite obvious why the barrier in net/packet/af_packet.c is needed; maybe it should be implied through flush_dcache_page?

I'll check out that timewait sock case later, but AF_PACKET mmap() is totally broken on D-cache aliasing architectures. Many years ago I tried to fix this up with Alexey but those talks went nowhere :)

maintainer
#2 lore raw
In reply to Ralf Baechle

From: Ralf Baechle <[email protected]>
Date: Wed, 6 Dec 2006 21:49:46 +0000

Quoted 10 lines

I believe all the below memory barriers only matter on SMP so therefore the smp_* variant of the barrier should be used.

I'm wondering if the barrier in net/ipv4/inet_timewait_sock.c should be dropped entirely. schedule_work's implementation currently implies a memory barrier and I think sane semantics of schedule_work() should imply a memory barrier, as needed so the caller shouldn't have to worry. It's not quite obvious why the barrier in net/packet/af_packet.c is needed; maybe it should be implied through flush_dcache_page?

Quoted 3 lines

Ralf

Signed-off-by Ralf Baechle <[email protected]>

Ok, Ralf this looks good, and I think your suspicions are correct about the timewait case, I'll just delete that memory barrier. I put it there and aparently for now justifiable reason :-)