Xen devel - Oct 2005 - [PATCH] Restore, comment, correct memory barriers in xenstored.

Keir moved barriers,
	Competence questions are raised:
	Correctness withers.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff -r 067b9aacb6c2 linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_comms.c
--- a/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_comms.c	Wed Oct 12 09:11:35
2005
+++ b/linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_comms.c	Thu Oct 13 01:18:26
2005
@@ -130,7 +130,7 @@
 
 		wait_event_interruptible(xb_waitq, output_avail(out));
 
-		mb();
+		/* Make local copy of header to check for sanity. */
 		h = *out;
 		if (!check_buffer(&h))
 			return -EIO;
@@ -140,10 +140,20 @@
 			continue;
 		if (avail > len)
 			avail = len;
+
+		/* Make sure we read header before we write data
+		 * (implied by data-dependency, but let''s play safe). */
+		mb();
+
 		memcpy(dst, data, avail);
 		data += avail;
 		len -= avail;
+
+		/* Other side must not see new header until data is there. */
+		wmb();
 		update_output_chunk(out, avail);
+
+		/* This implies mb() before other side sees interrupt. */
 		notify_remote_via_evtchn(xen_start_info->store_evtchn);
 	} while (len != 0);
 
@@ -171,7 +181,6 @@
 
 		wait_event_interruptible(xb_waitq, xs_input_avail());
 
-		mb();
 		h = *in;
 		if (!check_buffer(&h))
 			return -EIO;
@@ -183,13 +192,21 @@
 			avail = len;
 		was_full = !output_avail(&h);
 
+		/* We must read header before we read data. */
+		rmb();
+
 		memcpy(data, src, avail);
 		data += avail;
 		len -= avail;
+
+		/* Other side must not see free space until we''ve copied out */
+		mb();
+
 		update_input_chunk(in, avail);
 		pr_debug("Finished read of %i bytes (%i to go)\n", avail, len);
 		/* If it was full, tell them we''ve taken some. */
 		if (was_full)
+			/* Implies mb(): they will see new header. */
 			notify_remote_via_evtchn(xen_start_info->store_evtchn);
 	}
 

-- 
A bad analogy is like a leaky screwdriver -- Richard Braakman


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Rusty Russell wrote:
> 	Keir moved barriers,
> 	Competence questions are raised:
> 	Correctness withers.
> 
[...]
> +		/* Make sure we read header before we write data
> +		 * (implied by data-dependency, but let''s play safe). */
> +		mb();
> +
>  		memcpy(dst, data, avail);
>  		data += avail;
>  		len -= avail;
> +
> +		/* Other side must not see new header until data is there. */
> +		wmb();
>  		update_output_chunk(out, avail);
Perhaps the barriers were removed on the basis that they are not needed on
x86. But if so, that reasoning is incorrect, because memcpy might use SSE
string writes which have weaker memory ordering. See the P4 arch manual,
volume 3, section 7.2.3. The code also has to work on other platforms as
well, of course.

-- 
David Hopwood <david.nospam.hopwood@blueyonder.co.uk>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel