cygrunsrv + sshd + rsync = 20 times too slow -- throttled?

[Corinna Vinschen]

On Sep 16 22:02, Takashi Yano wrote:
> On Thu, 16 Sep 2021 18:09:05 +0900
> Takashi Yano wrote:
> > I encountered a problem with current git head.
> > 
> > To reproduce the problem:
> > 1) Make the CPU hight load state (100% load for all cores).
> > 2) Run '/bin/echo AAAAAAAAAAA 2>&1 |cat' several times.
> > Sometimes, no output is shown.
> > 
> > This seems to be a race issue between multiple pipe writers.
> > Now I am investigating the problem.
> 
> I would like to propose three additional patches, two are for
> the issue above (0001 and 0003), and the other one (0002) fixes
> the error handling.
> 
> Please have a look at patches attached.

> diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc
> index fd2ab87af..7eedc010c 100644
> --- a/winsup/cygwin/fhandler_pipe.cc
> +++ b/winsup/cygwin/fhandler_pipe.cc
> @@ -200,7 +200,14 @@ fhandler_pipe::open_setup (int flags)
>        SECURITY_ATTRIBUTES *sa = sec_none_cloexec (flags);
>        read_mtx = CreateMutex (sa, FALSE, NULL);
>        if (!read_mtx)
> -	debug_printf ("CreateMutex failed: %E");
> +	debug_printf ("CreateMutex read_mtx failed: %E");
> +    }
> +  if (!hdl_cnt_mtx)
> +    {
> +      SECURITY_ATTRIBUTES *sa = sec_none_cloexec (flags);
> +      hdl_cnt_mtx = CreateMutex (sa, FALSE, NULL);
> +      if (!hdl_cnt_mtx)
> +	debug_printf ("CreateMutex hdl_cnt_mtx failed: %E");
>      }
>    if (get_dev () == FH_PIPEW && !query_hdl)
>      set_pipe_non_blocking (is_nonblocking ());
> @@ -390,8 +397,10 @@ fhandler_pipe_fifo::reader_closed ()
>  {
>    if (!query_hdl)
>      return false;
> +  cygwait (hdl_cnt_mtx);

This is missing an `if (hdl_cnt_mtx)' check.

Also, given you're using hdl_cnt_mtx only for short-lived blocking,
you may want to reduce the overhead and just call WFSO with INFINITE
timeout.

>    int n_reader = get_obj_handle_count (query_hdl);
>    int n_writer = get_obj_handle_count (get_handle ());
> +  ReleaseMutex (hdl_cnt_mtx);
>    return n_reader == n_writer;
>  }
>  
>  int
> @@ -576,6 +594,7 @@ fhandler_pipe::dup (fhandler_base *child, int flags)
>    ftp->set_popen_pid (0);
>  
>    int res = 0;
> +  cygwait (hdl_cnt_mtx);

Same here and all the other cygwait(hdl_cnt_mtx) calls.

It would be great if open_setup() would be converted to a method which
is allowed to fail, rather than ignoring errors in sync object creation
and having to test the handle throughout the code.  Given there's only a
single caller of that function (dtable::init_std_file_from_handle), that
shouldn't be much work.  But it's certainly better than ignoring creation
failures in the long run.

> +      fhs[0]->hdl_cnt_mtx = CreateMutexW (sa, FALSE, NULL);
> +      if (!fhs[0]->hdl_cnt_mtx)
> +	{
> +	  CloseHandle (fhs[0]->read_mtx);
> +	  CloseHandle (fhs[0]->select_sem);
> +	  delete fhs[0];
> +	  NtClose (r);
> +	  CloseHandle (fhs[1]->select_sem);
> +	  CloseHandle (fhs[1]->query_hdl);
> +	  delete fhs[1];
> +	  NtClose (w);
> +	  goto out;
> +	}
> +      if (!DuplicateHandle (GetCurrentProcess (), fhs[0]->hdl_cnt_mtx,
> +			    GetCurrentProcess (), &fhs[1]->hdl_cnt_mtx,
> +			    0, sa->bInheritHandle, DUPLICATE_SAME_ACCESS))
> +	{
> +	  CloseHandle (fhs[0]->read_mtx);
> +	  CloseHandle (fhs[0]->select_sem);
> +	  CloseHandle (fhs[0]->hdl_cnt_mtx);
> +	  delete fhs[0];
> +	  NtClose (r);
> +	  CloseHandle (fhs[1]->select_sem);
> +	  CloseHandle (fhs[1]->query_hdl);
> +	  delete fhs[1];
> +	  NtClose (w);
> +	  goto out;
> +	}
> +
>        res = 0;
>      }

What about converting this to a goto error chain as in
fhandler_pty_slave::setup_pseudoconsole?  This makes error handling
much cleaner, IMHO.


Corinna