| Message ID | 20240718164842.3650702-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | tools/libxs: Fix SIGPIPE handling issues | expand |
On 18.07.2024 18:48, Andrew Cooper wrote: > While the purpose of this series is patch 6, it has a side effect of reducing > the number of system calls substantally. > > Using a strace of test-xenstore as an example, we go from this: > > rt_sigaction(SIGPIPE, {sa_handler=SIG_IGN, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, 8) = 0 > write(3, "\v\0\0\0\0\0\0\0\0\0\0\0\30\0\0\0", 16) = 16 > write(3, "xenstore-test/502673/a\0", 23) = 23 > write(3, "a", 1) = 1 > read(3, "\v\0\0\0\0\0\0\0\0\0\0\0\3\0\0\0", 16) = 16 > read(3, "OK\0", 3) = 3 > rt_sigaction(SIGPIPE, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, NULL, 8) = 0 > > down to this: > > sendmsg(3, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\v\0\0\0\0\0\0\0\0\0\0\0\30\0\0\0", iov_len=16}, {iov_base="xenstore-test/504021/a\0", iov_len=23}, {iov_base="a", iov_len=1}], msg_iovlen=3, msg_controllen=0, msg_flags=0}, MSG_NOSIGNAL) = 40 > read(3, "\v\0\0\0\0\0\0\0\0\0\0\0\3\0\0\0", 16) = 16 > read(3, "OK\0", 3) = 3 > > > I.e., it removes 2x rt_sigaction(), and turns all write()'s into a single > writev() or sendmsg(). > > > Reads are a little more problematic to deal with. Xenstored will produce a > full package basically in one go, but libxenstore's reading is horrbly > complicated by virtue of it being completely different depending on whether > xs_watch() has created a secondary read thread or not. > > Andrew Cooper (6): > tools/libxs: Fix length check in xs_talkv() > tools/libxs: Rework xs_talkv() to take xsd_sockmsg within the iovec > tools/libxs: Rationalise the definition of struct xs_handle > tools/libxs: Track whether we're using a socket or file > tools/libxs: Use writev()/sendmsg() instead of write() > tools/libxs: Stop playing with SIGPIPE The title of the entire series containing "Fix" vs there being no single Fixes: tag throughout, afaics, leave unclear to me whether any or all of this work wants/needs backporting. Please clarify. Jan
On 24/07/2024 7:58 am, Jan Beulich wrote: > On 18.07.2024 18:48, Andrew Cooper wrote: >> While the purpose of this series is patch 6, it has a side effect of reducing >> the number of system calls substantally. >> >> Using a strace of test-xenstore as an example, we go from this: >> >> rt_sigaction(SIGPIPE, {sa_handler=SIG_IGN, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, 8) = 0 >> write(3, "\v\0\0\0\0\0\0\0\0\0\0\0\30\0\0\0", 16) = 16 >> write(3, "xenstore-test/502673/a\0", 23) = 23 >> write(3, "a", 1) = 1 >> read(3, "\v\0\0\0\0\0\0\0\0\0\0\0\3\0\0\0", 16) = 16 >> read(3, "OK\0", 3) = 3 >> rt_sigaction(SIGPIPE, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, NULL, 8) = 0 >> >> down to this: >> >> sendmsg(3, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\v\0\0\0\0\0\0\0\0\0\0\0\30\0\0\0", iov_len=16}, {iov_base="xenstore-test/504021/a\0", iov_len=23}, {iov_base="a", iov_len=1}], msg_iovlen=3, msg_controllen=0, msg_flags=0}, MSG_NOSIGNAL) = 40 >> read(3, "\v\0\0\0\0\0\0\0\0\0\0\0\3\0\0\0", 16) = 16 >> read(3, "OK\0", 3) = 3 >> >> >> I.e., it removes 2x rt_sigaction(), and turns all write()'s into a single >> writev() or sendmsg(). >> >> >> Reads are a little more problematic to deal with. Xenstored will produce a >> full package basically in one go, but libxenstore's reading is horrbly >> complicated by virtue of it being completely different depending on whether >> xs_watch() has created a secondary read thread or not. >> >> Andrew Cooper (6): >> tools/libxs: Fix length check in xs_talkv() >> tools/libxs: Rework xs_talkv() to take xsd_sockmsg within the iovec >> tools/libxs: Rationalise the definition of struct xs_handle >> tools/libxs: Track whether we're using a socket or file >> tools/libxs: Use writev()/sendmsg() instead of write() >> tools/libxs: Stop playing with SIGPIPE > The title of the entire series containing "Fix" vs there being no single > Fixes: tag throughout, afaics, leave unclear to me whether any or all of > this work wants/needs backporting. Please clarify. I was on the fence about whether I should have used Fixes: (2005 commit) in patch 6 or not. Also I've had a spectacularly bad run of luck with the CLOEXEC fixes so leaving them to bake for a little longer in staging is no bad thing. That said, I am likely to backport them into the patchqueue imminently. Personally, I think it is very buggy for libxenstore to play with SIGPIPE, but it also came from reading the strace of the CLOEXEC fixes and thinking "well that's clearly broken too" rather than from a real SIGPIPE problem. ~Andrew P.S. Then again, the Xen libraries all get away with murder doing things that libraries must never do... I think it's still unforgivable that libxentoollog adjusts stdout/stderr if you pass NULL into xc_open_handle(), and that we have 5 different trivial few-kb libraries which are co-dependent on each other when it should be a single one.
While the purpose of this series is patch 6, it has a side effect of reducing the number of system calls substantally. Using a strace of test-xenstore as an example, we go from this: rt_sigaction(SIGPIPE, {sa_handler=SIG_IGN, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, 8) = 0 write(3, "\v\0\0\0\0\0\0\0\0\0\0\0\30\0\0\0", 16) = 16 write(3, "xenstore-test/502673/a\0", 23) = 23 write(3, "a", 1) = 1 read(3, "\v\0\0\0\0\0\0\0\0\0\0\0\3\0\0\0", 16) = 16 read(3, "OK\0", 3) = 3 rt_sigaction(SIGPIPE, {sa_handler=SIG_DFL, sa_mask=[], sa_flags=SA_RESTORER, sa_restorer=0x7fda8278e2f0}, NULL, 8) = 0 down to this: sendmsg(3, {msg_name=NULL, msg_namelen=0, msg_iov=[{iov_base="\v\0\0\0\0\0\0\0\0\0\0\0\30\0\0\0", iov_len=16}, {iov_base="xenstore-test/504021/a\0", iov_len=23}, {iov_base="a", iov_len=1}], msg_iovlen=3, msg_controllen=0, msg_flags=0}, MSG_NOSIGNAL) = 40 read(3, "\v\0\0\0\0\0\0\0\0\0\0\0\3\0\0\0", 16) = 16 read(3, "OK\0", 3) = 3 I.e., it removes 2x rt_sigaction(), and turns all write()'s into a single writev() or sendmsg(). Reads are a little more problematic to deal with. Xenstored will produce a full package basically in one go, but libxenstore's reading is horrbly complicated by virtue of it being completely different depending on whether xs_watch() has created a secondary read thread or not. Andrew Cooper (6): tools/libxs: Fix length check in xs_talkv() tools/libxs: Rework xs_talkv() to take xsd_sockmsg within the iovec tools/libxs: Rationalise the definition of struct xs_handle tools/libxs: Track whether we're using a socket or file tools/libxs: Use writev()/sendmsg() instead of write() tools/libxs: Stop playing with SIGPIPE tools/libs/store/xs.c | 394 +++++++++++++++++++++++++++--------------- 1 file changed, 258 insertions(+), 136 deletions(-)