From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.kundenserver.de (mout.kundenserver.de [212.227.17.24]) by sourceware.org (Postfix) with ESMTPS id 62F933858C39 for ; Thu, 16 Sep 2021 13:25:49 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 62F933858C39 Authentication-Results: sourceware.org; dmarc=fail (p=none dis=none) header.from=cygwin.com Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=cygwin.com Received: from calimero.vinschen.de ([24.134.7.25]) by mrelayeu.kundenserver.de (mreue108 [212.227.15.183]) with ESMTPSA (Nemesis) id 1MuDTn-1mlYTR0NIL-00uZ8B for ; Thu, 16 Sep 2021 15:25:48 +0200 Received: by calimero.vinschen.de (Postfix, from userid 500) id CB74FA80D67; Thu, 16 Sep 2021 15:25:46 +0200 (CEST) Date: Thu, 16 Sep 2021 15:25:46 +0200 From: Corinna Vinschen To: cygwin-developers@cygwin.com Subject: Re: cygrunsrv + sshd + rsync = 20 times too slow -- throttled? Message-ID: Reply-To: cygwin-developers@cygwin.com Mail-Followup-To: cygwin-developers@cygwin.com References: <20210915194858.845bcd54c0c63246b40703a8@nifty.ne.jp> <20210915205433.014102a9c1204687135e1417@nifty.ne.jp> <20210915220450.a96397d8b8159d9adfbdab07@nifty.ne.jp> <3ca95472-9fb2-3e62-3e28-1bd2f85bc018@cornell.edu> <99e3ac84-bee1-4abd-de9a-4ea2e7b62b61@cornell.edu> <20210916180905.5f857dad0568be96ac36cba5@nifty.ne.jp> <20210916220201.63924a67f337ea876954d7c0@nifty.ne.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210916220201.63924a67f337ea876954d7c0@nifty.ne.jp> X-Provags-ID: V03:K1:+Lf9eP2sIlia+F6zTOSIO6t4k0vuPpqI7KhFooYS8phvK+J5Qot gNuAVCywaFIpDLJqaBnhNIxBbTXatOaEwneTT5125EN/nnnbvfonVQb6CioljZm3h9UVVtw 9eg0IxmndlIWEWGxv4/3mNa67gd3pYNcJiXI1kOWvjQDjVya+qp3ACBwj7gqjYk/kjtlDv+ cDKee4w2J9Rvxx8kwTGTw== X-UI-Out-Filterresults: notjunk:1;V03:K0:+mfOx2G6mx8=:xB+GRqRiHNjLsDy4p9Keau 3Vtk+ZVVEtap9Kc32TElEgwieX40OFqZz3T2sq2j/dnx6tntnwNFnGkXjKE1kNAX/ucMc4wkE IyAXEUQNQVFJ/FT/KXNoftBjVDkj5Qv2MPc3HXT97X0Tn/MvShXcd3QxHfnFeqRkANG73ACgg 3xHxXl4yCZLnvYn2qJ9+QNo0jKi4K/A5z53Bghp5sqFyJ8o6EpI4FfHokW20xDO6/zujHRur6 F26j2Y6j8eLmUYhQc2ETAeRfYPP/g+4qxyrMkPNUZdbcoHe39kNjkqPzx64l0GpC/TwDgd1sH T4RbTMIVj26+iiAdKpuQZz6Vup77mlxy7yyQ2pSAlB9TRGBFA2kPQF49M25SaFLR9Mah3Jrjt flWTpiargANMbdaswJ4wwinIBt3DUu2dp+AWM8TjhEdSMl9h4b69Q812HP0wbAN5bKkpnz33J jsWFH9lZXrOJktRDg5bZXm7xe2k3NQKG8jIjEdHRAwDsn20NP1eGtDeSBvbOkT+BVwKJy0KCb l/x6ufdwFsikk9TTysA1X4NGuUEIlmYfAOCv0PlkeDjXyPK9Qd5Rb2DzOsicL4cv2mMpplHpu e1XIcFNLjrzKe2r60/OtaMfsGNYq4Ig0gV8B9fKtqZNnn2RlMzcgDdvRlD0ok1TLDTm+HfA9g aqs0yGMViS+Gxl5hDIshsnZL0LgtcN+btzFo8W496kvO337Vdh/dwNKqTbnTtqQxX/ok+iFGP gWUfIdvpSXsA/Tm0hI+ULBZnfOxpbGEIEP/WaYqkpswI+qKUGtBK6PX+2FA= X-Spam-Status: No, score=-105.7 required=5.0 tests=BAYES_00, GIT_PATCH_0, GOOD_FROM_CORINNA_CYGWIN, JMQ_SPF_NEUTRAL, KAM_DMARC_NONE, KAM_DMARC_STATUS, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NEUTRAL, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: cygwin-developers@cygwin.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Cygwin core component developers mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 16 Sep 2021 13:25:51 -0000 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