From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id A7511CCFA0A for ; Thu, 7 Sep 2023 23:00:17 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A13256B0071; Thu, 7 Sep 2023 19:00:16 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 99BF28D0002; Thu, 7 Sep 2023 19:00:16 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 815426B0074; Thu, 7 Sep 2023 19:00:16 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 6A69F6B0071 for ; Thu, 7 Sep 2023 19:00:16 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 33AB114035A for ; Thu, 7 Sep 2023 23:00:16 +0000 (UTC) X-FDA: 81211321632.07.C55EA5E Received: from mail-pl1-f181.google.com (mail-pl1-f181.google.com [209.85.214.181]) by imf14.hostedemail.com (Postfix) with ESMTP id 23979100028 for ; Thu, 7 Sep 2023 23:00:13 +0000 (UTC) Authentication-Results: imf14.hostedemail.com; dkim=pass header.d=fromorbit-com.20230601.gappssmtp.com header.s=20230601 header.b=InFQhnsb; dmarc=pass (policy=quarantine) header.from=fromorbit.com; spf=pass (imf14.hostedemail.com: domain of david@fromorbit.com designates 209.85.214.181 as permitted sender) smtp.mailfrom=david@fromorbit.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1694127614; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=OQjB6U/O9n8tbnlyikl1SjX4EEA6NY7PMDL3dsEt8ag=; b=FdmPAp1LpNPsnyYH4TbA3jh5CwOA5BnRU4JONo6os6XQfcNBxFmCWoJZc/1PIOJepdTI8y vxhXxhCE2j/1pxY/nvlb/jusmKeRBSLK+DX5tgzTpENGXyFvr35FP68eA4L22PU2F1Il+M RQJ2rha2uZ/n1SWTxub63PNJDf6VN7U= ARC-Authentication-Results: i=1; imf14.hostedemail.com; dkim=pass header.d=fromorbit-com.20230601.gappssmtp.com header.s=20230601 header.b=InFQhnsb; dmarc=pass (policy=quarantine) header.from=fromorbit.com; spf=pass (imf14.hostedemail.com: domain of david@fromorbit.com designates 209.85.214.181 as permitted sender) smtp.mailfrom=david@fromorbit.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1694127614; a=rsa-sha256; cv=none; b=c97UpnszCzZqu/zb4qINO36v8NIZdq0cwiIfUI1kCC8EsK0trsRQjSG5SCsJQLfD5KcYjg Fg5or2qyUcFz5NpJpRfByu7UXf2CVEw+YAQeGmxSlE3C6inrN+udCVhGn2Fr/3TxJ3ZjiF K2N+9qsnFvcvxhMgq78cJ9yGQgQlkQY= Received: by mail-pl1-f181.google.com with SMTP id d9443c01a7336-1bc8a2f71eeso12940695ad.0 for ; Thu, 07 Sep 2023 16:00:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fromorbit-com.20230601.gappssmtp.com; s=20230601; t=1694127613; x=1694732413; darn=kvack.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=OQjB6U/O9n8tbnlyikl1SjX4EEA6NY7PMDL3dsEt8ag=; b=InFQhnsbwbi5bhLmC/cGAEMfK/78dn1ZyLuW1YZ0uPcE7ZQeXHA2hmGGXIdmFcU4/b KS6tVgyrxjLzgyzHLZ+1uxhN6e29M87qSVnP9sl63/56IAXkhwPYQdmFkyAWBVytscGC OB1wwSa79sRa/8LAS7Vfh5b4n6ltaMWVBbx3CMGrecNXZYBvtOOhfoWXDCcPCRQzIBJC syAtEGR5E1KYS0arxaurQM7KOQwIw0U5/tHRCiP7zwk0r7d55Afjsk7OdWKu8QZgCL4K xcCJmQG2dV1CS4A3RQu9mOey95TGYZMo/dlfdh275hck8oN1QElTPHWQ8H7H1TqhMmH6 qveA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1694127613; x=1694732413; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=OQjB6U/O9n8tbnlyikl1SjX4EEA6NY7PMDL3dsEt8ag=; b=NWeBO4GUvfTlzofE3xsZjpz0r8XzpGo/rwxDSDz465vzF47/PsFKH/s42C9cF6cJDl FcidCkq8z1kWKIPQuh50Hr94vrnabcs1LNcaAJUjqdbUzFLsDM/K+lRWtd84RDUzMz5r NAJTdheEWBfP1neoI3xmBXRG/PyjWfEnmQCmq7OkYKHXvSu0WvMebU2525B4fxfQkq+u q1tIGfeT3exTmfiTd3UrS0zBpHDJJUZ+7TTyWZnlXMYA+Vr/kToHWz25u0mOSY6XkLdG j+pnAg+eKsISuMHR6FYHUi/pz7+Ht1MlObqc+H3Dlf7MQA590ScHke+hYWge732emk2V fyvQ== X-Gm-Message-State: AOJu0Yy5nPk1E9z0H3QCeh5OnhMLNQMHLsSDpOwJ1KbVFJnbhAjP8844 si4Z3yjpHOrcr1ogn4hr8inU2A== X-Google-Smtp-Source: AGHT+IGsffnzD07F/Mr4xarHC2RsEQpoBGEzHLbPmvs8gys7LfOCUfvBMIv/aJDTbmnyZ0TH/dEdBw== X-Received: by 2002:a17:903:11c9:b0:1b9:e972:134d with SMTP id q9-20020a17090311c900b001b9e972134dmr972287plh.3.1694127612389; Thu, 07 Sep 2023 16:00:12 -0700 (PDT) Received: from dread.disaster.area (pa49-195-66-88.pa.nsw.optusnet.com.au. [49.195.66.88]) by smtp.gmail.com with ESMTPSA id l5-20020a170902f68500b001bc0f974117sm265116plg.57.2023.09.07.16.00.11 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 07 Sep 2023 16:00:11 -0700 (PDT) Received: from dave by dread.disaster.area with local (Exim 4.96) (envelope-from ) id 1qeNyq-00CDqz-38; Fri, 08 Sep 2023 09:00:08 +1000 Date: Fri, 8 Sep 2023 09:00:08 +1000 From: Dave Chinner To: Peter Zijlstra Cc: Matthew Wilcox , Ingo Molnar , Will Deacon , Waiman Long , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Chandan Babu R , "Darrick J . Wong" , linux-xfs@vger.kernel.org Subject: Re: [PATCH 1/5] locking: Add rwsem_is_write_locked() Message-ID: References: <20230907174705.2976191-1-willy@infradead.org> <20230907174705.2976191-2-willy@infradead.org> <20230907190810.GA14243@noisy.programming.kicks-ass.net> <20230907193838.GB14243@noisy.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230907193838.GB14243@noisy.programming.kicks-ass.net> X-Rspamd-Queue-Id: 23979100028 X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: 5hcik44k7tj5akfpsc3tdbyb53dyawnn X-HE-Tag: 1694127613-723346 X-HE-Meta: U2FsdGVkX19WG0kP5B+MQyfOzqC+LsZQjw9jgRqpoSnjnOUO/8e/e/jm8BbKJVlRJBe1dRaDyIkOI+UsQ94b1eHts42vLsYjuEMIxKZ/Q+SzUAby/G7CMgJKYe+miOTibfV2yT1hsm+RWgiA7zRgzl/ufI+xitqqpkg4V36fDfFALw5iCEOe31h8j52iKNJMYezJYvkrfbA5fMv30d36aMgs6AP3A4I4paLA780pYxNmibhmUelp1r0W9MeRQ63D3t2uUV/UNAnNYrgtlXYxgLE/+ksqHUQYwVwJWRzaH6gkN8/jwahlj1+unO0zVa3k05HzDMs10Tor2R/n6+NLCTVKa2gmqX2+J2Q1SdlU57vG34WMLFzmw121NHvPjo+kPD/facAd5OL3I382psZp4SfB5eXpRFimSZMAAVY227L7m9ABfjt9WV3IIdj+8UTGBqSksWlLQ5+x06077/mwwIDN6l/EEbGVf15NOyMUxyhaJFTxh0gOWH1UR1WyYiI2e7y31N62skaIsV/x11f2cqdkiKhUyWKgh6zJSQOk28ZswdmB7cAHO+FeSQVXkXWRoKjmwmtklTW0nWciVooSznayvNgt0Iq8zB4azmKe7aE3ubQuVFntmWy1ikoHhpkH+bM4emu6mehlVkKn2XFw3/qywsWIdDT3K3zRtcx3BssCBa3tKEbCD/TeEdWvvS4NzdB/a8pbkRMqwmS29loCw5pHnbYcZBgMUj2/2KfkXNwL0urDUkQrvnjV930ZOC6+SSnmiJp0FpnOx1VPrxB4aOwauzlXbrUbef4F6S9R5D4n4mSYEymMeBBdPDKAdvsjF6oHqE7HlRjRLQR+FAQxGhSEIEF5eV2hAz6yQoGOykVnKkGX87aV56Yl/Xb8lSO+2E3WH1a5QL3MEnUd8umoEAREvCy3yx+gLTd+z9i+DOaSeQvdj1TAdvp2x1VUvOf1iD5kvD5NBNWp265OuBy ExWlz33A JVh+QSqfLNb4bO0XdWJ07LaZV32gW9/qHqZcSwF2m6lAGhHkorJCVJNYAFIJVFatwapS5YHWCxZR2Dum0jzeaNfGLKoxmZAJ8UAHRdG+m8yoVr3hz50HDNtm2jZTqgRlVGe9j5F/19DjlwfvU80Yi2J+WbALfWXWyo90ca5f3zIR8waKfNz/Z7ffHO8tLJvtMXzWfBm0A0i7+4HAifLqlq1wiXhUcMvJeGz3Nn6yNNULqAq78mx/pJMG2DUokHsMOx1veTf8nl/sZpYVIKJ+VyM+q8/E2KrRPArrXcZSsrVz0cMe9ZFXVeQNZ12z0E+NqqnW0308IFNYDVlgl6rUsoldNLLiux4DnvNvDXKKkajio+FJ92fwrMwGvOnZEuH74FMj/n1fUiX5q12HWhLluGwxc7CwB/SAhL+5zTWQ90lwASqQ76pTABg+Lf3quneOJ6wXcwM29vb8Z3abzCeeTQZ4tV6jjipM2hlrWgbPEbquYY83jMwUWZ63mWQ== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu, Sep 07, 2023 at 09:38:38PM +0200, Peter Zijlstra wrote: > On Thu, Sep 07, 2023 at 08:20:30PM +0100, Matthew Wilcox wrote: > > On Thu, Sep 07, 2023 at 09:08:10PM +0200, Peter Zijlstra wrote: > > > On Thu, Sep 07, 2023 at 06:47:01PM +0100, Matthew Wilcox (Oracle) wrote: > > > > Several places want to know whether the lock is held by a writer, instead > > > > of just whether it's held. We can implement this for both normal and > > > > rt rwsems. RWSEM_WRITER_LOCKED is declared in rwsem.c and exposing > > > > it outside that file might tempt other people to use it, so just use > > > > a comment to note that's what the 1 means, and help anybody find it if > > > > they're looking to change the implementation. > > > > > > I'm presuming this is deep in a callchain where they know they hold the > > > lock, but they lost in what capacity? > > > > No, it's just assertions. You can see that in patch 3 where it's > > used in functions called things like "xfs_islocked". > > Right, but if you're not the lock owner, your answer to the question is > a dice-roll, it might be locked, it might not be. Except that the person writing the code knows the call chain that leads up to that code, and so they have a pretty good idea whether the object should be locked or not. If we are running that code, and the object is locked, then it's pretty much guaranteed that the owner of the lock is code that executed the check, because otherwise we have a *major lock implementation bug*. i.e. if we get to a place where rwsem_is_write_locked() fires because some other task holds the lock, it almost always means we have *two* tasks holding the lock exclusively. Yes, it's these non-lockdep checks in XFS that have found rwsem implementation bugs in the past. We've had them fire when the lock was write locked when we know a few lines earlier it was taken as a read lock, or marked write locked when they should have been unlocked, etc because the rwsem code failed to enforce rw exclusion semantics correctly. So, really, these lock checks should be considered in the context of the code that is running them and what such a "false detection" would actually mean. In my experience, a false detection like you talk about above means "rwsems are broken", not that there is a problem with the code using the rwsems or the rwsem state check. > > > In general I strongly dislike the whole _is_locked family, because it > > > gives very poorly defined semantics if used by anybody but the owner. > > > > > > If these new functions are indeed to be used only by lock holders to > > > determine what kind of lock they hold, could we please put: > > > > > > lockdep_assert_held() > > > > > > in them? > > > > Patch 2 shows it in use in the MM code. We already have a > > lockdep_assert_held_write(), but most people don't enable lockdep, so > > Most devs should run with lockdep on when writing new code, and I know > the sanitizer robots run with lockdep on. > > In general there seems to be a ton of lockdep on coverage. *cough* Bit locks, semaphores, and all sorts of other constructs for IO serialisation (like inode_dio_wait()) have no lockdep coverage at all. IOWs, large chunks of many filesystems, the VFS and the VM have little to no lockdep coverage at all. > > we also have VM_BUG_ON_MM(!rwsem_is_write_locked(&mm->mmap_lock), mm) > > to give us a good assertion when lockdep is disabled. > > Is that really worth it still? I mean, much of these assertions pre-date > lockdep. And we're trying to propagate them because lockdep isn't a viable option for day to day testing of filesystems because of it's overhead vs how infrequently it finds new problems. > > XFS has a problem with using lockdep in general, which is that a worker > > thread can be spawned and use the fact that the spawner is holding the > > lock. There's no mechanism for the worker thread to ask "Does struct > > task_struct *p hold the lock?". > > Will be somewhat tricky to make happen -- but might be doable. It is > however an interface that is *very* hard to use correctly. Basically I > think you want to also assert that your target task 'p' is blocked, > right? > > That is: assert @p is blocked and holds @lock. That addresses the immediate symptom; it doesn't address the large problem with lockdep and needing non-owner rwsem semantics. i.e. synchronous task based locking models don't work for asynchronous multi-stage pipeline processing engines like XFS. The lock protects the data object and follows the data object through the processing pipeline, whilst the original submitter moves on to the next operation to processes without blocking. This is the non-blocking, async processing model that io_uring development is pushing filesystems towards, so assuming that we only hand a lock to a single worker task and then wait for it complete (i.e. synchronous operation) flies in the face of current development directions... -Dave. -- Dave Chinner david@fromorbit.com