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 62DED39DDD99 for ; Thu, 8 Dec 2022 15:30:14 +0000 (GMT) Authentication-Results: sourceware.org; dmarc=permerror header.from=cygwin.com Authentication-Results: sourceware.org; spf=none smtp.mailfrom=cygwin.com Received: from calimero.vinschen.de ([24.134.7.25]) by mrelayeu.kundenserver.de (mreue109 [212.227.15.183]) with ESMTPSA (Nemesis) id 1MD9Kp-1pCPXJ2Db7-0097aF for ; Thu, 08 Dec 2022 16:30:12 +0100 Received: by calimero.vinschen.de (Postfix, from userid 500) id 7BAD5A80A3A; Thu, 8 Dec 2022 16:30:11 +0100 (CET) Date: Thu, 8 Dec 2022 16:30:11 +0100 From: Corinna Vinschen To: cygwin-developers@cygwin.com Subject: Re: Performance regression in cygwin 3.4.0 Message-ID: Reply-To: cygwin-developers@cygwin.com Mail-Followup-To: cygwin-developers@cygwin.com References: <20221208184656.ea6b9796eccc503c1e238f0f@nifty.ne.jp> <20221208235445.eb2a862dac8db77b5be8f6bc@nifty.ne.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20221208235445.eb2a862dac8db77b5be8f6bc@nifty.ne.jp> X-Provags-ID: V03:K1:QISVft764huHwYXLQDPhyJOqLkbzatWVAbVEfxT9sWwm3iWj+/8 X9OYZkmtwVQkd7u5wEiV9OTqJGKQZAS8Lq79OQbLL2ScBAvMx0pm2WVNrgVSSw/c4jF7pgY 5RH/br2H2WV1WheRjfmVDN7P+HMUpAemd3ohAMZYQWVunR7RI8WDl6Gy9X5pHXZGTJWsA7M /UTnBJw50DvXw7EwdDTFQ== UI-OutboundReport: notjunk:1;M01:P0:8nGgrZTeJmM=;NBY6i7XN5ZSLb3gpM+UrjoYaMMP Cpmdk/+M7AamFQ9wjD/S8pyFPdvzlxP1nJq7y+qEunIqEIlbrcKeWvLxDkVe31fpSdL9GpMNM I5hbi6RKGmIQ71pSYDyVFHdgTtHIOya0yMzzw1hNx87kobdfYLEkXymHeUJwX3P8XPtJuMwIx 45W1Vn9Fv3DetvhPyPntnRpR2vVlj00Y4Hy1ZjPJPVRdwDsKQVihnjbEpvwmShhAEbdUxlPya z+BqLeKAd9SQ583LdZfdBPQiO06zVzk2+VNaGv1QSpKNtxM656/48Z6933mVQ/Jd44wPcV8Gs nnr0eDOffl/VTvFdqWirJA6Metc4uTCwBM0XIeElpcKh4C3B71e6Mn39qKjzKTTGeGr0K8SWw IC/JHk58SPh3O7OAG/p9txj6LIbq3i6cC7dPsMmOe1hW0mU9CIl2jVIi0a+qZqsBuEmFQFIBO cY+hFf20qISXzgbWr7SQ6gasGR/afitJcE7rSFBTrahI0CF9mzb8Hd3YQ6mhKp+A7fED6I8yR Iuikyoj7AyXjF29+NVeOwtt8c5YuZXAXcx/mT58WAfU1SWZSlBW0cznltnvfC0INYXVsBiqlC aP7sCaLg5uZEd6i97E+jUyf2DB0WhWMBDRTg05tnN+5G7yhkMhmgHOEcFxqD/fkFytLNHEWwu zqFsZN1U96LVzDptL90kuNrNh6qIQTcl9e0FKGn0uw== X-Spam-Status: No, score=-95.2 required=5.0 tests=BAYES_00,GOOD_FROM_CORINNA_CYGWIN,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,KAM_NUMSUBJECT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Hi Takashi, On Dec 8 23:54, Takashi Yano wrote: > Hi, Corinna, Ken, and other cygwin developers, > > I tried to fix the following performance problem of > non-cygwin pipe and succeeded by applying the patch > attached. > > Although it is not smart enough, it works as far as > I tested. > > What do you think of this patch? Any other idea? > > Comments or suggestions will be appreciated. > From eb11d098dd8d0bf9c52291c5b5106715d5e3128d Mon Sep 17 00:00:00 2001 > From: Takashi Yano > Date: Thu, 8 Dec 2022 22:02:43 +0900 > Subject: [PATCH] Cygwin: pipe: Fix performance degradation for non-cygwin > pipe. > > - After the commit 9e4d308cd592fe383dec58ea6523c1b436888ef8, the I wrote about that on cygwin-patches a while back. Can we follow Linux kernel commit messages and use standard tags per https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html, please? I mean especially the "Fixes:" tag as well as the "Signed-off-by:" tag per https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin and https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes The commit id in the Fixes tag is typically 12 digits, i.e.: > performance of read from non-cygwin pipe has been degraded. This > is because select_sem mechanism does not work for non-cygwin pipe. > This patch fixes the issue. > > Addresses: > https://cygwin.com/pipermail/cygwin/2022-December/252628.html Fixes: 9e4d308cd592 ("Cygwin: pipe: Adopt FILE_SYNCHRONOUS_IO_NONALERT flag for read pipe.") Signed-off-by: If we do this from now on, that would be great. As for the code, I have just one nit. Don't use GetTickCount. Use GetTickCount64(). I just noticed you're using it in tty::wait_fwd as well. Please use GetTickCount64() there, too. This way you can get rid of the overflow handling. Ken? Any more input? Thanks, Corinna > if (!len) > return; > @@ -312,6 +315,7 @@ fhandler_pipe::raw_read (void *ptr, size_t& len) > { > ULONG_PTR nbytes_now = 0; > ULONG len1 = (ULONG) (len - nbytes); > + DWORD select_sem_timeout = 0; > > FILE_PIPE_LOCAL_INFORMATION fpli; > status = NtQueryInformationFile (get_handle (), &io, > @@ -338,6 +342,7 @@ fhandler_pipe::raw_read (void *ptr, size_t& len) > nbytes += nbytes_now; > if (select_sem && nbytes_now > 0) > release_select_sem ("raw_read"); > + t0 = GetTickCount (); /* Reset timer */ > } > else > { > @@ -358,7 +363,21 @@ fhandler_pipe::raw_read (void *ptr, size_t& len) > nbytes = (size_t) -1; > break; > } > - waitret = cygwait (select_sem, 1); > + /* Prevent timer from overflow */ > + if (GetTickCount () - t0 > t0_ovr) > + t0 += t0_ovr - t0_threshold; > + /* If the pipe is a non-cygwin pipe, select_sem trick > + does not work. As a result, the following cygwait() > + will return only after timeout occurs. This causes > + performance degradation. However, setting timeout > + to zero causes high CPU load. So, set timeout to > + non-zero only when select_sem is valid or pipe is > + not ready to read for more than t0_threshold. > + This prevents both the performance degradation and > + the high CPU load. */ > + if (select_sem || GetTickCount () - t0 > t0_threshold) > + select_sem_timeout = 1; > + waitret = cygwait (select_sem, select_sem_timeout); > if (waitret == WAIT_CANCELED) > pthread::static_cancel_self (); > else if (waitret == WAIT_SIGNALED) > -- > 2.38.1 >