[PATCH v8] Cygwin: pipe: Switch pipe mode to blocking mode by default
Corinna Vinschen
corinna-cygwin@cygwin.com
Wed Oct 23 09:00:33 GMT 2024
Hi Takashi,
first of all, this is quite a piece of work, thanks for pulling
this through!
Just a few points:
On Sep 22 06:15, Takashi Yano wrote:
> @@ -370,35 +415,15 @@ fhandler_pipe::raw_read (void *ptr, size_t& len)
> break;
> case STATUS_PIPE_LISTENING:
> case STATUS_PIPE_EMPTY:
> + /* Only for real_non_blocking_mode */
> + if (!is_nonblocking ())
> + /* Should not reach here */
> + continue;
Can you explain why this is necessary at all?
> @@ -439,24 +452,100 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t len)
> if (!len)
> return 0;
This new implementation of raw_write() skips the mechanism added in
commit 170e6badb621 ("Cygwin: pipe: improve writing when pipe buffer is
almost full") for non-blocking pipes, if the pipe has less space than
is requested by user-space.
Rather than trying to write multiple of 4K chunks or smaller multiple of
2 chunks if < 4K, it just writes as much as possible in one go, i.e.
Before:
$ ./x 40000
pipe capacity: 65536
write: writable 1, 40000 25536
write: writable 1, 24576 960
write: writable 0, 512 448
write: writable 0, 256 192
write: writable 0, 128 64
write: writable 0, 64 0
write: writable 0, -1 / Resource temporarily unavailable
After:
$ ./x 40000
pipe capacity: 65536
write: writable 1, 40000 25536
write: writable 1, 25536 0
write: writable 0, -1 / Resource temporarily unavailable
This way, we get into the EAGAIN case much faster again, which was
one reason for 170e6badb621.
Does this make more sense, and if so, why? If this is really the
way to go, the comment starting at line 634 (after applying your patch)
will have to be changed as well.
> + /* Pipe should be empty because reader is waiting the data. */
^^^
for
> @@ -925,7 +952,7 @@ fhandler_pipe::create (fhandler_pipe *fhs[2], unsigned psize, int mode)
> HANDLE r, w;
> SECURITY_ATTRIBUTES *sa = sec_none_cloexec (mode);
> int res = -1;
> - int64_t unique_id;
> + int64_t unique_id = 0;
unique_id will be set by the following nt_create() anyway.
Is there a case where it's not set? I don't see this.
Other than that, this looks great!
Thanks,
Corinna
More information about the Cygwin-patches
mailing list