Restore, comment, correct memory barriers in xenstored.
PATCH 1/1 Restore, comment, correct memory barriers in xenstored.
Keir moved barriers,
Competence questions are raised:
Correctness withers.
Signed-off-by Rusty Russell <[email protected]>
linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_comms.c +19 -2
@@ -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); }
Signature · 1 line
A bad analogy is like a leaky screwdriver -- Richard Braakman
Quoted 4 lines · Rusty Russell
Keir moved barriers,
Competence questions are raised:
Correctness withers.
[...]
linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_comms.c line 154
Quoted 11 lines
linux-2.6-xen-sparse/drivers/xen/xenbus/xenbus_comms.c +7
-0+/* 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.
Signature · 1 line
David Hopwood <[email protected]>