From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mout.kundenserver.de (mout.kundenserver.de [217.72.192.74]) by sourceware.org (Postfix) with ESMTPS id 6AC893858C60 for ; Mon, 13 Sep 2021 18:32:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 6AC893858C60 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 (mreue106 [212.227.15.183]) with ESMTPSA (Nemesis) id 1N7hrw-1n2qfL47Ws-014jKV for ; Mon, 13 Sep 2021 20:32:34 +0200 Received: by calimero.vinschen.de (Postfix, from userid 500) id 679C2A8053B; Mon, 13 Sep 2021 20:32:33 +0200 (CEST) Date: Mon, 13 Sep 2021 20:32:33 +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: <20210909211940.51ef391e27d43f0421962cb8@nifty.ne.jp> <20210909214246.cd1ff1a3062fea27e51ad4ae@nifty.ne.jp> <33386baf-3b2d-d57f-2ad3-1bd328ed7935@cornell.edu> <20210911075734.aaf37697ba7db2ad14d911a3@nifty.ne.jp> <20210911113517.f74fc3ac1971bbf04c7a9bd1@nifty.ne.jp> <695ce1f4-4f7d-f3f3-6dd3-087467d67b28@cornell.edu> <20210912174849.3d38107568065a95aeb19c7c@nifty.ne.jp> <20210912200423.667e40eb1adc52461bbefa20@nifty.ne.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20210912200423.667e40eb1adc52461bbefa20@nifty.ne.jp> X-Provags-ID: V03:K1:xXX+ARjE/RnLCDrairCOfP6RF9+OvEIOPOY3sE6B7kHqCBmd3hk kWi5NUzcM2c6tWcUpvn381w4HF4L32063VzoD+XL899NW30/Xml1UHbcgZPlbWm92jD36a9 1ZauZx6OVmcJTJSYPoM13oj1IwOJqTXbab5x0Ex9zCTkjN+iE+6niMZmfotWPgrkzMVr59O UpqMro+KGJkFGzDSDyDtw== X-UI-Out-Filterresults: notjunk:1;V03:K0:DEHRI6eMHxU=:wSIW8bK/f2KA/pbZmERzrZ HaYidizsJ5q5dEebOd2zhjkXCv42xc+J3KqN4S98Gf7Cvs69pHIbWU82yr77sO8WIg9xvrENk 0Mrsjtv7/hdQyBRHHamptwMsOoqLZWpqlDyeo7ny8coNfmOULpSZL9cF3rQDgSde4bp4THWCM CJWb0jcomyEeKvStgjV9kLO4Tp0di3erjjkVz8+KvoosZW5Kr+LWGwpT+IO+XTTho935AcIOG 8gm8/tE+rc/9baHoQENf48QcumF7xJwyApw4ySt1YMH/42ByMDhq7h8W6F82mDDrsvu5Hov3M FuAq8agOdur2+w3Mq0kbSYq7pF8sEg5rlfrgENEMGACYYjGHZOW6XZUUrKOfyLtPnWFP5NeM5 HcjuJ2WwfT1Wxaq0CahVrHqPSM6RkPx4/NIJ2xbto6FmNxE3hLYoyIi+siciwCGmyPztSrX3d wGdI71ryTqYaySPhTJup9ycxjujCGA1/QW3qCuI0juSzX1uKBMZgZGUdWnQJ0hmmU+BKWdPHh 5ZJBzPS1ETtpW646FWKnEBFhKPYnUw+ttaHEIEoDPgmbodAfWrsxgTssAUVYq/grN2JD+WJym S5V8QKtemUnBfvom5v5zbriQ8lmYTyoaIJaIaqGvcRQ5U3nc+HPlL2h4XrN8WjRvRMpGE02yU HfoscohVbS5HzGiqyaeYHNnfqwF5gYWCaD3C1tg/Vx7b3SA0SE1i6/BJGzSP7gZjq2v9BJl2E vrffYf1qEY7PhXuLVkOVhGdvKMfHN9V00sRqsoblcqbnPAAklywkSgKKxKE= X-Spam-Status: No, score=-105.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GOOD_FROM_CORINNA_CYGWIN, KAM_DMARC_NONE, KAM_DMARC_STATUS, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, 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: Mon, 13 Sep 2021 18:32:37 -0000 On Sep 12 20:04, Takashi Yano wrote: > On Sun, 12 Sep 2021 17:48:49 +0900 > Takashi Yano wrote: > > Hmm. Then, what about PoC code attached? This returns to Corinna's > > query_hdl, and counts read/write handles to detect closing reader side. > > > > If the number of read handles is equal to number of write handles, > > only the pairs of write handle and query_hdl are alive. So, read pipe > > supposed to be closed. > > > > This patch depends another patch I posted a few hours ago. > > Revised a bit. > [...] What I miss is a bit more detailed commit message... > diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h > index 13fba9a14..f09af2c37 100644 > --- a/winsup/cygwin/fhandler.h > +++ b/winsup/cygwin/fhandler.h > @@ -1176,10 +1176,15 @@ class fhandler_pipe_fifo: public fhandler_base > { > protected: > size_t pipe_buf_size; > + HANDLE query_hdl; > > public: > fhandler_pipe_fifo (); > > + HANDLE get_query_handle () const { return query_hdl; } > + void close_query_handle () { CloseHandle (query_hdl); query_hdl = NULL; } > + bool reader_closed (); > + > ssize_t __reg3 raw_write (const void *ptr, size_t len); > > }; > diff --git a/winsup/cygwin/fhandler_pipe.cc b/winsup/cygwin/fhandler_pipe.cc > index 9b4255cfd..b051f5c03 100644 > --- a/winsup/cygwin/fhandler_pipe.cc > +++ b/winsup/cygwin/fhandler_pipe.cc > @@ -56,6 +56,8 @@ fhandler_pipe::set_pipe_non_blocking (bool nonblocking) > fpi.ReadMode = FILE_PIPE_BYTE_STREAM_MODE; > fpi.CompletionMode = nonblocking ? FILE_PIPE_COMPLETE_OPERATION > : FILE_PIPE_QUEUE_OPERATION; > + if (query_hdl) > + fpi.CompletionMode = FILE_PIPE_COMPLETE_OPERATION; This should be a single expression, i.e. fpi.CompletionMode = nonblocking || query_hdl ? FILE_PIPE_COMPLETE_OPERATION : FILE_PIPE_QUEUE_OPERATION; ideally combined with a comment. But then again... you're basically switching the write side of a pipe to nonblocking mode unconditionally. The downside is a busy wait: > fhandler_pipe_fifo::raw_write (const void *ptr, size_t len) > { > @@ -493,7 +490,20 @@ fhandler_pipe_fifo::raw_write (const void *ptr, size_t len) > get_obj_handle_count (select_sem), NULL); > /* 0 bytes returned? EAGAIN. See above. */ > if (NT_SUCCESS (status) && nbytes == 0) > - set_errno (EAGAIN); > + { > + if (reader_closed ()) > + { > + set_errno (EPIPE); > + raise (SIGPIPE); > + } > + else if (is_nonblocking ()) > + set_errno (EAGAIN); > + else > + { > + cygwait (select_sem, 10); > + continue; I'm a bit puzzled. The cygwait branch neglects to check if select_sem is NULL (the preceeding ReleaseSemaphore expression does!) And then it doesn't matter if the caller got blocked or not, it will always perform a continue. So why do it at all? Worse, if this expression loops, it will eat up the semaphore, because each call will decrement the semaphore count until it blocks. That sounds wrong to me. Btw., while looking into the current pipe code, I wonder what select_sem is doing in the pipe code at all so far. It gets released, but it never gets waited on?!? Am I missing something? > { > @@ -522,6 +532,10 @@ fhandler_pipe::fixup_after_fork (HANDLE parent) > fork_fixup (parent, read_mtx, "read_mtx"); > if (select_sem) > fork_fixup (parent, select_sem, "select_sem"); > + /* Do not duplicate query_hdl if it has been already inherited. */ > + if (query_hdl && !get_obj_handle_count (query_hdl)) > + fork_fixup (parent, query_hdl, "query_hdl"); I don't understand why calling fork_fixup on query_hdl should depend on the handle count. If you duplicate a writer, you always have to duplicate query_hdl as well to keep the count, no? Inheritence is handled by the O_CLOEXEC flag and fork_fixup will do the right thing. > + if (!DuplicateHandle (GetCurrentProcess (), r, > + GetCurrentProcess (), &fhs[1]->query_hdl, > + GENERIC_READ, !(mode & O_CLOEXEC), 0)) This is a bug I introduced accidentally during testing. This GENERIC_READ is actually supposed to be a FILE_READ_ATTRIBUTES. Sorry about that. Corinna