From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753475AbcEJCYa (ORCPT ); Mon, 9 May 2016 22:24:30 -0400 Received: from mail-bn1bn0101.outbound.protection.outlook.com ([157.56.110.101]:50640 "EHLO na01-bn1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750982AbcEJCY3 (ORCPT ); Mon, 9 May 2016 22:24:29 -0400 Authentication-Results: infradead.org; dkim=none (message not signed) header.d=none;infradead.org; dmarc=none action=none header.from=hpe.com; Message-ID: <57314650.5050206@hpe.com> Date: Mon, 9 May 2016 22:24:16 -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 , , 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> <20160509082741.GF3430@twins.programming.kicks-ass.net> In-Reply-To: <20160509082741.GF3430@twins.programming.kicks-ass.net> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [63.156.242.50] X-ClientProxiedBy: BY1PR15CA0006.namprd15.prod.outlook.com (10.162.17.144) To CS1PR84MB0312.NAMPRD84.PROD.OUTLOOK.COM (10.162.190.30) X-MS-Office365-Filtering-Correlation-Id: 87be9ea6-47db-46b3-49b8-08d3787a355b X-Microsoft-Exchange-Diagnostics: 1;CS1PR84MB0312;2:vtsrwErCjAZ9l/y05EnTQ9LFIizFiyHJa5hcTM+fJKF7bbZGiA77m+F/d2r6go/pNC2EHtIyAOmu1R1UgRzBB9r2FlLdPoe3FmewDkStd2BUd41BPt2nuO5+Dtumn1wI/DLezXcTpHYvA5Wt+m/XVr1jv83XOGbZrrRjI0LymmyiAIh8fsHPk8g5JkkVvJKb;3:DVktQSuunRFb0U1WXR69LgqPP4dzU4uRDIXFVMsPtTHz9chaTkBqxq16x2rrHyrYwg1XnJPPF8ULHUxMQWcEJS/i05ST28T59aGNKK4EZAf62z2koRP3WF4nWJftpMlH;25:w+JWc857lFEKl1jvj9YjXLNq7jAhrj635fQLrHYkQ/VFTf31r1U3HAUQLYdyVmKerX9nDK21TfWy1YM8Z+WbTp3agx+idENua1wkTZKadhwWQ00NO015btjH/1eYgF0ztK6FHcpq7ipvpllhjKpf8ZdeuZE0aHRT8fTWi9hBtgpdNCHlr6btdTMBbgmQb2bPdyLdYxEkxDOW4oFm6e7kFsswhCauz6wU/PERZrLZzKxcYRj0hktY7180tpCu8CfOgNslH5jO6KzZEft6noc7Ql56YB/5xih2gLrxKw4IZUWcZpL18MkkHM7PHsbsH+RaA5ilk2iUGWgo6cfGYKNTM+gY3JKjZgKwONT87hM2W40cELp0siRJUCR4TuEUI9rs+8AcctMX77JlhxIY2J2QEbGhAH0PeLY3MPseJYM2xeJyhppl++gstKMnavQc6PdW X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:CS1PR84MB0312; X-LD-Processed: 105b2061-b669-4b31-92ac-24d304d195dc,ExtAddr X-Microsoft-Exchange-Diagnostics: 1;CS1PR84MB0312;20:PdskYvy/+Upgs0lyFvxflMR3DHzGIeQIlGdx5PC1h+eUJ7vQkJMtRu2KJNKtnumBd6icKAAkx/1CqUwLM01zOpiC5qpi5Er2yT+pqZYW3KFmh7zwQzHESGIxJ7DjbyDmZc3YjZCiFVxf8WoB3AjhbWxhLuD+chEMJLdhTp7wYprp6W1qc4eENuRTw/rLIjr7v3Hm7YZF/GlhptEsCsBXG3M5uD//aFnZNzkGaZpNWb6O0ocQR9rlBo9V81+oFZhmpeV5umsY+hl4TG/9fg28rUdrPPyI6FDmAyC0tIHPsQevil0hbGWcAtat2GpLGKidlt7PzSaRVQtIUBEuSb2ToQ==;4:+BxxAdojvGuNrhTNECMm1MnoF5vfOL9k1/b36SCxm5F6eF24nq8H11YRGc5FTjNdTreatPndv1trkfT90RodjMwD001DoawLEa4e8E0hAX+kVp1ZDBZEssPbfGiy+/aahniB9dzPP26nCEqMZDyEx5rbga6Gh4zBcu5LuKNs457e0geVEBGzkYwt0BBasDps6QGDTAKcK0o2kTxy0tyf8lpxc7A5mKasfo0aECEivnwIn36FL0pc5yeozroqwEK9Zc+65sgajDMjWOWFUGaheKEp/MZ8RP7DE0DELfVQivoPEDPYze1a1u4MZTHsVn7bXW9nSRt1+nz1CgK55aY04zKLFND8+7+K97uhbhcSFRRsknI1NEhGmsUyhfuVndnN X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(601004)(2401047)(5005006)(8121501046)(3002001)(10201501046);SRVR:CS1PR84MB0312;BCL:0;PCL:0;RULEID:;SRVR:CS1PR84MB0312; X-Forefront-PRVS: 0938781D02 X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6049001)(6009001)(377454003)(24454002)(4326007)(99136001)(76176999)(50986999)(65816999)(54356999)(87266999)(92566002)(83506001)(2906002)(42186005)(36756003)(5008740100001)(65806001)(117156001)(47776003)(65956001)(66066001)(230700001)(586003)(6116002)(3846002)(117636001)(2950100001)(122286003)(4001350100001)(77096005)(189998001)(5004730100002)(86362001)(81166005)(64126003)(110136002)(23756003)(50466002)(7059030)(62816006);DIR:OUT;SFP:1102;SCL:1;SRVR:CS1PR84MB0312;H:[192.168.142.128];FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?iso-8859-1?Q?1;CS1PR84MB0312;23:hdbwVcDKncZ1ENXP9WbD/BnPBWj5f0Vul7By0rM?= =?iso-8859-1?Q?mgO4vRzHiymCytDsaMckEIxpKkDbLMpvTDCjYj5sS5TXiuBCJylJpQlaEh?= =?iso-8859-1?Q?bc1nsPB70TDiJJZrXvZ8I1NhNl22Ijbs1leboF7b/URbJ3D9p7TkSxsbWn?= =?iso-8859-1?Q?VenB/LPXSlgysu3eghvAZHyqLOjR71COuiGVqBq7EH82V7k2cUJiEa8MLa?= =?iso-8859-1?Q?qacPbA03u8Yxf1sBqmbIv7qRHtce45lysXazwwhpFPUy7IKp524FxJnl65?= =?iso-8859-1?Q?NltY/bHEbF3d61PlpcDMaLVOBmkdwM106uK6s3qGEUr4/5ufgbDFoh8/wy?= =?iso-8859-1?Q?lt9rstKOSS/Or1VGBq4e5VxIohM4jiY9B+IrSDE2EAFP1NA5vyu5qDgfry?= =?iso-8859-1?Q?JxKDFDpQ7wdlGcLQ/Z/xPfW+obvsLnzEnHJeTuhL1o2HL8TeGQKIVG0x0w?= =?iso-8859-1?Q?D/7342rvG3wnCYBGEp8P1XGtLMfBbihuGxBLqgLn6iSrn53pJjUs5yrVCy?= =?iso-8859-1?Q?6MSjqxS6T9sKcnj3Uf/Z2THaz+eVqBfM+RG+fxcYmWtPoERD+a7jHPJ3Yy?= =?iso-8859-1?Q?HMxEwk63L28FqU7gWtqFfQZRtKIykPWRQDDNuToxwBA4JUmTt6jrN5E90I?= =?iso-8859-1?Q?RI96oZGfXWT6S/1HCYnV6RElDB1lUHkfH6An8jJW62j2c4Mvw6X5I8tkeJ?= =?iso-8859-1?Q?GzXeMhT46W/98YKPPYgGL/xuO1UTLoh3AoVyLVlXDs9EP6w+PlA4lGVu+1?= =?iso-8859-1?Q?HO2vuUIelIZqWHLZ1Y10XBGYfeQHPnD6bfr7Dqp8ErpyJ/OLgc9G32XuWo?= =?iso-8859-1?Q?i032DsNrwiY5f2i2H+Gd517SKZdua6P6ESkNkW/ooHK/egsgwmT1dGhFhx?= =?iso-8859-1?Q?XATIsvZQMzWqC5hor8YF1M/Rw2jOlgR+gIXJcCoRt3oc0B/zcw4Zzp1Os5?= =?iso-8859-1?Q?TNU9sXjqxgVExSr9XQJKUw1Jc6jrbvun7ewzoIjfuwWOcsK1r1CR21RBzX?= =?iso-8859-1?Q?+x3uVhh1G+MFji/0PSq6/Gcjb2XrI6HqHSLPoD8izUzynBB2JsRpLkHVy/?= =?iso-8859-1?Q?R2meb2h8HYE/UjOcqWmqF0vdK4KNf5ITu8w4nBf6ERv5zbJkF+tY/lGGjA?= =?iso-8859-1?Q?y26eFqR9w+CLuC5c/PRgPdzzdXg=3D=3D?= X-Microsoft-Exchange-Diagnostics: 1;CS1PR84MB0312;5:W3u6jkDxKahHMZfYSWdn4tHReGDe3xblhHKS0aDR99Si9YACeLmzqTefOCHZxfWDrzRMg/0Q82yiBaqjh21QlrXYAZez+YeUkk3iZHxRgYOzXcka0QpEiXqfwtVs+qBsc+KHLfMNgT3NNPX/FZ+UFw==;24:J3zP8xgoSzdsXPy4MZnLa6KE9ubgbaPS/Bhz8LP8KMJlJ9amCYIteL89Mwo4yJaWopPcu43MG+08Ke+Hxpf/+NtaoNcDyk44WXwYBNPsMDk=;7:ik6VCKBLyA4F0STRR3gUm2k4H7yXdloIUXB1X7XHjSSbty/P3UGdh6D4jR8pvvCahkC2vQV7i8Ydpr4CL6GJYw65xTt/sSONoD4+imiCIVXuHxIlEqU9CpxC9dOxzjbGK440qfNA7wyNGhipEZKAkSWwl0m1UISqNqWBXBtKZlFPTaSfBBeiw2yxFt/s/C+W SpamDiagnosticOutput: 1:23 SpamDiagnosticMetadata: NSPM X-OriginatorOrg: hpe.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 May 2016 02:24:25.5421 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: CS1PR84MB0312 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/09/2016 04:27 AM, Peter Zijlstra wrote: > On Fri, May 06, 2016 at 08:20:24PM -0400, Waiman Long wrote: >> @@ -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. We also quit if the lock is owned by >> + * readers. > Maybe also note why we quit on readers. Sure. Will do so. >> */ >> + if (rwsem_is_reader_owned(owner) || >> + (!owner&& (need_resched() || rt_task(current)))) >> break; >> >> /* > >> diff --git a/kernel/locking/rwsem.h b/kernel/locking/rwsem.h >> index 870ed9a..d7fea18 100644 >> --- a/kernel/locking/rwsem.h >> +++ b/kernel/locking/rwsem.h >> @@ -1,3 +1,20 @@ >> +/* >> + * The owner field of the rw_semaphore structure will be set to >> + * RWSEM_READ_OWNED when a reader grabs the lock. A writer will clear >> + * the owner field when it unlocks. A reader, on the other hand, will >> + * not touch the owner field when it unlocks. >> + * >> + * In essence, the owner field now has the following 3 states: >> + * 1) 0 >> + * - lock is free or the owner hasn't set the field yet >> + * 2) RWSEM_READER_OWNED >> + * - lock is currently or previously owned by readers (lock is free >> + * or not set by owner yet) >> + * 3) Other non-zero value >> + * - a writer owns the lock >> + */ >> +#define RWSEM_READER_OWNED 1UL > #define RWSEM_READER_OWNED ((struct task_struct *)1UL) Will make the change. >> + >> #ifdef CONFIG_RWSEM_SPIN_ON_OWNER >> static inline void rwsem_set_owner(struct rw_semaphore *sem) >> { >> @@ -9,6 +26,26 @@ static inline void rwsem_clear_owner(struct rw_semaphore *sem) >> sem->owner = NULL; >> } >> >> +static inline void rwsem_set_reader_owned(struct rw_semaphore *sem) >> +{ >> + /* >> + * We check the owner value first to make sure that we will only >> + * do a write to the rwsem cacheline when it is really necessary >> + * to minimize cacheline contention. >> + */ >> + if (sem->owner != (struct task_struct *)RWSEM_READER_OWNED) >> + sem->owner = (struct task_struct *)RWSEM_READER_OWNED; > How much if anything did this optimization matter? I hadn't run any performance test to verify the effective of this change. For a reader-heavy rwsem, this change should be able to save quite a lot of needless write to the rwsem cacheline. >> +} >> + >> +static inline bool rwsem_is_writer_owned(struct task_struct *owner) >> +{ >> + return (unsigned long)owner> RWSEM_READER_OWNED; >> +} > Tad too clever that; what does GCC generate if you write the obvious: > > return owner&& owner != RWSEM_READER_OWNER; You are right. GCC is intelligent enough to make the necessary optimization. I will revert it to this form which is more obvious. >> + >> +static inline bool rwsem_is_reader_owned(struct task_struct *owner) >> +{ >> + return owner == (struct task_struct *)RWSEM_READER_OWNED; >> +} > So I don't particularly like these names; they read like they take a > rwsem as argument, but they don't. > > Would something like: rwsem_owner_is_{reader,writer}() make more sense? Yes, these names look good to me. Cheers, Longman