[PATCH] Restore, comment, correct memory barriers in xenstored.

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Restore, comment, correct memory barriers in xenstored.

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

Signed-off-by: Rusty Russell <[hidden email]>

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
[hidden email]
http://lists.xensource.com/xen-devel
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Restore, comment, correct memory barriers in xenstored.

David Hopwood
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 <[hidden email]>


_______________________________________________
Xen-devel mailing list
[hidden email]
http://lists.xensource.com/xen-devel