From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758723AbcH3XN0 (ORCPT ); Tue, 30 Aug 2016 19:13:26 -0400 Received: from mail-bn3nam01on0102.outbound.protection.outlook.com ([104.47.33.102]:43108 "EHLO NAM01-BN3-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1757053AbcH3XNY (ORCPT ); Tue, 30 Aug 2016 19:13:24 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=waiman.long@hpe.com; Message-ID: <57C60F7B.9040804@hpe.com> Date: Tue, 30 Aug 2016 18:58:03 -0400 From: Waiman Long User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.12) Gecko/20130109 Thunderbird/10.0.12 MIME-Version: 1.0 To: Peter Zijlstra CC: Ingo Molnar , , Linus Torvalds , Ding Tianhong , Jason Low , Davidlohr Bueso , "Paul E. McKenney" , Thomas Gleixner , Will Deacon , Tim Chen , Imre Deak Subject: Re: [RFC PATCH-queue/locking/rfc 2/2] locking/mutex: Enable optimistic spinning of woken waiter References: <1472254509-27508-1-git-send-email-Waiman.Long@hpe.com> <1472254509-27508-2-git-send-email-Waiman.Long@hpe.com> <20160830150841.GB3318@worktop.controleur.wifipass.org> In-Reply-To: <20160830150841.GB3318@worktop.controleur.wifipass.org> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [72.71.243.137] X-ClientProxiedBy: SN2PR17CA0022.namprd17.prod.outlook.com (10.169.188.160) To TU4PR84MB0319.NAMPRD84.PROD.OUTLOOK.COM (10.162.186.29) X-MS-Office365-Filtering-Correlation-Id: 641dfc5e-666b-4def-6bfc-08d3d1291edf X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0319;2:ZYByNhwJwCsuOs4e8o4ypYfv8dZr3eoZN9fspNvVs+tELaxAkfE1YLEwbQbDIReISKNKIGy7ziHD8ku5Tnk4FZ+JtTeDY88Wtg1T8ErOtZKzwiYeMtMe1h8UnL8uifuCs0nnM/RD/9ZbBdVprLrq3jSNYqd8HsHgMRK7En6agiLphtYBn68DH5xCcZiu09UX;3:IIRGAGGjxjT4ZjYiLNloAzkU37pa5jVDETX/2AFhuko97G/OiCKkm1nki3FFe0uV5WmesJVMvU0yEqePPYTOaUle7jAtIsyvq0wycNW+Foo+QoD7WB40ixI4EeCzHtcU;25:DobQLHymvJ5aA/XwVIOtuddXHsTQc9Aa78LiRFStK8yyTq0brCuEgjyappNHTG9FTE8Equdj57rVQdV220I9fYBYrzrEKKqneCy2/dbyDSmH6LM+xa2g9CE7B2/5FMfCksiKQKfVdLVsrlZI6gFEbKlGGvjGDg0+ZpLZzEi/JCOWUIZGrGPenEzmx/IC9cYdzewF6ySIekxOu0VL0qmXpj6qARuuO5XgGCB1pTQ+5glHt++BvOhFUr37X1AxXrGVsrWm1Of5ZFSSWRUpCBOth/7PZl1/eTfHKCNE85LY33XzoPlYfVwPE2myFhw/85lyGxJy4YDg5hCP0maCm2Tn2sZ8IQO8PksIe1D7vf27kyAbmvD9BbyOtA/KbahEvTmPmiCKSopVN2KDWq/hU8+g2eBstjDrEQJX7Tol79tlz5A= X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:TU4PR84MB0319; X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0319;31:23EdTHvUE7hWLvog+14pM8fC7yg4Rv4BtbpXa/+UKgF+iAFcJ9xMY/3rQJbFGkDomKYpiPb4T615u1brOmFHUFldUkMFmUSbJXSFKsKuagdvfUF/fTBr0gwZ8j1aW2zJK0kALFpoJZERPHfN2DXgRNsrejrihAmVxHGEhP5XWa++l0PRp+S9CDNR6VdAvZgRkr+IbdbyhxqUGptyAXqpGSTXbm627Ag2BN0lFA/pw/k=;20:xFE80SOBl0/W1reIyLWQuQTi7hzminxVanL6wMtLXPFwABWfXgBk4zdMouvKa8wONAx3EUDV5fzD4yVhqqZC19KvcCuhyLlKIRrLRQLY8EaxrfpspG5vYXAtfZfz4LMTqXktAcTHxNBq015bL3yIX+ntSH5+Bu1X8vtL/hYQw08kJA2NZ1jKm8UB+aESDU0DwfuBmz48LzYjQZ+3JjA1xdiOgN9CQ05P9cDQx3lJ/s3/pyfCl5FiR5cwn2eUmSz6lV+0tT8OZTbzcGyQJazv3u2bFydbivO7CECbS8Az64FCVX2FGIxIZ59fPdyZZqVb3L67pJ61sRFIwZNS3gm9wvMz7FTEtV7baBIWLgtdzvDQeeSPMd2Lxz4w+xxNg0nrjzyqVbm+qIoQ0SUvbn/sOcRB8kC1W8sMNTAdtzm56gW8Bf9szox/5sqjDo1lJ/7bHLMIt0uy6uCKXvAul02KS26CsqmhHXUmD40HANyA2smTAVkOgPupmFJyuZgJDgCC X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(6040176)(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001)(6055026);SRVR:TU4PR84MB0319;BCL:0;PCL:0;RULEID:;SRVR:TU4PR84MB0319; X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0319;4:iFxHL2TzyRdGizLFZ8ej9glUVXhHc8qzwIhvlW8w6UpO5yI1bh65qij9qvfCkatl2ULlHVxdTydcz7uhBgfxvlKcfw8cyI9VDdjg5HyQks6Om2caqYZUc42vEaUEVQu4HOpeu5eK/+aPAQVzQMnZ2dOHN/qBK/S14Rg7wmzB2fPYur/rL7EN0yaIRyXy1JGeRPtSHm1/J6G9210RCIyZ6w/+r1hL+Qk2bV+OfQdEgfhrU29ZTiFUW9h40wVUb+/ec8CaTgoLdotdQf/w+nFUTxpCoabZdYc4J1J5uZErBPooEvoW95Y7VEAc0eVltJihjiugaNn65Qi2U8eCLtsxAIyQO4yjCITAqRLLjXC/JNPZZmRuK+gwmY21GYvqF45y+eqvkCG1B06eqz5U6UxyfA== X-Forefront-PRVS: 0050CEFE70 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6049001)(6009001)(7916002)(377454003)(199003)(24454002)(189002)(65816999)(80316001)(7736002)(47776003)(65806001)(50466002)(23756003)(65956001)(66066001)(117156001)(36756003)(64126003)(54356999)(50986999)(76176999)(86362001)(68736007)(7846002)(8666005)(305945005)(87266999)(8676002)(81156014)(81166006)(230700001)(83506001)(6116002)(4326007)(3846002)(42186005)(2950100001)(2906002)(59896002)(92566002)(77096005)(586003)(105586002)(97736004)(189998001)(110136002)(33656002)(7416002)(101416001)(106356001)(5660300001)(4001350100001)(7059030);DIR:OUT;SFP:1102;SCL:1;SRVR:TU4PR84MB0319;H:[192.168.142.194];FPR:;SPF:None;PTR:InfoNoRecords;A:1;MX:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?iso-8859-1?Q?1;TU4PR84MB0319;23:fos/MrgxqBt+4+Xq5IMGx7ZvnhdFCB2qoe4MaE6?= =?iso-8859-1?Q?z8ESrJfZGxKDjUs+hd8dkwC9DBN3JBSZCcJy3In38Eh5Ap7V1HT/o04NaX?= =?iso-8859-1?Q?TjFu1s1pqJJCiWhGcVeeeGA2Ru0FGbuT4BtA7kK8ZchDq5kGG6pNA6bhFj?= =?iso-8859-1?Q?c9PPr2eqTANa9aGT2jLXsb9aNETbj6z3vBV3XiUov8eyPbZFcKMXLrD4AC?= =?iso-8859-1?Q?cF88VJvn29sza8aAkyLMIJMt0IIfjhLlPs4CObDRBIOQk5nytiOhEqnhSs?= =?iso-8859-1?Q?RD7tdY5gsVwk4P18UhVAWlpuU0d2FpLTFFYGYvEJb8n4ZhdYJ6i6zzI+Ot?= =?iso-8859-1?Q?yiBe+uSVTZTFCnBPi0Jjr0oY4Z5JEHivyCM0jaLe2Znstc/YRf64phgRA7?= =?iso-8859-1?Q?N7J6iVqSHWukEQusuDwTrBOtEIdAA+YT9ltTHfjPUZe3prvNxFopr8KBCN?= =?iso-8859-1?Q?/0sRqEsN1uQs9pcKyG+ofkB0MjOw61RePYs105ZxjkCMARXTTxB7lOrKuL?= =?iso-8859-1?Q?RhVhgrxULAUEDLsWYUBuv6Nktl54CprEpR7Bef0CEXboer4J2mFx2Eobnk?= =?iso-8859-1?Q?76IJVnbuBCE3poeS44acSq1J0qVzKDOs9+R5PxzernV1PZVtUwMF6H1aaZ?= =?iso-8859-1?Q?565K/7ZrGNrLTv1NII5wSia02fH0G22DnnytR/fKqAOj/Iy2j7FkdJDs2I?= =?iso-8859-1?Q?Pt1dliodDwSA+oHSpbdK3n6dZBnT/eq0o4Iuyd+1jPUHkpzF2uuEc2yYVb?= =?iso-8859-1?Q?Aymqa8Fk0AcWW1xEzFjIPPUIjmDlmHBDFTtBmm1bxtkfdZ75A0JtC3gt6N?= =?iso-8859-1?Q?WL/FwLzrPyqCT2eZZqE+bUH2h2arNjhNQ7OS6YyGAeKytBM6Bxl+fvRVPl?= =?iso-8859-1?Q?IW87u3pV1f476UEYBxl1jXsveeVElBjSEENgq8u6FWMXE5r97xV4IaZ1q5?= =?iso-8859-1?Q?QFdPtUpiqjUVtA8I+A4zwdDimlJgLCKLnErx4K8KUkyo4Y+nDJSeuKfDl2?= =?iso-8859-1?Q?J/zXnXTGfAI2Uxmei2u8ky9S/EKHUiWMkaAPUlZ22gX1XchMemPSWTYYk7?= =?iso-8859-1?Q?Z2/Xy06b5IEy+fj01BpACJMcD59/rrUpygbCfUT8dQCaGqTMTT30MtD69r?= =?iso-8859-1?Q?CuDaxdIAnbb3o+MD6jTzX7FK226SoSz0LiD7ccFZ59LeoqnDlHwTCz67gi?= =?iso-8859-1?Q?IhnoxaEE4+VUSfC7lCdw1nbrX4idBDGzRQhJgZ8RziIASVSaS3LhV8JaPa?= =?iso-8859-1?Q?P4jV3zI3Nh7I6I2+vLpi0JKTJG92vmLcunjmG66t0l9UFIJvQqvVLKE+0M?= =?iso-8859-1?Q?yeGw0Km+4VPbZcpSVAJK3yIU7VKb51AQ110WvZEk1ZhhFcU/GXFcxRUYq8?= =?iso-8859-1?Q?/NPVKRS5IkKcC2W3y7aNipb0uKTyW?= X-Microsoft-Exchange-Diagnostics: 1;TU4PR84MB0319;6:EZLpwsO2T5dK78L8Jq/qt3YJulTRZKWmbyo2HhCOf4ATFyqqgUCVsEE3hnI6p4+0quTahCUMg/3u5axFoz1eJhbg0ibMr8LYKao8Fdf33xFeAN+7bfyhqJT/ryHMU8Xi5WxpLdyVra/Mu3cqLgEU3ls88vgv2gQS7d8+xP4i9qsUwybkmVy6TjWgScEwZJRe5vi+ds0fyySyasc+yadJ9JVPZMjDFn4q8FyHYKq24GiMHr9LVtTQjMEH+6aph+WGnit5sFwr6wtweTOB0IXY3qU84jG3c/RLlzJtzkrjygfO38myshdT3IFglwebXRpwmXrvX1+D273IEY7coPuLZQ==;5:U3XAFPq4fJ8COHr79IedRMe8Z8E6Gu9i24Z2acqwP7GjcExMjxh9vX9itSl3m0PG+Xw5YJBtsOO2N15vw33drPvHHJAvEGVF5wZ4dUJSE7wqtVtjaqkJJcw3Cn7DHOsN2RBjrmcBTOJAI0NqlmjUwA==;24:7YUg4auHznt+VHGie92NscuVtuhOTPK6tb7s+ZLzGjFmeWCleFva4NkUrFcBAXYVaZLSeZ5BpanNPm3c/sxbrOJ+XOyYdinYvODBqNDjUTE=;7:FZJTK1WrtMEhrIXu34jv/xJ+BZzyX5DUUqscj9mwkHnniijiYCjmJF/OhTwvizUR9Omz4V6ivjXPLJL0bVOEJo/YSO/elXgjqKgbvZ+bQlNA/H8GChgKiqh1flTgbUHWHh6bHv/3ac4a12OIygmfKJ0T2JZSLdSkV+wkGwzFux5XME7d0m0Ba6gEISJmkgLbiH5gvSRLir8SzLon0lhMeGP0z+KZHXJLMut68u8FJy07hEbF3izSKdRvv/ELyz41 SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: hpe.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Aug 2016 22:58:11.9302 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: TU4PR84MB0319 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 08/30/2016 11:08 AM, Peter Zijlstra wrote: > On Fri, Aug 26, 2016 at 07:35:09PM -0400, Waiman Long wrote: > >> @@ -624,13 +649,24 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, >> /* didn't get the lock, go to sleep: */ >> spin_unlock_mutex(&lock->wait_lock, flags); >> schedule_preempt_disabled(); >> >> + /* >> + * Both __mutex_trylock() and __mutex_waiter_is_first() >> + * can be done without the protection of wait_lock. >> + */ > True, but it took me a little while to figure out why > __mutex_waiter_is_first() is safe without the lock :-) Yes, if you are the first waiter, the condition will not be changed even when new waiter is being added to the tail of the list. > >> + acquired = __mutex_trylock(lock); >> >> + if (!acquired&& __mutex_waiter_is_first(lock,&waiter)) { >> __mutex_set_flag(lock, MUTEX_FLAG_HANDOFF); >> + /* >> + * Wait until the lock is handed off or the owner >> + * sleeps. >> + */ >> + acquired = mutex_optimistic_spin(lock, ww_ctx, >> + use_ww_ctx, true); >> + } > That said; I think there's a few problems with this. Since we now poke > at the loop termination conditions outside of the wait_lock, it becomes > important where we do the task->state vs wakeup bits. > > Specifically, since we still have state==RUNNING here, its possible > we'll fail to acquire the lock _and_ miss the wakeup from > mutex_unlock(). Leaving us stuck forever more. > > Also, we should do the __mutex_trylock _after_ we set the handoff, > otherwise its possible we get the lock handed (miss the wakeup as per > the above) and fail to notice, again going back to sleep forever more. > Yes, you are right. I am less familiar with the intricacy of the sleep-wakeup interaction. > @@ -638,7 +636,8 @@ __mutex_lock_common(struct mutex *lock, > > lock_contended(&lock->dep_map, ip); > > - for (acquired = false; !acquired; ) { > + set_task_state(task, state); > + for (;;) { > /* > * got a signal? (This code gets eliminated in the > * TASK_UNINTERRUPTIBLE case.) > @@ -654,30 +653,23 @@ __mutex_lock_common(struct mutex *lock, > goto err; > } > > - __set_task_state(task, state); > - > - /* didn't get the lock, go to sleep: */ > spin_unlock_mutex(&lock->wait_lock, flags); > schedule_preempt_disabled(); > > - /* > - * Both __mutex_trylock() and __mutex_waiter_is_first() > - * can be done without the protection of wait_lock. > - */ > - acquired = __mutex_trylock(lock, true); > + set_task_state(task, state); > > - if (!acquired&& __mutex_waiter_is_first(lock,&waiter)) { > + if (__mutex_waiter_is_first(lock,&waiter)) { > __mutex_set_flag(lock, MUTEX_FLAG_HANDOFF); > - /* > - * Wait until the lock is handed off or the owner > - * sleeps. > - */ > - acquired = mutex_optimistic_spin(lock, ww_ctx, > - use_ww_ctx, true); > + if (mutex_optimistic_spin(lock, ww_ctx, use_ww_ctx, true)) > + break; > } > > + if (__mutex_trylock(lock, true)) > + break; > + I think the set _task_state() can be moved to just before __mutex_trylock(). In this way, we can save a smp_mb() if we can get the lock in the optspin loop. Other than that, I am fine with the other changes. Cheers, Longman