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

Re: [PATCH v1.1 5/6] tools/libxs: Use writev()/sendmsg() instead of write()


  • To: Juergen Gross <jgross@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • From: Jason Andryuk <jason.andryuk@xxxxxxx>
  • Date: Tue, 23 Jul 2024 09:45:15 -0400
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass (sender ip is 165.204.84.17) smtp.rcpttodomain=suse.com smtp.mailfrom=amd.com; dmarc=pass (p=quarantine sp=quarantine pct=100) action=none header.from=amd.com; dkim=none (message not signed); arc=none (0)
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector10001; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=vrOKkQBU4TDjr90t1Z4yRMA7wSjtwgBuAkIgZmtVEdA=; b=ieLV5LWiBsVPJK+jJfYTCYmdubOAWW3nOA2odrM1V54YckP3NHhkiAXvXlM9KmevZezyQ9CPtFZ+2RcJTaNUv51SpZagUkyJJpZMaMTHtXDYsEgquqzER19nRRTtz8PO6+OeeOgir/6i7+zpRM68Yoc9szmeWI4T5L+HOW/sIY31lfqu5D38/AvynABQFsqg5ThKDKUD79uYUhFt52yj22FD067R2hb1Ohg7e78e08g8+yeN8swUDnQEjh9Wmyb/j6OMIZbx4ok1euq30pxA0SMO2ir4AnDP0nk+A8F/m29oE+wxRkPQhgcojWnkeF1/ex2i5u77L2lQEdzTRZcJFg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector10001; d=microsoft.com; cv=none; b=h3Eu5lzcFX9FLBi/PPf/lCxTKiTo+McefbyeMJ88f4mW5kuuo1tgzhjkofgPvkOZUNqA8/exi5tzZoN+ofIOj+LiC3J/DJ9I7+q77m0q1s9SgIeQxAH0Xhc86wE2YarVNLaXCttDD5mkPmTl8u6bnHQujfFiXDt/87oKw2rsB1f/PKnvbSLGr6e+PF9FqyQNMMAaBb/0SCsHrND3XRUKbBnafz2zotOYtld841gGJhiVw+gnT0TaubCtoImHkFO4Tgtdn1ejugeKWyOT/AxjrLHXi8R1IYcES77xQeXOugvl3PzTt0uEf4fk0gH7TIe60Qv1wvy+qM18tFEIBswMyQ==
  • Cc: Anthony PERARD <anthony.perard@xxxxxxxxxx>, Frediano Ziglio <frediano.ziglio@xxxxxxxxx>
  • Delivery-date: Tue, 23 Jul 2024 14:08:34 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 2024-07-23 08:30, Juergen Gross wrote:
On 23.07.24 14:25, Andrew Cooper wrote:
On 23/07/2024 10:37 am, Jürgen Groß wrote:
On 22.07.24 18:25, Andrew Cooper wrote:
With the input data now conveniently arranged, use writev()/sendmsg()
instead
of decomposing it into write() calls.

This causes all requests to be submitted with a single system call,
rather
than at least two.  While in principle short writes can occur, the
chances of
it happening are slim given that most xenbus comms are only a handful of
bytes.

Nevertheless, provide {writev,sendmsg}_exact() wrappers which take
care of
resubmitting on EINTR or short write.

Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
CC: Juergen Gross <jgross@xxxxxxxx>
CC: Frediano Ziglio <frediano.ziglio@xxxxxxxxx>

v1.1:
   * Fix iov overread, spotted by Frediano.  Factor the common
updating logic
     out into update_iov().
---
   tools/libs/store/xs.c | 94 +++++++++++++++++++++++++++++++++++++++++--
   1 file changed, 91 insertions(+), 3 deletions(-)

diff --git a/tools/libs/store/xs.c b/tools/libs/store/xs.c
index e820cccc2314..f80ac7558cbe 100644
--- a/tools/libs/store/xs.c
+++ b/tools/libs/store/xs.c
@@ -563,6 +563,95 @@ static void *read_reply(
       return body;
   }
   +/*
+ * Update an iov/nr pair after an incomplete writev()/sendmsg().
+ *
+ * Awkwardly, nr has different widths and signs between writev() and
+ * sendmsg(), so we take it and return it by value, rather than by
pointer.
+ */
+static size_t update_iov(struct iovec **p_iov, size_t nr, size_t res)
+{
+    struct iovec *iov = *p_iov;
+
+        /* Skip fully complete elements, including empty elements. */
+        while (nr && res >= iov->iov_len) {
+                res -= iov->iov_len;
+                nr--;
+                iov++;
+        }
+
+        /* Partial element, adjust base/len. */
+        if (res) {
+                iov->iov_len  -= res;
+                iov->iov_base += res;
+        }
+
+        *p_iov = iov;
+
+    return nr;
+}
+
+/*
+ * Wrapper around sendmsg() to resubmit on EINTR or short write.
Returns
+ * @true if all data was transmitted, or @false with errno for an
error.
+ * Note: May alter @iov in place on resubmit.
+ */
+static bool sendmsg_exact(int fd, struct iovec *iov, unsigned int nr)
+{
+    struct msghdr hdr = {
+        .msg_iov = iov,
+        .msg_iovlen = nr,
+    };
+
+    /* Sanity check first element isn't empty */
+    assert(iov->iov_len == sizeof(struct xsd_sockmsg));

Can you please move this assert() into write_request(), avoiding to have
2 copies of it?

It was more relevant before update_iov() was split out.

But, there's exactly the same assertion in the write_request()'s caller,
so I'd prefer to simply drop it if that's ok?

The writev()/sendmsg() won't malfunction if the first element is 0, and
update_iov() will now cope too, so I don't think it's necessary.

Fine with me.

Reviewed-by: Jason Andryuk <jason.andryuk@xxxxxxx>

Looks like xs_write_all() is now unused internally, but it's an exposed library function. I guess it can just be kept instead of bumping the library version.

Regards,
Jason



 


Rackspace

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