[PATCH v8] Cygwin: pipe: Switch pipe mode to blocking mode by default

[Takashi Yano]

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>