From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751371AbcELUPp (ORCPT ); Thu, 12 May 2016 16:15:45 -0400 Received: from mail-by2on0130.outbound.protection.outlook.com ([207.46.100.130]:45184 "EHLO na01-by2-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750956AbcELUPn (ORCPT ); Thu, 12 May 2016 16:15:43 -0400 Authentication-Results: hurleysoftware.com; dkim=none (message not signed) header.d=none;hurleysoftware.com; dmarc=none action=none header.from=hpe.com; Message-ID: <5734E45A.4030904@hpe.com> Date: Thu, 12 May 2016 16:15:22 -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 Hurley CC: Peter Zijlstra , Ingo Molnar , , Davidlohr Bueso , Jason Low , Dave Chinner , Scott J Norton , Douglas Hatch Subject: Re: [PATCH v2] locking/rwsem: Add reader-owned state to the owner field References: <1462580424-40333-1-git-send-email-Waiman.Long@hpe.com> <5733AC64.6020306@hurleysoftware.com> In-Reply-To: <5733AC64.6020306@hurleysoftware.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [72.71.243.221] X-ClientProxiedBy: CY1PR12CA0064.namprd12.prod.outlook.com (10.163.230.32) To DF4PR84MB0314.NAMPRD84.PROD.OUTLOOK.COM (10.162.193.28) X-MS-Office365-Filtering-Correlation-Id: 479bf50c-6b92-4419-2583-08d37aa2300a X-Microsoft-Exchange-Diagnostics: 1;DF4PR84MB0314;2:ZPUhzFxtzDys0k2wTqYb6NQKZhna8G7jfaPcAMgujonHI3tTy9CoaEfStp6i7DHJpvhF7HxulQuWyBZ5lv6bo6L055uoWLy0gm+H9fXAUnsC+rwmytMxTCxCR/oPALH1KBhCtAXAhsVN9hqibpAYFaniC2YwNUmU6n+9rFttF/r850WnFpS0IKg4k3GZIn0g;3:TqzRdlhSUnrfCU6gWJCsO0YdfQs/TBpC41fLlgnCaZyVojNEhiSp5GDYKJ5KVVhsYRYnMqtgUwe/fLrqgnKf2sbt7x5XZx35AdEPFIW5gI4HoiBDQdANI3Kpi9ZXXEk3;25:1YfLeVFEP9FeidqOqo76/G3aCG7hGkMZSDFZd+u7fsuROyltorvLtG0nWKxYrLcxA/8Puozzqc0yP281EkLldcIlFLmJaYFLQopIz+52qSHwDBZs0jT90bGQubRqGOgfK0kUNa/wk1vK+ZbxH7imSBIbDlY0R6KyCEdsPteNF3eqGAE8DgKVj30MIOxf66WTcYAm1tED2WiJPuxdEHi6djoCzfiXIM7+qk2+pcbR7YHyok3vc0Zg8PSz9TK1OFmu2nsE9UaAIDpciP0AzNnBKjgGBAnEA7PVYrFyVkWeMT/rA/cqPjIxmckFJyq4od+Nbpwkbw34L2u/kDZpFiStbz6mn5++jUAemWDJxoapcA1elROv4eoDV8RJZ1CfYFC7dfXoFYzOgh1wpsO2TXWYR4ji0NxClGPaQsW4uKoPSWs= X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:DF4PR84MB0314; X-LD-Processed: 105b2061-b669-4b31-92ac-24d304d195dc,ExtAddr X-Microsoft-Exchange-Diagnostics: 1;DF4PR84MB0314;20:WVoBu85Ae1HLPC5edPTZHyyyU9pE/nhyA4t49jfgwQgs4arxBs8hzfNgkCGmk56aQXQfjwXW2pgyMVmmS3wUPSyfBsH4W+eYE19etR9cgUzod49ZFRodbkJuexQ4O3U09Wd/06on9wscl6ujM1Quc9Lv1hZ1SJ1tDbw06YomSegXj74rbJYRHzaLvRQBwTPw9tEWq4AyRIrnF2Pu2EEqlMpoRxIZ8MuwSV2/jPykPBWAfm5YJsn5NPfvJqVO6R3uz2xt3DfW/B/OP46CNWTWKJN8BysjorC3pE1sjbFlEEAbPqoq6v8W/aZC8pZEKE+b6ql1l1geoiazZS29UjLbfg==;4:UbjPPatB87M5x3zoAKXN/WIQuP8WdlDWh/RNhGNJLef+YXq4DciZJsVGqOGmCj9DLF6yBRylbSYD3aVp1CmyKP0Q+hM4WkVHb2oYDJi1OLvBmsuU1royObv3t0/TtfFPCEC0wpNgk8POk/vzdZ+WXx2MnsrwTKSaKPidsOLNEEEsGcopkpkT+C0RlNKASnuyzw2esw1fOtSFUh+Liu7EUKsT30Qqo+5XUUaCWcFD4++EG/fTN74ksu9DnEBZVTWF2oogGQm5NobkCxEwTUhG2xhikB9xVN+Mx1JqDfyMpD+UAUBEHw6b4Z1k9lTCo36qscRTLJp6zRCN0nwZWX3dWJTrofhk6BPmlSUvZiU2NU21wZWgfgm1vXPvNPuflDXS X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(8121501046)(5005006)(3002001)(10201501046);SRVR:DF4PR84MB0314;BCL:0;PCL:0;RULEID:;SRVR:DF4PR84MB0314; X-Forefront-PRVS: 0940A19703 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6009001)(6049001)(24454002)(377454003)(59896002)(42186005)(65956001)(66066001)(65806001)(47776003)(5004730100002)(50466002)(36756003)(83506001)(81166006)(23676002)(117156001)(64126003)(2950100001)(65816999)(110136002)(92566002)(230700001)(33656002)(4326007)(2906002)(6116002)(3846002)(586003)(4001350100001)(189998001)(54356999)(86362001)(50986999)(76176999)(5008740100001)(87266999)(77096005)(7059030);DIR:OUT;SFP:1102;SCL:1;SRVR:DF4PR84MB0314;H:[192.168.142.129];FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtERjRQUjg0TUIwMzE0OzIzOjVtTWJ1aG9mUkw5SU9VWXVmaTV0Z0VBM2hw?= =?utf-8?B?Y1Zob2NLVWFCenoxMDRDbVZOMm0xMFZhNlQxdnpxc2M4VEdmODJSTzZHMUty?= =?utf-8?B?NW9STkt4NUk5SSt0TzVsT1hOUWVZTC9aa0xWWHUzMVFUMExxQ21nTlFkQVFv?= =?utf-8?B?a1BMUFVNZlRQbXZ0Snk0T05IdTRNKzg2ckpFTGFUdVQveXhwU2hjRlRUakY2?= =?utf-8?B?OEdRS0Rxd3lRK1RjTi9oV1JMZ0hpajYzdW85SmMwSGdNeStudTBCblprVWlU?= =?utf-8?B?Qm5yYmFGMGNOZkM0cnhLTGF1U0NJKytOQVlzdnZPcjVjcTdFUEJPN25SS2Z5?= =?utf-8?B?Y1M4R0RBcjRFWS9jUTY1MU5VaFdaUDFPM1RYR3ovaGcyc1haRkd3dFFkcWRF?= =?utf-8?B?QlhMSzBKaW04R1RkbUdHQU5LR09BWjlndW11bUt5UUtVOW1hVkdVTGdSeldy?= =?utf-8?B?eGpBL1VwRlkzbElBeUgvTUxOcVFYTzR0c0hxU1NCS1ZNZ2dEQU1SU3FyQWdU?= =?utf-8?B?b0xucmF4SldxNmgyZE91ZmRYaS9KbEFYQmljOW85TXhMNG1nRTQ3ZndOd3Ax?= =?utf-8?B?VFk4OCtBdTdicUtQNExKaWkwQWRCbWNaejZQdFF3NlV4N2tkSVJLNlJXdzFQ?= =?utf-8?B?SWNiN0MvRUZJVWdMcGw3akRhSHZDZDNvUFVFdDhjUkxja1BKKzFBd1hVU2Nr?= =?utf-8?B?ZTZ1VmZSSEU0L0l6bm1iUlhMNlVXRzk3WEJ1Z1FkdnZ3aVhRUTFTODJDdHUr?= =?utf-8?B?M00ybmlIT3AxZTJZa3RnZC95REtrM05wVG5qK3FoK0diaGtuYmlZYUF4RDBV?= =?utf-8?B?YmZ1dFVVb05GUVVEdUxVMTdWT1RIUzdDUWQ0SXR0YnEzR2x5dEYzRjB3SVdh?= =?utf-8?B?UlMzZ0NjWkpoalJDbnZqQ21SZDhzR1h2V3A4dFU0STZIYnF4bXA1SkRucmMv?= =?utf-8?B?TWNFeS8rWEhNOS9kMUVDbUlVekd0RFhRbS9hQ1FkYTBoNC9FbENLMkF2ZVZo?= =?utf-8?B?cm5yaWZ4OEMwa2pDRk9DT0htTnEvZm90RFVRR2FrZmZPa0x5elRTcEU3Q0Jt?= =?utf-8?B?dmYzVVNrckgzTlRkSi9oVXhzWFloMHJnVTNNMkp3YWc3dUVTU2tHb1NyZEor?= =?utf-8?B?UlVvVzB3NWU2V3ROVlQyUUU1aHpieU4wenNTRlhPNVVabjMrWlJuc2VUOXc4?= =?utf-8?B?SGRyRGNxc2Rzb1JNMXpQbDZkbFVyQ0hZUzJPYUJXdTJrcU0xMXFQSWJIVEsx?= =?utf-8?B?NkhKTmVleXdza1RzeEFMTFN4MDhmQ29wQ1JnUmVJNFV1eVJIQ1ZjYWNDWTJC?= =?utf-8?B?NHlCNlloa1ZWU1ZhaDFkMUVVSWsxYnhadytDZXI1eG9iaGZUalNJdjkrUXVK?= =?utf-8?B?aFI0Vnpsd0FRYTd2R0E2U2RkUTFWelBrQ2ZNZStTc1dJSmpPR2hhYWcwUitC?= =?utf-8?Q?wxJ0z8=3D?= X-Microsoft-Exchange-Diagnostics: 1;DF4PR84MB0314;5:jXLRqC2dehcrPJDBYghuY5iHsjw8ZnOjd1gWgKqtQpOg7Llmy6fkS7LRM/fkVYRe8fvItqmrnzMyoumxYObWjXXJKeLmMbV6FLSOOfftoGDgderepRKpp2x/c97y63gcQK5BZ9sF9u37zQn9y44r9Q==;24:JldlHB6/hdrxp9eXBmfd0i2Y4jwIhLB2u8vlK7xUccZrVt+wocLWaBy2SYCgyrUnCjsZNDlNPnzsq2UYHgK8z/2F3pHLUNg2Fg6Htg7fV0g=;7:ct6iHUZt+hlknWKDGMYn3MyYkY1eZ4IzN1Sd3EVPrEP1uk1g4/98+IviBF9c+djNz165muhlyqY7OEKsUqWlx+AODa0ajLxFVfGRuU0QVd2lBt+Yj8t5iQ37GfBKJ8IU1c2SxNNyZPkvcBwfN1VtOMg9qMEeDhbXlzZRCa85/qxzZa/KKXb2PVImLYF/AFvp SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: hpe.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 May 2016 20:15:34.7069 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: DF4PR84MB0314 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/11/2016 06:04 PM, Peter Hurley wrote: > /* Grant an infinite number of read locks to the readers at the front > @@ -306,16 +312,11 @@ static inline bool rwsem_can_spin_on_owner(struct rw_semaphore *sem) > > rcu_read_lock(); > owner = READ_ONCE(sem->owner); > - if (!owner) { > - long count = READ_ONCE(sem->count); > + if (!rwsem_is_writer_owned(owner)) { > /* > - * If sem->owner is not set, yet we have just recently entered the > - * slowpath with the lock being active, then there is a possibility > - * reader(s) may have the lock. To be safe, bail spinning in these > - * situations. > + * Don't spin if the rwsem is readers owned. > */ > - if (count& RWSEM_ACTIVE_MASK) > - ret = false; > + ret = !rwsem_is_reader_owned(owner); > goto done; > } > I'm not a big fan of all the helpers; istm like they're obfuscating the more > important requirements of rwsem. For example, this reduces to > > rcu_read_lock(); > owner = READ_ONCE(sem->owner); > ret = !owner || (rwsem_is_writer_owned(owner)&& owner->on_cpu); > rcu_read_unlock(); > return ret; > Using helper functions usually make the code easier to read. This is helpful for the rwsem code which can be hard to understand especially how the different count values interact. > >> @@ -328,8 +329,6 @@ done: >> static noinline >> bool rwsem_spin_on_owner(struct rw_semaphore *sem, struct task_struct *owner) >> { >> - long count; >> - >> rcu_read_lock(); >> while (sem->owner == owner) { >> /* >> @@ -350,16 +349,11 @@ bool rwsem_spin_on_owner(struct rw_semaphore *sem, struct task_struct *owner) >> } >> rcu_read_unlock(); >> >> - if (READ_ONCE(sem->owner)) >> - return true; /* new owner, continue spinning */ >> - >> /* >> - * When the owner is not set, the lock could be free or >> - * held by readers. Check the counter to verify the >> - * state. >> + * If there is a new owner or the owner is not set, we continue >> + * spinning. >> */ >> - count = READ_ONCE(sem->count); >> - return (count == 0 || count == RWSEM_WAITING_BIAS); >> + return !rwsem_is_reader_owned(READ_ONCE(sem->owner)); > It doesn't make sense to force reload sem->owner here; if sem->owner > is not being reloaded then the loop above will execute forever. I agree that we don't actually need to use READ_ONCE() here for sem->owner as the barrier() call will force the reloading. It is more like a habit to use it for public variable or we will have to think a bit harder to make sure that we are doing the right thing. > Arguably, this check should be bumped out to the optimistic spin and > reload/check the owner there? > > Or better yet; don't pass the owner in as a parameter at all, but > instead snapshot the owner and check its ownership on entry. That will make the main optimistic spinning loop more complex. > > Because see below... > >> } >> >> static bool rwsem_optimistic_spin(struct rw_semaphore *sem) >> @@ -378,7 +372,8 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem) >> >> while (true) { >> owner = READ_ONCE(sem->owner); >> - if (owner&& !rwsem_spin_on_owner(sem, owner)) >> + if (rwsem_is_writer_owned(owner)&& >> + !rwsem_spin_on_owner(sem, owner)) >> break; >> >> /* wait_lock will be acquired if write_lock is obtained */ >> @@ -391,9 +386,11 @@ static bool rwsem_optimistic_spin(struct rw_semaphore *sem) >> * When there's no owner, we might have preempted between the >> * owner acquiring the lock and setting the owner field. If >> * we're an RT task that will live-lock because we won't let >> - * the owner complete. >> + * the owner complete. We also quit if the lock is owned by >> + * readers. >> */ >> - if (!owner&& (need_resched() || rt_task(current))) >> + if (rwsem_is_reader_owned(owner) || >> + (!owner&& (need_resched() || rt_task(current)))) > This is using the stale owner value that was cached before spinning on the owner; > That can't be right. The code is going to loop back and reload the new owner value anyway. It is just a bit of additional latency. I will move the is_reader check up after loading sem->owner to clear any confusion. Cheers, Longman