From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755161AbcGTSh6 (ORCPT ); Wed, 20 Jul 2016 14:37:58 -0400 Received: from mail-by2nam03on0094.outbound.protection.outlook.com ([104.47.42.94]:52040 "EHLO NAM03-BY2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751721AbcGTShx (ORCPT ); Wed, 20 Jul 2016 14:37:53 -0400 Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=waiman.long@hpe.com; Message-ID: <578FC4EE.1070000@hpe.com> Date: Wed, 20 Jul 2016 14:37:34 -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: Jason Low CC: , Peter Zijlstra , , Ingo Molnar , Chris Wilson , Daniel Vetter , Davidlohr Bueso , Subject: Re: [RFC] Avoid mutex starvation when optimistic spinning is disabled References: <1468858607-20481-1-git-send-email-imre.deak@intel.com> <20160718171537.GC6862@twins.programming.kicks-ass.net> <1468864069.2367.21.camel@j-VirtualBox> <1468947205.31332.40.camel@intel.com> <1468969470.10247.15.camel@j-VirtualBox> <1468989556.10247.22.camel@j-VirtualBox> In-Reply-To: <1468989556.10247.22.camel@j-VirtualBox> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [72.71.243.182] X-ClientProxiedBy: BLUPR0401CA0015.namprd04.prod.outlook.com (10.162.114.153) To DF4PR84MB0316.NAMPRD84.PROD.OUTLOOK.COM (10.162.193.30) X-MS-Office365-Filtering-Correlation-Id: 200af60e-600a-416b-355f-08d3b0ccf460 X-Microsoft-Exchange-Diagnostics: 1;DF4PR84MB0316;2:YNI5obO0c9WKGyhmAEvNUllNk06JsubL79lMhOjqrpzxqhorsF8vXIF7+vOvdUG6VUyswqZCuIosGCwzY3Eq0z6GBgvPef7cdDRcXe313wofBtbhRbf9/WGe2pglX6x4I0pgjA+US0Cum0tRuKvGgw08LDAIjZSGDhCGiohMAeIIL93IEL66Db0SUIo8K2Wu;3:P4NECBtyuLr7FsgVcOxXMcNFL/dr39xHa/ie0zjGpTVSc3iqodWhBYZ3XvpnpyQF262qb974Y2O8jKmbXGNE8BVlIlpRgJsuav4zIEs6NdYiE7d8rhnomkhR/QsRCHM8;25:Nc6rZVc7lln48Xi3X2W0Elte58uBNHGKzfst2ZvoWnWMoiFqyuPncx4YCVlVglPMlPNy5EcfkA9gvmTL3l84LPIRWKeKcVPx8kMbNm+KaD7pz5QVulyHkkDBvd9jQdrEfpoenvX+3huon9BgrZNX/BKn2/BsWcvkAa38rmLfgE+hXyMLm2ZGh2QB66gucx24hHOeLWjhkUlD/k53vcypvE1dkIjYqjbrwh9ggzt8Ukt51leCdhdXIDqrfXGzsZti8pFO74mzQxCV+OT+MfXRmjLl8ca0AJGdr9LanumomSSTTZ2vhmrlJhV8auCCOzY6Pz7opw+6wPjeRQeuFrmf+lvXq8boWZW7cJZDwlwnSeuEy1Pl1gc/3rB3IeFmi6zIuEU+6Jz6n/rL0SAZ8/y97JWFTH8Liz8OyDZ+GTjcDd0=;31:WlrrnqBWON07kzevtDZ6ypR2Qjdn+te9EwxzsAAz9equUnsEjMpFW8rXEQRJStMFXxENPEFKcdlNSUfKNE0ymbmSbvl97TJOqfc74ySGZWbFIVD8a6g7YzbtdGh2G4vZ+p3XxWuLnwTJF5NBxySiiLQjnhHO8VBtGnRkZSTv95M68sOWnT1vTSoBU6Cbq2C6s/3fUHhez2/lafyeTx3w+Q== X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:DF4PR84MB0316; X-Microsoft-Exchange-Diagnostics: 1;DF4PR84MB0316;20:fxAtK/Yz6nCqaqJWwznjlre2cZxmejXS45ZFz1Gb3boq/YvtAD84AxOrVRRi7DeLT/JZs09quxweMXEtz0/pbqXUWRVzpRIu8OI9/Hfsrs8RidSGRJVmS9+CSJO0tO4vLtJvtDl5Nzp0DxyfKuT5yliKMhDh26XuAjW8tvRovbKEipEBBvDfZrgsEJXfznSwMiO+vacdBM/hQRPg7Zd6Ee6rl1A1yeC9q/TUdxHdlKqVGbhxJS/xsO2Y5cEU29HkpP2gEnepKIJOyOCRNC0tYdX273N/yWPlX1Lu9k5rYFJk82RBoqt0LlTHnzX6SNIFUsAo1uZS/dyPkxI5/eg/vW+q6EZJWELXzI5YUWMXH2YkImYtgaz4Gl2q0mnf6PaGwLVtMhavLt/x4P56ZqiCyXFruOHIt9fhsMbajDE3d0OFKSHQOaTFd5Q4a00DLf125qFb17TNTSxP0RzARiyqsz8ST8a/ec3xP9BrRqMYVJQe8StLzzjQ9UWjQjwKfHyW;4:c2aStVV3b0INrvIfDSw1USxCz3jRF2VxxKNdBeBKDnuSXNF5DolJxAOZCjwDJAuhT8e8SLUtaTBvHUOJzGrxRuRIGT78QFjY+oWr/FUOa5ojwVr4mNS0/H+OFbqNXWiphH89/fyuO2DoYg9VNHxWuSAkc/WdHe2HWK5+R1Zt0qjBlIlZrHZKG8ks3jyboTSsszDTWohXKVqRF8EQJRd4/TsBJ727/9bD5EzZ+RYFCi3wJSNNlA0hZuPdmZfnSGpJybCD/LGKjt9BiDBFdC2UGrC3SBDagSVPRH6ogjSVAOrlJa9LeON40DgMTpzO5bEMLzuMvRwSOTepaBT/bJhbAE5NqiosPjqog7DpUzt97R+Fgyoa09OsmecGQphVHxaeZHLH6XLwX9Xoizf+Pk70w7/rGWSN26q5lS5S2ozOZmjEqF/2zyCEYYLtFC0sfXHW X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:(227479698468861); X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046)(6055026);SRVR:DF4PR84MB0316;BCL:0;PCL:0;RULEID:;SRVR:DF4PR84MB0316; X-Forefront-PRVS: 000947967F X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6009001)(6049001)(7916002)(377454003)(189002)(24454002)(377424004)(43544003)(199003)(80316001)(93886004)(19580405001)(19580395003)(92566002)(106356001)(86362001)(64126003)(3846002)(8676002)(36756003)(110136002)(83506001)(189998001)(4001350100001)(117156001)(59896002)(47776003)(97736004)(65956001)(33656002)(66066001)(4326007)(2906002)(68736007)(65806001)(42186005)(305945005)(23676002)(7736002)(8666005)(7846002)(81156014)(81166006)(50986999)(87266999)(586003)(2950100001)(76176999)(54356999)(77096005)(105586002)(50466002)(6116002)(230700001)(65816999)(101416001)(7059030);DIR:OUT;SFP:1102;SCL:1;SRVR:DF4PR84MB0316;H:[192.168.142.174];FPR:;SPF:None;PTR:InfoNoRecords;MX:1;A:1;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtERjRQUjg0TUIwMzE2OzIzOlBQNjEzdHZyU2VJMTdaWjBUZm5qS2NjdzJZ?= =?utf-8?B?TlNCeHR4T1NlU3lQT2gxSVpLNkRKT0trdzBhYzZVcTFPMlBrbDBYMHBCWGpU?= =?utf-8?B?NlBkRzI3V05DSFZFU3R0YnRPWWpuTnhMZnM4OFBTeVY1TXduNDFPVDhsUkJk?= =?utf-8?B?R3JVRUtKdUxWdlhjR2Q1WmxzaE1LcmNvRGd1L0ZzUEc5dHk4UzhXREIxQTg3?= =?utf-8?B?NklIL0RGK1JFRnlJdHdvcDQ2S2ZVbXlEeXBFUE9lWDVNZzlkUEw1NjkxMTdh?= =?utf-8?B?VTFYc1g3QU1jVkdyaDcwS1NiNVF1WVNOL0dkY2JrYjEwbER5OWNjZ0JOTndS?= =?utf-8?B?WmVNdVQrN3huakl1Mm5hV0FQeHMvTmYzang0Uk1VeEd3RUxSOGs4RFhxTzEw?= =?utf-8?B?MDNkcjc4enN0dnIrU2dxRHFpQ1JxMzNta2kzR2k0c3YveTg3dnFQdXR0akhj?= =?utf-8?B?byswTHlkZ0hMYXMwQktxVVZkbEpOQjIycWtDQmRkZy9NcnZSRUFYTTRNS0U4?= =?utf-8?B?QVd1REUrMk9RcWk4WVFvSHdGQnRtSGVZbTF0YVc5My9GOVJabUZKYmovNlRz?= =?utf-8?B?OVZWWG5UVVNneVFmT1FvS2FEaHd4RzlZdm5vL1FlNmxKUFFIaXphbURZM1lC?= =?utf-8?B?NnpWLzh2bzhMdHo2d2NPNXg1ZkdDKzNSWmNVV3ZhSUV3YUxxWW9ZQUxsZHR4?= =?utf-8?B?VTM0YXZYSkJySm1oUzRTc1JuMWEwZmhhbmI2N25zMk5WdmVKaXdXVHdFelQx?= =?utf-8?B?QWxpN1ZzaXlvNng4NDZYQk1URlhJRzZLWHpFNitDV29tYUQrSkt2bGhSUUJV?= =?utf-8?B?VFRZR2dNT2JMdXBjZFVNVC9ZQzJ4Y1hNQjdIUExrMjYwOE10NmFSczhFT1Zp?= =?utf-8?B?TmJORjYrWVJQZ24rNjlxc0RuVWlGZHBoL1k2Y3hpRGtRY256dFpzV1ZsNkx6?= =?utf-8?B?SW5MQmVoV0RIN014MVIvd1UrZXp6a2lndERrVE5ndVI4ejJwSUs4UE83QzVz?= =?utf-8?B?aXY0RkR1WUMxM1JRZ2Nuc1poNUdIVmpub01SNy8xdUoySFdWbGxucDN0OVVO?= =?utf-8?B?Tm1HcHBNdVZOWWZmenM0dVUyamhuUGFFU3dRYnBpNG4yVWZhWkpsQ2o2bVpl?= =?utf-8?B?Z3RhbDFYUU9VeFA0OG9ReDRRQlk5THYrWXNSMmxjd3E4WDZSS2piT0Y2d1Bk?= =?utf-8?B?T3FuRXNzTnNGZW5qUlpBMHl1R24ybVZwMXc1aU1vSnIyV2xsMkRGS0RvZk5D?= =?utf-8?B?ZExTUU1ZcGtEQkQ4d2VTTmIvYTFqeHkwcTdtSENZYkpvakdEcWNFNEgrQk56?= =?utf-8?B?anVuMllzRmNLckFNb3poZy9ZWmlaV0dOWllqTjZzeFdGWUF3bFBKYndkUStz?= =?utf-8?B?aDJRbWJRMkJiMXNYR055dzRPejE0RWxPTnhzd3kvZUg2eFJZdDZuYkFTUld4?= =?utf-8?B?RmlhcUFlTXdiYk8wdUdnVXdUWDBSTk42YkYzeE9WRHlIdGc2Z0FpSU9oM29i?= =?utf-8?B?WWhHMWhCMUppSzBTZVR4THpnUnFIaWNNcFZBUkNuTnhSYjUxcmY5anhjMzZO?= =?utf-8?B?RjM2MGNtVGZDWWV2T2h3MnpwTVlUMVhyQi9MRVNYaDlUOHZyS0VjamZoVXhy?= =?utf-8?B?UlRPdHljLzh4NzhYbFVncG5Ia0w3bU54SkhjYjdjTmpsRlJKMlZKZzh2TlZO?= =?utf-8?B?QUhEZWxzMVdaNy9kbDUxeUJnYjFRek13VDg4MWlSS3BYcklVUFd3L2lVTUdi?= =?utf-8?B?K05iRHhVZGl3dGJMajNFU1BEQ0hqa2M0c1cwQnE3MXhMb24xMXp0bU11VDkz?= =?utf-8?B?M2ppc3JtRjcxL1FiTVUzUWhDdEN4VFRZWS9kd3RBZUJMd0M0Vnpuc01rd1hP?= =?utf-8?B?eHBBd3U5UHJySFZucTErM052bE1aTHNHSUJvenEvcHk2VUpBRnF0ZzUwS2s5?= =?utf-8?B?V1h0VjA5THhBPT0=?= X-Microsoft-Exchange-Diagnostics: 1;DF4PR84MB0316;6:UkAbC8gXR4MDMKMCVMJdcThEa2QiM3G+Sll0JOsTskmgVLNM0qGKYXEWdISduDewVEKgbX/IWokRYDP2HLyVoeHF+o+qMCAyevCyZRwvTyqbha62o8E7rN/gRUXSCXd2fjRSXhKMeMYqm0jmfl2SBu/nEFN+bCpCB2JWItcpLRQBlMMLwK06MAEvfH+si2x69+5CkU94I0uNP6SKQrPqZAtuyqzNsLqpcg6ZV3dc1x80HKR9pNcWAIUF9iRh4cHf+UnZ+ARSUMlsV8Ao54AR67RIBzhbvwKtgrjkby5aCWptVECobqaaZwAZeIUdmHTouFM5q2YR3bsTbDJbDBE3qw==;5:dRfSivWpr4ycW14zs8Url0U06PMNS9OBxEZE99SI25u7nl5tg0yhM+sE17zmm8uja/LNUe2uzttZ1ILAY62DfZOTmN9uakGsSfw9EZGvOo68B1WbMAdL7V4uU1uMTDnlhzxGH1+f6YL4Z4AMOcFhGA==;24:xQHAf30va8ZWsWRfa+UvWAs9QSrU28OXYIXOzazHwa8yu7b5iazUXZ7JGSaswTxZnvqJSWHiRaGoMsXhDOkhqzFbRz+h+vwOKyM4RKsu1JA=;7:vjC0lbOp/g6qLOEC7CkhAeWerkNeNl/rKKv0rErhAuaQZghjIZtikTZF/Mlllv8h4o8d8YYZKWqFneMopjv2qVhHHFGcd9npaWaxhBw4TtjOkwB3YwCCktUCfPrEj8uq1FN8yXLlkJhZFxhdAXJ5jkDOYnaiIeaY9CTMuhn28ZbmFAedYrCZWKf1YCrqgi88KJBQi23eR9lsZVG7fumoZcJvlAQ1gIGikB26kKT2x1PIomgWEx70PI67+xmxvg21 SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: hpe.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Jul 2016 18:37:50.0937 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: DF4PR84MB0316 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/20/2016 12:39 AM, Jason Low wrote: > On Tue, 2016-07-19 at 16:04 -0700, Jason Low wrote: >> Hi Imre, >> >> Here is a patch which prevents a thread from spending too much "time" >> waiting for a mutex in the !CONFIG_MUTEX_SPIN_ON_OWNER case. >> >> Would you like to try this out and see if this addresses the mutex >> starvation issue you are seeing in your workload when optimistic >> spinning is disabled? > Although it looks like it didn't take care of the 'lock stealing' case > in the slowpath. Here is the updated fixed version: > > --- > Signed-off-by: Jason Low > --- > include/linux/mutex.h | 2 ++ > kernel/locking/mutex.c | 65 ++++++++++++++++++++++++++++++++++++++++++++------ > 2 files changed, 60 insertions(+), 7 deletions(-) > > diff --git a/include/linux/mutex.h b/include/linux/mutex.h > index 2cb7531..c1ca68d 100644 > --- a/include/linux/mutex.h > +++ b/include/linux/mutex.h > @@ -57,6 +57,8 @@ struct mutex { > #endif > #ifdef CONFIG_MUTEX_SPIN_ON_OWNER > struct optimistic_spin_queue osq; /* Spinner MCS lock */ > +#else > + bool yield_to_waiter; /* Prevent starvation when spinning disabled */ > #endif > #ifdef CONFIG_DEBUG_MUTEXES > void *magic; You don't need that on non-SMP system. So maybe you should put it under #ifdef CONFIG_SMP block. I actually have a similar boolean variable in my latest v4 mutex patchset. We could probably merge them together. > diff --git a/kernel/locking/mutex.c b/kernel/locking/mutex.c > index a70b90d..6c915ca 100644 > --- a/kernel/locking/mutex.c > +++ b/kernel/locking/mutex.c > @@ -55,6 +55,8 @@ __mutex_init(struct mutex *lock, const char *name, struct lock_class_key *key) > mutex_clear_owner(lock); > #ifdef CONFIG_MUTEX_SPIN_ON_OWNER > osq_lock_init(&lock->osq); > +#else > + lock->yield_to_waiter = false; > #endif > > debug_mutex_init(lock, name, key); > @@ -71,6 +73,9 @@ EXPORT_SYMBOL(__mutex_init); > */ > __visible void __sched __mutex_lock_slowpath(atomic_t *lock_count); > > + > +static inline bool need_yield_to_waiter(struct mutex *lock); > + > /** > * mutex_lock - acquire the mutex > * @lock: the mutex to be acquired > @@ -95,11 +100,15 @@ __visible void __sched __mutex_lock_slowpath(atomic_t *lock_count); > void __sched mutex_lock(struct mutex *lock) > { > might_sleep(); > + > /* > * The locking fastpath is the 1->0 transition from > * 'unlocked' into 'locked' state. > */ > - __mutex_fastpath_lock(&lock->count, __mutex_lock_slowpath); > + if (!need_yield_to_waiter(lock)) > + __mutex_fastpath_lock(&lock->count, __mutex_lock_slowpath); > + else > + __mutex_lock_slowpath(&lock->count); > mutex_set_owner(lock); > } > > @@ -398,12 +407,39 @@ done: > > return false; > } > + > +static inline void do_yield_to_waiter(struct mutex *lock, int loops) > +{ > + return; > +} > + > +static inline bool need_yield_to_waiter(struct mutex *lock) > +{ > + return false; > +} > + > #else > static bool mutex_optimistic_spin(struct mutex *lock, > struct ww_acquire_ctx *ww_ctx, const bool use_ww_ctx) > { > return false; > } > + > +#define MUTEX_MAX_WAIT 32 > + > +static inline void do_yield_to_waiter(struct mutex *lock, int loops) > +{ > + if (loops< MUTEX_MAX_WAIT) > + return; > + > + if (lock->yield_to_waiter != true) > + lock->yield_to_waiter =true; > +} > + > +static inline bool need_yield_to_waiter(struct mutex *lock) > +{ > + return lock->yield_to_waiter; > +} > #endif > > __visible __used noinline > @@ -510,6 +546,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, > struct mutex_waiter waiter; > unsigned long flags; > int ret; > + int loop = 0; > > if (use_ww_ctx) { > struct ww_mutex *ww = container_of(lock, struct ww_mutex, base); > @@ -532,7 +569,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, > * Once more, try to acquire the lock. Only try-lock the mutex if > * it is unlocked to reduce unnecessary xchg() operations. > */ > - if (!mutex_is_locked(lock)&& > + if (!need_yield_to_waiter(lock)&& !mutex_is_locked(lock)&& > (atomic_xchg_acquire(&lock->count, 0) == 1)) > goto skip_wait; > > @@ -546,6 +583,8 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, > lock_contended(&lock->dep_map, ip); > > for (;;) { > + loop++; > + > /* > * Lets try to take the lock again - this is needed even if > * we get here for the first time (shortly after failing to > @@ -556,7 +595,8 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, > * other waiters. We only attempt the xchg if the count is > * non-negative in order to avoid unnecessary xchg operations: > */ > - if (atomic_read(&lock->count)>= 0&& > + if ((!need_yield_to_waiter(lock) || loop> 1)&& > + atomic_read(&lock->count)>= 0&& > (atomic_xchg_acquire(&lock->count, -1) == 1)) > I think you need to reset the yield_to_waiter variable here when loop > 1 instead of at the end of the loop. > break; > > @@ -581,6 +621,7 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, > spin_unlock_mutex(&lock->wait_lock, flags); > schedule_preempt_disabled(); > spin_lock_mutex(&lock->wait_lock, flags); > + do_yield_to_waiter(lock, loop); > } > __set_task_state(task, TASK_RUNNING); > > @@ -590,6 +631,10 @@ __mutex_lock_common(struct mutex *lock, long state, unsigned int subclass, > atomic_set(&lock->count, 0); > debug_mutex_free_waiter(&waiter); > > +#ifndef CONFIG_MUTEX_SPIN_ON_OWNER > + lock->yield_to_waiter = false; > +#endif > + Maybe you should do the reset in an inline function instead. > skip_wait: > /* got the lock - cleanup and rejoice! */ > lock_acquired(&lock->dep_map, ip); > @@ -789,10 +834,13 @@ __mutex_lock_interruptible_slowpath(struct mutex *lock); > */ > int __sched mutex_lock_interruptible(struct mutex *lock) > { > - int ret; > + int ret = 1; > > might_sleep(); > - ret = __mutex_fastpath_lock_retval(&lock->count); > + > + if (!need_yield_to_waiter(lock)) > + ret = __mutex_fastpath_lock_retval(&lock->count); > + > if (likely(!ret)) { > mutex_set_owner(lock); > return 0; > @@ -804,10 +852,13 @@ EXPORT_SYMBOL(mutex_lock_interruptible); > > int __sched mutex_lock_killable(struct mutex *lock) > { > - int ret; > + int ret = 1; > > might_sleep(); > - ret = __mutex_fastpath_lock_retval(&lock->count); > + > + if (!need_yield_to_waiter(lock)) > + ret = __mutex_fastpath_lock_retval(&lock->count); > + > if (likely(!ret)) { > mutex_set_owner(lock); > return 0; Cheers, Longman