[PATCH v8] Cygwin: pipe: Switch pipe mode to blocking mode by default
Takashi Yano
takashi.yano@nifty.ne.jp
Thu Oct 24 08:58:45 GMT 2024
On Wed, 23 Oct 2024 11:00:33 +0200
Corinna Vinschen wrote:
> first of all, this is quite a piece of work, thanks for pulling
> this through!
Thanks!
> 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?
Just in case. We do not need this. I will remove.
> > @@ -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.
Perhaps, I did not understand intent of 170e6badb621. Could you please
provide the test program (./x)? I will check my code.
>
> > + /* Pipe should be empty because reader is waiting the data. */
> ^^^
> for
Fixed.
> > @@ -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.
Without initialization, compiler complains... due to false positive?
--
Takashi Yano <takashi.yano@nifty.ne.jp>
More information about the Cygwin-patches
mailing list