From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from conssluserg-02.nifty.com (conssluserg-02.nifty.com [210.131.2.81]) by sourceware.org (Postfix) with ESMTPS id B41DF3858402 for ; Mon, 13 Sep 2021 19:37:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org B41DF3858402 Authentication-Results: sourceware.org; dmarc=fail (p=none dis=none) header.from=nifty.ne.jp Authentication-Results: sourceware.org; spf=fail smtp.mailfrom=nifty.ne.jp Received: from Express5800-S70 (z221123.dynamic.ppp.asahi-net.or.jp [110.4.221.123]) (authenticated) by conssluserg-02.nifty.com with ESMTP id 18DJbCp5030773 for ; Tue, 14 Sep 2021 04:37:12 +0900 DKIM-Filter: OpenDKIM Filter v2.10.3 conssluserg-02.nifty.com 18DJbCp5030773 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=nifty.ne.jp; s=dec2015msa; t=1631561832; bh=WcqLmSUA71cgv7b4P0eYhVkkjQRC4CEdrnJFeO+XxsI=; h=Date:From:To:Subject:In-Reply-To:References:From; b=yyjZTQwGM0wskZZ22bTOK/zKn5NsIfQTaBKMllLGPaVC9WxvlvJG3ZrcFQPcQjQ2t awxnUWkf0jGfIQNvsN0C6xbkbptZSLOuoamUS2Mrl/0Qkj+l0IfrwnxaX7SfSMW4y/ lfJajzCmBlYTFoVlBkNj0Ez0vfvgQg7xfycaJVXsmIlUTnBA4CwDgvNdn0AEHryqSx evHLjVpDntxIeAu8dyNfRRVxjP1d6uJ9c4xM5Wqa2KOabFcgWe2Vb9osuj4lRYxRmN ll3iUtd0j7cf0CvgJxpVhK8hNwz9lvG3V8iRcufG7r6kZepwsh4JZjdT0yE6t6H7+t yccFdKSvjl4Aw== X-Nifty-SrcIP: [110.4.221.123] Date: Tue, 14 Sep 2021 04:37:18 +0900 From: Takashi Yano To: cygwin-developers@cygwin.com Subject: Re: cygrunsrv + sshd + rsync = 20 times too slow -- throttled? Message-Id: <20210914043718.f420491c6723f3dc2e2d9753@nifty.ne.jp> In-Reply-To: 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> X-Mailer: Sylpheed 3.7.0 (GTK+ 2.24.30; i686-pc-mingw32) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, 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 19:37:45 -0000 On Mon, 13 Sep 2021 20:32:33 +0200 Corinna Vinschen wrote: > 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... I am sorry, I will add more detail commit message. > > 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. Thanks. I'll do that. > 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. It is by design. ReleaseSemaphore() releases maximum number of semaphore which the waiter can exists. If only one writer and one reader exist, ReleaseSemaphore releases 2 semaphores. Then cygwait here consume semaphore two times and return to wait state. This wait state is released by raw_read() or close(). > 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? The semaphore is waited in select.cc. But, wait. Wat happens if select() is not called? Released semaphore can be accumulated up to INT32_MAX!!? Let me consider. > > { > > @@ -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. I thought so, however, counting query_hdl cannot work as expected without this check. The number of query_hdl opend seems to exceed the number of writer. There seems to be the case that handle is already inherited here without fork_fixup. Any idea? > > > + 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. The PoC code uses PeekNamedPipe for query_hdl, so GENERIC_READ is necessary I think. -- Takashi Yano