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

[Corinna Vinschen]

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