[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] mini-os: xenbus: support large messages

On 15.09.21 01:17, Samuel Thibault wrote:

Thanks for having worked on this!

Juergen Gross, le mer. 18 août 2021 17:26:10 +0200, a ecrit:
+static void xenbus_read_data(char *buf, unsigned int len)
+    unsigned int off = 0;
+    unsigned int prod;
+    unsigned int size;
+    int notify;
+    while (off != len)
+    {
+        if (xenstore_buf->rsp_prod == xenstore_buf->rsp_cons)
+            wait_event(xb_waitq,
+                       xenstore_buf->rsp_prod != xenstore_buf->rsp_cons);

The if is redundant since wait_event already tests it.

Ah, right.

+        prod = xenstore_buf->rsp_prod;
+        DEBUG("Rsp_cons %d, rsp_prod %d.\n", xenstore_buf->rsp_cons, prod);
+        size = min(len - off, prod - xenstore_buf->rsp_cons);
+        memcpy_from_ring(xenstore_buf->rsp, buf + off,
+                         MASK_XENSTORE_IDX(xenstore_buf->rsp_cons), size);
+        off += size;
+        notify = (xenstore_buf->rsp_cons + XENSTORE_RING_SIZE ==
+                  xenstore_buf->rsp_prod);

This looks odd to me?  We want to notify as soon as the ring is empty,
which can happen at any place in the ring right?

No, we want to notify if the ring was full and is about to gain some
space again, as the other side was probably not able to put all data
in and is now waiting for more space to become available.

Linux' code uses (intf->rsp_prod - cons >= XENSTORE_RING_SIZE), *after*
the rsp_cons increase.

+        rmb();

rmb() must be placed before memcpy_from_ring, to make sure that the data
we read from the buffer is up-to-date according to the read we made from

Ah, yes. Thanks for spotting that one!

The rest seems ok to me.



Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.