On Mon, 2 Sep 2024 20:49:28 +0900 Takashi Yano wrote: > On Mon, 2 Sep 2024 13:10:31 +0200 > Corinna Vinschen wrote: > > On Aug 30 23:15, Takashi Yano wrote: > > > If a cygwin app is executed from a non-cygwin app and the cygwin > > > app exits, read pipe remains on non-blocking mode because of the > > > commit fc691d0246b9. Due to this behaviour, the non-cygwin app > > > cannot read the pipe correctly after that. With this patch, the > > > blocking mode of the read pipe is stored into was_blocking_read_pipe > > > on set_pipe_non_blocking() when the cygwin app starts and restored > > > on close(). > > > > Looks ok to me, but it would be helpful if Johannes could test this as > > well. > > > > I just wonder if the whole code could be simplified, if we set > > the pipe to non-blocking only temporarily while reading or writing, > > while the pipe is blocking all the time otherwise: > > > > - Create pipe blocking > > > > - set_pipe_non_blocking(true); > > NtReadFile() or NtWriteFile(); > > set_pipe_non_blocking(false) > > > > How costly is it to call NtSetInformationFile(FilePipeInformation) > > for each read/write? > > Good point. I'll try performance test for that idea.
I did perfomance test using dd command: dd if=/dev/zero ibs=1M count=1K obs=WB | dd ibs=RB obs=1M of=/dev/null status=none with the patch attached. [Current master] WB=1M,RB=1M: 1.6GB/s WB=1K,RB=1M: 200MB/s WB=1M,RB=1K: 245MB/s WB=1K,RB=1K: 170MB/s [With experimental patch] WB=1M,RB=1M: 1.5GB/s WB=1K,RB=1M: 110MB/s WB=1M,RB=1K: 125MB/s WB=1K,RB=1K: 95MB/s With the experimental implementation, the performance degraded quite a bit, expecially small read/write block size. -- Takashi Yano <takashi.y...@nifty.ne.jp>
From 02d4498de1805118ff2989d9e6251a39578cfe7b Mon Sep 17 00:00:00 2001 From: Takashi Yano <takashi.y...@nifty.ne.jp> Date: Mon, 2 Sep 2024 21:46:45 +0900 Subject: [PATCH] Cygwin: pipe: [Experimental] Set the default pipe mode blocking With this patch, pipe mode is set to blocking when the pipe is created for compatibility with non-cygwin apps. Instead, raw_read()/raw_write() set the pipe mode appropriate and restore it blocking mode on return. This makes the code simpler than before. Addresses: https://github.com/git-for-windows/git/issues/5115 Fixes: fc691d0246b9 ("Cygwin: pipe: Make sure to set read pipe non-blocking for cygwin apps."); Reported-by: isaacag, Johannes Schindelin <johannes.schinde...@gmx.de> Reviewed-by: Signed-off-by: --- winsup/cygwin/dtable.cc | 3 -- winsup/cygwin/fhandler/pipe.cc | 55 ++++++++----------------- winsup/cygwin/local_includes/fhandler.h | 2 - 3 files changed, 18 insertions(+), 42 deletions(-) diff --git a/winsup/cygwin/dtable.cc b/winsup/cygwin/dtable.cc index 9508f3e0b..4bfc4c46f 100644 --- a/winsup/cygwin/dtable.cc +++ b/winsup/cygwin/dtable.cc @@ -410,9 +410,6 @@ dtable::init_std_file_from_handle (int fd, HANDLE handle) { fhandler_pipe *fhp = (fhandler_pipe *) fh; fhp->set_pipe_buf_size (); - /* Set read pipe always to nonblocking */ - fhp->set_pipe_non_blocking (fhp->get_device () == FH_PIPER ? - true : fhp->is_nonblocking ()); } if (!fh->open_setup (openflags)) diff --git a/winsup/cygwin/fhandler/pipe.cc b/winsup/cygwin/fhandler/pipe.cc index c686df650..12858507c 100644 --- a/winsup/cygwin/fhandler/pipe.cc +++ b/winsup/cygwin/fhandler/pipe.cc @@ -91,10 +91,8 @@ fhandler_pipe::init (HANDLE f, DWORD a, mode_t mode, int64_t uniq_id) set_ino (uniq_id); set_unique_id (uniq_id | !!(mode & GENERIC_WRITE)); if (opened_properly) - /* Set read pipe always nonblocking to allow signal handling - even with FILE_SYNCHRONOUS_IO_NONALERT. */ - set_pipe_non_blocking (get_device () == FH_PIPER ? - true : is_nonblocking ()); + /* Set the pipe blocking mode for compatibility with non-cygwin apps. */ + set_pipe_non_blocking (false); /* Store pipe name to path_conv pc for query_hdl check */ if (get_dev () == FH_PIPEW) @@ -327,6 +325,9 @@ fhandler_pipe::raw_read (void *ptr, size_t& len) len = (size_t) -1; return; } + /* Set read pipe always nonblocking to allow signal handling + even with FILE_SYNCHRONOUS_IO_NONALERT. */ + set_pipe_non_blocking (true); while (nbytes < len) { ULONG_PTR nbytes_now = 0; @@ -410,6 +411,8 @@ fhandler_pipe::raw_read (void *ptr, size_t& len) || status == STATUS_BUFFER_OVERFLOW) break; } + /* Set the pipe blocking mode for compatibility with non-cygwin apps. */ + set_pipe_non_blocking (false); ReleaseMutex (read_mtx); len = nbytes; } @@ -459,6 +462,11 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t len) return -1; } + if (get_device () == FH_PIPEW) + { + fhandler_pipe *fh = (fhandler_pipe *) this; + fh->set_pipe_non_blocking (is_nonblocking ()); + } /* Write in chunks, accumulating a total. If there's an error, just return the accumulated total unless the first write fails, in which case return -1. */ @@ -610,6 +618,12 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t len) break; } out: + if (get_device () == FH_PIPEW) + { + /* Set the pipe blocking mode for compatibility with non-cygwin apps. */ + fhandler_pipe *fh = (fhandler_pipe *) this; + fh->set_pipe_non_blocking (false); + } CloseHandle (evt); if (status == STATUS_THREAD_SIGNALED && nbytes == 0) set_errno (EINTR); @@ -649,17 +663,6 @@ fhandler_pipe::fixup_after_fork (HANDLE parent) ReleaseMutex (hdl_cnt_mtx); } -void -fhandler_pipe::fixup_after_exec () -{ - /* Set read pipe itself always non-blocking for cygwin process. - Blocking/non-blocking is simulated in raw_read(). For write - pipe, follow is_nonblocking(). */ - bool mode = get_device () == FH_PIPEW ? is_nonblocking () : true; - set_pipe_non_blocking (mode); - fhandler_base::fixup_after_exec (); -} - int fhandler_pipe::dup (fhandler_base *child, int flags) { @@ -1174,22 +1177,6 @@ fhandler_pipe::ioctl (unsigned int cmd, void *p) return 0; } -int -fhandler_pipe::fcntl (int cmd, intptr_t arg) -{ - if (cmd != F_SETFL) - return fhandler_base::fcntl (cmd, arg); - - const bool was_nonblocking = is_nonblocking (); - int res = fhandler_base::fcntl (cmd, arg); - const bool now_nonblocking = is_nonblocking (); - /* Do not set blocking mode for read pipe to allow signal handling - even with FILE_SYNCHRONOUS_IO_NONALERT. */ - if (now_nonblocking != was_nonblocking && get_device () != FH_PIPER) - set_pipe_non_blocking (now_nonblocking); - return res; -} - int fhandler_pipe::fstat (struct stat *buf) { @@ -1362,17 +1349,11 @@ fhandler_pipe::spawn_worker (int fileno_stdin, int fileno_stdout, && (fd == fileno_stdout || fd == fileno_stderr)) { fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd; - pipe->set_pipe_non_blocking (false); /* Setup for query_ndl stuff. Read the comment below. */ if (pipe->request_close_query_hdl ()) need_send_noncygchld_sig = true; } - else if (cfd->get_dev () == FH_PIPER && fd == fileno_stdin) - { - fhandler_pipe *pipe = (fhandler_pipe *)(fhandler_base *) cfd; - pipe->set_pipe_non_blocking (false); - } /* If multiple writers including non-cygwin app exist, the non-cygwin app cannot detect pipe closure on the read side when the pipe is diff --git a/winsup/cygwin/local_includes/fhandler.h b/winsup/cygwin/local_includes/fhandler.h index 1c339cfbc..e972c6700 100644 --- a/winsup/cygwin/local_includes/fhandler.h +++ b/winsup/cygwin/local_includes/fhandler.h @@ -1234,13 +1234,11 @@ public: int open (int flags, mode_t mode = 0); bool open_setup (int flags); void fixup_after_fork (HANDLE); - void fixup_after_exec (); int dup (fhandler_base *child, int); void set_close_on_exec (bool val); int close (); void raw_read (void *ptr, size_t& len); int ioctl (unsigned int cmd, void *); - int fcntl (int cmd, intptr_t); int fstat (struct stat *buf); int fstatvfs (struct statvfs *buf); int fadvise (off_t, off_t, int); -- 2.45.1
