Message-ID: <506B5254.3060708@malth.us>
Date: Tue, 02 Oct 2012 13:45:08 -0700
From: "Gregory M. Turner" <gmt AT malth DOT us>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20120907 Thunderbird/15.0.1
MIME-Version: 1.0
To: cygwin AT cygwin DOT com
Subject: [PATCH] Re: Fifo blocking and performance issues
References: <2415374 DOT xBCzgxA7JH AT bob-kubuntu> <20121002201947 DOT GA5314 AT ednor DOT casa DOT cgf DOT cx>
In-Reply-To: <20121002201947.GA5314@ednor.casa.cgf.cx>
Content-Type: multipart/mixed; boundary="------------020101040304010500090507"
Mailing-List: contact cygwin-help AT cygwin DOT com; run by ezmlm
Sender: cygwin-owner AT cygwin DOT com
Mail-Followup-To: cygwin AT cygwin DOT com

--------------020101040304010500090507
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

On 10/2/2012 1:19 PM, Christopher Faylor wrote:
> On Tue, Oct 02, 2012 at 03:15:37PM -0400, bob wrote:
>> Any suggestions on how we can achieve a higher performance blocking read on a
>> Cygwin RDWR fifo?
>
> As always, if you can provide test cases of bugs we will endeavor to fix problems.

I didn't think the RDWR fifo's worked at all -- they certainly don't if 
you create them via bash.

I have a patch for this but there are some problems with it.

I've enclosed the work-in-progress patch, feel free to give it a try and 
see if that changes anything (you'll need to rebuild your cygwin1.dll 
from cvs, the instructions are in the cygwin FAQ).

If people have any ideas on how to improve on what I did here I'd love 
to hear from them.  My current thinking is that to really fix cygwin 
fifo's we need to divorce cygwin's fifo handles from the win32 named 
pipe handles used to implement them.

No promises that I ever actually get around to doing this -- but to 
/really/ fix cygwin named pipes, I'm leaning towards the following approach:

  o create a fhandler_pipe_base class that implements pipes using
    MailSlots instead of win32 named pipes; it should be a
    descendant of fhandler_overlapped_base so that we have enough
    freedom to implement the correct blocking semantics

  o make fhandler_pipe and fhandler_fifo into trivial superclasses
    of fhandler_pipe_base; they should only need to override
    ::open and, I think, only need to override the naming
    convention applied to the MailSlot objects.

But the above is bikeshedding based on what I learned mucking around 
with the existing implementation.  It's a decent amount of work to 
actually implement that plan.

-gmt


--------------020101040304010500090507
Content-Type: text/plain; charset=windows-1252;
 name="cygwin-1.7.pre17-duplex-fifo-support.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename="cygwin-1.7.pre17-duplex-fifo-support.patch"

Index: winsup/cygwin/fhandler.h
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/fhandler.h,v
retrieving revision 1.474
diff -u -p -r1.474 fhandler.h
--- winsup/cygwin/fhandler.h	16 Aug 2012 23:34:43 -0000	1.474
+++ winsup/cygwin/fhandler.h	1 Oct 2012 02:31:07 -0000
@@ -731,10 +731,18 @@ class fhandler_fifo: public fhandler_bas
 {
   HANDLE read_ready;
   HANDLE write_ready;
+  HANDLE duplex_write_handle;
   bool wait (HANDLE) __attribute__ ((regparm (2)));
   char *fifo_name (char *, const char *) __attribute__ ((regparm (2)));
 public:
   fhandler_fifo ();
+  bool is_duplex() { return (duplex_write_handle != INVALID_HANDLE_VALUE); }
+  HANDLE& get_output_handle ()
+    {
+      return is_duplex() ? 
+               duplex_write_handle :
+               fhandler_base_overlapped::get_output_handle ();
+    }
   int open (int, mode_t);
   int close ();
   int dup (fhandler_base *child, int);
Index: winsup/cygwin/fhandler_fifo.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/fhandler_fifo.cc,v
retrieving revision 1.55
diff -u -p -r1.55 fhandler_fifo.cc
--- winsup/cygwin/fhandler_fifo.cc	17 Jun 2012 20:50:24 -0000	1.55
+++ winsup/cygwin/fhandler_fifo.cc	1 Oct 2012 02:31:07 -0000
@@ -26,7 +26,8 @@
 
 fhandler_fifo::fhandler_fifo ():
   fhandler_base_overlapped (),
-  read_ready (NULL), write_ready (NULL)
+  read_ready (NULL), write_ready (NULL),
+  duplex_write_handle(INVALID_HANDLE_VALUE)
 {
   max_atomic_write = DEFAULT_PIPEBUFSIZE;
   need_fork_fixup (true);
@@ -34,8 +35,6 @@ fhandler_fifo::fhandler_fifo ():
 
 #define fnevent(w) fifo_name (npbuf, w "-event")
 #define fnpipe() fifo_name (npbuf, "fifo")
-#define create_pipe(r, w) \
-  fhandler_pipe::create (sa_buf, (r), (w), 0, fnpipe (), open_mode)
 
 char *
 fhandler_fifo::fifo_name (char *buf, const char *what)
@@ -81,112 +80,137 @@ fhandler_fifo::open (int flags, mode_t)
     error_errno_set,
     error_set_errno
   } res;
-  bool reader, writer, duplexer;
   DWORD open_mode = FILE_FLAG_OVERLAPPED;
 
-  /* Determine what we're doing with this fhandler: reading, writing, both */
-  switch (flags & O_ACCMODE)
-    {
-    case O_RDONLY:
-      reader = true;
-      writer = false;
-      duplexer = false;
-      break;
-    case O_WRONLY:
-      writer = true;
-      reader = false;
-      duplexer = false;
-      break;
-    case O_RDWR:
-      open_mode |= PIPE_ACCESS_DUPLEX;
-      reader = true;
-      writer = false;
-      duplexer = true;
-      break;
-    default:
-      set_errno (EINVAL);
-      res = error_errno_set;
-      goto out;
-    }
-
-  debug_only_printf ("reader %d, writer %d, duplexer %d", reader, writer, duplexer);
   set_flags (flags);
   char char_sa_buf[1024];
   LPSECURITY_ATTRIBUTES sa_buf;
   sa_buf = sec_user_cloexec (flags & O_CLOEXEC, (PSECURITY_ATTRIBUTES) char_sa_buf,
 		      cygheap->user.sid());
   char npbuf[MAX_PATH];
+  int err;
 
   /* Create control events for this named pipe */
-  if (!(read_ready = CreateEvent (sa_buf, duplexer, false, fnevent ("r"))))
+  if (!(read_ready = CreateEvent (sa_buf, false, false, fnevent ("r"))))
     {
-      debug_printf ("CreatEvent for %s failed, %E", npbuf);
+      debug_printf ("CreateEvent for %s failed, %E", npbuf);
       res = error_set_errno;
       goto out;
     }
   if (!(write_ready = CreateEvent (sa_buf, false, false, fnevent ("w"))))
     {
-      debug_printf ("CreatEvent for %s failed, %E", npbuf);
-      res = error_set_errno;
-      goto out;
-    }
-
-  /* If we're reading, create the pipe, signal that we're ready and wait for
-     a writer.
-     FIXME: Probably need to special case O_RDWR case.  */
-  if (!reader)
-    /* We are not a reader */;
-  else if (create_pipe (&get_io_handle (), NULL))
-    {
-      debug_printf ("create of reader failed");
-      res = error_set_errno;
-      goto out;
-    }
-  else if (!arm (read_ready))
-    {
+      debug_printf ("CreateEvent for %s failed, %E", npbuf);
       res = error_set_errno;
       goto out;
     }
-  else if (!duplexer && !wait (write_ready))
-    {
-      res = error_errno_set;
-      goto out;
-    }
-
-  /* If we're writing, it's a little tricky since it is possible that
-     we're attempting to open the other end of a pipe which is already
-     connected.  In that case, we detect ERROR_PIPE_BUSY, reset the
-     read_ready event and wait for the reader to allow us to connect
-     by signalling read_ready.
 
-     Once the pipe has been set up, we signal write_ready.  */
-  if (writer)
+  /* Determine what we're doing with this fhandler: reading, writing, both */
+  switch (flags & O_ACCMODE)
     {
-      int err;
+    case O_WRONLY:
+      /* After waiting for read_ready to signal, but before
+         ::create completes the connection process, it's possible
+         for another writer to sneak in and get connected, in
+         which case, we receive ERROR_PIPE_BUSY.  If this occurs,
+         we reset the read_ready event, wait for another signal,
+         and try again.
+
+         Once the pipe has been connected, we signal write_ready,
+         in order to wake up the reader, who will block pending
+         this notification. */
       while (1)
-	if (!wait (read_ready))
-	  {
-	    res = error_errno_set;
-	    goto out;
-	  }
-	else if ((err = create_pipe (NULL, &get_io_handle ())) == 0)
-	  break;
-	else if (err == ERROR_PIPE_BUSY)
-	  {
-	    debug_only_printf ("pipe busy");
-	    ResetEvent (read_ready);
-	  }
-	else
-	  {
-	    debug_printf ("create of writer failed");
-	    res = error_set_errno;
-	    goto out;
-	  }
+        if (!wait (read_ready))
+          {
+            res = error_errno_set;
+            debug_printf("error waiting for read_ready: %d", res);
+            goto out;
+          }
+        else if ((err = fhandler_pipe::create (sa_buf, NULL, &get_io_handle(),
+                                               0, fnpipe (), open_mode)) == 0)
+          break; /* from the while(1) loop */
+        else if (err == ERROR_PIPE_BUSY)
+          debug_only_printf ("write pipe-end busy after signal; retrying");
+        else
+          {
+            res = error_set_errno;
+            debug_printf ("create of writer failed: error %d", res);
+            goto out;
+          }
+
       if (!arm (write_ready))
-	{
-	  res = error_set_errno;
-	  goto out;
-	}
+        {
+          res = error_set_errno;
+          debug_printf("!arm (write_ready): error %d", res);
+          goto out;
+        }
+      else
+        break;
+    case O_RDONLY:
+      /* If we're reading, create the pipe, signal that we're ready and wait for
+         a writer. */
+      if (fhandler_pipe::create (sa_buf, &get_io_handle(), NULL,
+                                      0, fnpipe (), open_mode))
+        {
+          res = error_set_errno;
+          debug_printf ("create of reader pipe failed: error %d", res);
+          goto out;
+        }
+      else if (!arm (read_ready))
+        {
+          res = error_set_errno;
+          debug_printf("!arm (read_ready): error %d", res);
+          goto out;
+        }
+      else if (!wait (write_ready))
+        {
+          res = error_errno_set;
+          debug_printf("!wait (write_ready): error %d", res);
+          goto out;
+        }
+      else
+        break;
+    case O_RDWR:
+      /* FIXME: We have a serious problem if we get ERROR_PIPE_BUSY here, because we
+         aren't supposed to block.  POSIX is silent about this, but blocking really
+         seems bad (is some handsome virtual fireman coming to rescue us?)
+
+         This is not academic: we can create just one duplex instance before all
+         subsequent attempts will fail this way or block forever.
+
+         I'm afraid we may need to gut-rehab much of this class to get things to
+         really work right, even though they come tantalizingly close as-is. */
+      if ((err = fhandler_pipe::create (sa_buf, &get_io_handle(), &duplex_write_handle,
+                                        0, fnpipe (), open_mode )) != 0)
+        {
+          res = error_set_errno;
+          debug_printf("pipe create failure during duplex fifo open: error %d", res);
+          goto out;
+        }
+      else
+        /* JUSTIFICATION
+
+           Let's dry-run the 2x-half-duplex reader/writer scenario: someone forks,
+           opens a read-end, and then opens a write-end after a significant delay.
+           To wit:
+
+           o The reader creates its win32 pipe handle, triggers read_ready, and blocks
+             pending write_ready.
+
+           o Along comes the writer, who waits for read-ready.  However, read-ready
+             is already triggered, so now it automatically untriggers, and the writer
+             thread immediately continues (the reader remains blocked).
+
+           o The writer triggers write_ready, which the reader is already waiting
+             for; write_ready also becomes untriggered and both threads are awake.
+
+           That's it -- they both call setup_overlapped() and immediately return. 
+           As of now, we approximate this state of affairs perfectly; there's nothing
+           more for us to do (except fix the bugs in the above approach :P). */
+        break;
+    default:
+      set_errno (EINVAL);
+      res = error_errno_set;
+      goto out;
     }
 
   /* If setup_overlapped() succeeds (and why wouldn't it?) we are all set. */
@@ -215,6 +239,8 @@ out:
 	}
       if (get_io_handle ())
 	CloseHandle (get_io_handle ());
+      if (is_duplex())
+        CloseHandle (duplex_write_handle);
     }
   debug_printf ("res %d", res);
   return res == success;
@@ -322,6 +348,8 @@ fhandler_fifo::close ()
 {
   CloseHandle (read_ready);
   CloseHandle (write_ready);
+  if (is_duplex())
+    CloseHandle (duplex_write_handle);
   return fhandler_base::close ();
 }
 
@@ -351,6 +379,17 @@ fhandler_fifo::dup (fhandler_base *child
       __seterrno ();
       return -1;
     }
+  if (is_duplex() &&
+      !DuplicateHandle (GetCurrentProcess(), duplex_write_handle,
+                        GetCurrentProcess(), &fhf->duplex_write_handle,
+                        0, true, DUPLICATE_SAME_ACCESS))
+    {
+      CloseHandle (fhf->read_ready);
+      CloseHandle (fhf->write_ready);
+      fhf->close();
+      __seterrno();
+      return -1;
+    }
   return 0;
 }
 
@@ -360,6 +399,8 @@ fhandler_fifo::fixup_after_fork (HANDLE 
   fhandler_base_overlapped::fixup_after_fork (parent);
   fork_fixup (parent, read_ready, "read_ready");
   fork_fixup (parent, write_ready, "write_ready");
+  if (is_duplex())
+    fork_fixup (parent, duplex_write_handle, "duplex_write_handle");
 }
 
 void
@@ -368,4 +409,6 @@ fhandler_fifo::set_close_on_exec (bool v
   fhandler_base::set_close_on_exec (val);
   set_no_inheritance (read_ready, val);
   set_no_inheritance (write_ready, val);
+  if (is_duplex())
+    set_no_inheritance (duplex_write_handle, val);
 }


--------------020101040304010500090507
Content-Type: text/plain; charset=us-ascii

--
Problem reports:       http://cygwin.com/problems.html
FAQ:                   http://cygwin.com/faq/
Documentation:         http://cygwin.com/docs.html
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
--------------020101040304010500090507--