tst-cancel4: Make blocking on write more portable

Message ID 20200705181154.1393332-1-samuel.thibault@ens-lyon.org
State Superseded, archived
Headers
Series tst-cancel4: Make blocking on write more portable |

Commit Message

Samuel Thibault July 5, 2020, 6:11 p.m. UTC
  We should not be assuming how PF_UNIX buffering works and just choose a
"proper" write size. The additional pthread_testcancel() call after
write() shows it: we don't really control how buffering works.

We can however easily use select to fill the buffer byte by byte until
select() says that write() will block, in which case write() will not
even be able to perform a partial write.

We should however do this on the same side of the socketpair as the
read() tests, otherwise read() would get data from this. This is
actually fine: we can just read() and write() on the same side, with
nobody consuming or producing data on the other side.

* nptl/tst-cancel4-common.h (set_socket_buffer): Write data on the
socket until select informs us that it would block.
* nptl/tst-cancel4-common.c (do_test): Set send buffer size on fds[0]
instead of fds[1].
* nptl/tst-cancel4.c (tf_write, tf_writev): Use fds[0] instead of
fds[1].
(tf_write): Do not call pthread_testcancel() after write(), we are now not
even supposed to get partial writes.
(tf_send, tf_sendto): Fill socket buffer after connecting.
---
 nptl/tst-cancel4-common.c |  2 +-
 nptl/tst-cancel4-common.h | 19 ++++++++++++++++++-
 nptl/tst-cancel4.c        | 16 ++++++----------
 3 files changed, 25 insertions(+), 12 deletions(-)
  

Comments

Adhemerval Zanella July 7, 2020, 9:08 p.m. UTC | #1
On 05/07/2020 15:11, Samuel Thibault wrote:
> diff --git a/nptl/tst-cancel4-common.h b/nptl/tst-cancel4-common.h
> index c8763cacba..f99785c4f2 100644
> --- a/nptl/tst-cancel4-common.h
> +++ b/nptl/tst-cancel4-common.h
> @@ -62,7 +62,7 @@ static pthread_barrier_t b2;
>  
>  #define WRITE_BUFFER_SIZE 16384
>  
> -/* Set the send buffer of socket S to 1 byte so any send operation
> +/* Set the send buffer of socket S to 1 byte and fill it so any send operation
>     done with WRITE_BUFFER_SIZE bytes will force syscall blocking.  */
>  static void
>  set_socket_buffer (int s)
> @@ -74,6 +74,23 @@ set_socket_buffer (int s)
>  		    sizeof (val)) == 0);
>    TEST_VERIFY_EXIT (getsockopt (s, SOL_SOCKET, SO_SNDBUF, &val, &len) == 0);
>    TEST_VERIFY_EXIT (val < WRITE_BUFFER_SIZE);
> +
> +  char buf = '\0';
> +  struct timeval timeout = { .tv_sec = 0, .tv_usec = 0 };
> +
> +  while (1)
> +    {
> +      fd_set set;
> +      int ret;
> +
> +      FD_ZERO (&set);
> +      FD_SET (s, &set);
> +      ret = select (s + 1, NULL, &set, NULL, &timeout);
> +      if (!ret)
> +	break;
> +
> +      write (s, &buf, 1);
> +    }
>  }

Unfortunately this does not do what you are intending to do on Linux and 
the tests only pass because of BZ#12683 where cancellation does not act on 
write with side-effects.

For instance on kernel 5.4.0-40-generic the syscall sequence is:

[...]
select(4, NULL, [3], NULL, {tv_sec=0, tv_usec=0}) = 1 (out [3], left {tv_sec=0, tv_usec=0})
write(3, "\0", 1)                       = 1
select(4, NULL, [3], NULL, {tv_sec=0, tv_usec=0}) = 1 (out [3], left {tv_sec=0, tv_usec=0})
write(3, "\0", 1)                       = 1
select(4, NULL, [3], NULL, {tv_sec=0, tv_usec=0}) = 0 (Timeout)
[...]
clone(child_stack=0x7fd3ee4f4fb0, flags=CLONE_VM|CLONE_FS|CLONE_FILES|CLONE_SIGHAND|CLONE_THREAD|CLONE_SYSVSEM|CLONE_SETTLS|CLONE_PARENT_SETTID|CLONE_CHILD_CLEARTIDstrace: Process 2641554 attached
, parent_tid=[2641554], tls=0x7fd3ee4f5700, child_tidptr=0x7fd3ee4f59d0) = 2641554
[...]
[pid 2641554] write(3, "\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 16384 <unfinished ...>
[...]
[pid 2641553] tgkill(2641553, 2641554, SIGRTMIN) = 0
[pid 2641554] write(1, "write (3, buf, 16384) returned 2"..., 40write (3, buf, 16384) returned 2240 (0)
[...]

So it only fills 2 bytes on the socket buffer, but later the write *does*
return with side-effects indicating that it has at least written 2240 bytes.

I haven't investigate further on Linux kernel the iteration between
SOL_SOCKET, buffer filling and blocking operations.  But it does show that
is indeed system specific and partial write might still occur even with
clever tricks.  I am also not sure if this is something specific to 
socketpair and if using a named socket might have different results.  

Are you trying to make the tests behave better on Hurd? If so, in how
exactly Hurd behaves different than Linux in this case?
  

Patch

diff --git a/nptl/tst-cancel4-common.c b/nptl/tst-cancel4-common.c
index 9a6924c1c6..3d4b567d38 100644
--- a/nptl/tst-cancel4-common.c
+++ b/nptl/tst-cancel4-common.c
@@ -26,7 +26,7 @@  do_test (void)
       exit (1);
     }
 
-  set_socket_buffer (fds[1]);
+  set_socket_buffer (fds[0]);
 
   if (mktemp (fifoname) == NULL)
     {
diff --git a/nptl/tst-cancel4-common.h b/nptl/tst-cancel4-common.h
index c8763cacba..f99785c4f2 100644
--- a/nptl/tst-cancel4-common.h
+++ b/nptl/tst-cancel4-common.h
@@ -62,7 +62,7 @@  static pthread_barrier_t b2;
 
 #define WRITE_BUFFER_SIZE 16384
 
-/* Set the send buffer of socket S to 1 byte so any send operation
+/* Set the send buffer of socket S to 1 byte and fill it so any send operation
    done with WRITE_BUFFER_SIZE bytes will force syscall blocking.  */
 static void
 set_socket_buffer (int s)
@@ -74,6 +74,23 @@  set_socket_buffer (int s)
 		    sizeof (val)) == 0);
   TEST_VERIFY_EXIT (getsockopt (s, SOL_SOCKET, SO_SNDBUF, &val, &len) == 0);
   TEST_VERIFY_EXIT (val < WRITE_BUFFER_SIZE);
+
+  char buf = '\0';
+  struct timeval timeout = { .tv_sec = 0, .tv_usec = 0 };
+
+  while (1)
+    {
+      fd_set set;
+      int ret;
+
+      FD_ZERO (&set);
+      FD_SET (s, &set);
+      ret = select (s + 1, NULL, &set, NULL, &timeout);
+      if (!ret)
+	break;
+
+      write (s, &buf, 1);
+    }
 }
 
 /* Cleanup handling test.  */
diff --git a/nptl/tst-cancel4.c b/nptl/tst-cancel4.c
index 5250a30b2e..7c72d16d4c 100644
--- a/nptl/tst-cancel4.c
+++ b/nptl/tst-cancel4.c
@@ -147,7 +147,7 @@  tf_write  (void *arg)
   int fd;
 
   if (arg == NULL)
-    fd = fds[1];
+    fd = fds[0];
   else
     {
       char fname[] = "/tmp/tst-cancel4-fd-XXXXXX";
@@ -167,10 +167,6 @@  tf_write  (void *arg)
   char buf[WRITE_BUFFER_SIZE];
   memset (buf, '\0', sizeof (buf));
   s = write (fd, buf, sizeof (buf));
-  /* The write can return a value higher than 0 (meaning partial write)
-     due to the SIGCANCEL, but the thread may still be pending
-     cancellation.  */
-  pthread_testcancel ();
 
   pthread_cleanup_pop (0);
 
@@ -184,7 +180,7 @@  tf_writev  (void *arg)
   int fd;
 
   if (arg == NULL)
-    fd = fds[1];
+    fd = fds[0];
   else
     {
       char fname[] = "/tmp/tst-cancel4-fd-XXXXXX";
@@ -753,13 +749,13 @@  tf_send (void *arg)
   if (tempfd2 == -1)
     FAIL_EXIT1 ("socket (AF_UNIX, SOCK_STREAM, 0): %m");
 
-  set_socket_buffer (tempfd2);
-
   if (connect (tempfd2, (struct sockaddr *) &sun, sizeof (sun)) != 0)
     FAIL_EXIT1 ("connect: %m");
 
   unlink (sun.sun_path);
 
+  set_socket_buffer (tempfd2);
+
   xpthread_barrier_wait (&b2);
 
   if (arg != NULL)
@@ -1288,13 +1284,13 @@  tf_sendto (void *arg)
   if (tempfd2 == -1)
     FAIL_EXIT1 ("socket (AF_UNIX, SOCK_STREAM, 0): %m");
 
-  set_socket_buffer (tempfd2);
-
   if (connect (tempfd2, (struct sockaddr *) &sun, sizeof (sun)) != 0)
     FAIL_EXIT1 ("connect: %m");
 
   unlink (sun.sun_path);
 
+  set_socket_buffer (tempfd2);
+
   xpthread_barrier_wait (&b2);
 
   if (arg != NULL)