Received-SPF: pass (google.com: domain of linux-kernel+bounces-43566-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 29 Jan 2024 14:23:17 -0800
From: Dan Williams <dan.j.williams@intel.com>
To: "Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com>,
	<linux-kernel@vger.kernel.org>
CC: "Fabio M. De Francesco" <fabio.maria.de.francesco@linux.intel.com>, "Peter
 Zijlstra" <peterz@infradead.org>, Dan Williams <dan.j.williams@intel.com>,
	"Ira Weiny" <ira.weiny@intel.com>
Subject: Re: [RFC PATCH] cleanup: Add cond_guard() to conditional guards
Message-ID: <65b82555aaf86_5902829447@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <20240129190100.329791-1-fabio.maria.de.francesco@linux.intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240129190100.329791-1-fabio.maria.de.francesco@linux.intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?3exZEsUqxhUa2EmOXN46MsEOmaYC4zJYCZe1QQH2ra0khL4Hy+ZS2qEWYjfg?=
 =?us-ascii?Q?7r7WoWBuTMEXQXr0btwppLzhto+Yo5zJVs4J41YgGpeZ8jcooLuLxbZVV8tO?=
 =?us-ascii?Q?zctC4A0XvbcDXuPSnKnwn+DA4CwiqqA39Gbfbt4vYZesw9rtp5hKOjliFKtY?=
 =?us-ascii?Q?ipMlI8VV7s3phV/lqnvZMMPsTC6TaPn/gBOZmuCftczMhiT5O86mUDfy7D02?=
 =?us-ascii?Q?ErSbkbqubaCAdmSbl3NZ0fd8bAaeb5hBuLgwKWQjOSESHS1k+0xQb0qYipzO?=
 =?us-ascii?Q?cu0sDv4dRLVRNMgCbc8fgR8rRLAeJjsqZOFVu6NW4d+Bc2sJdc0jqneBPWeh?=
 =?us-ascii?Q?t8ZlBjf6uLigVIBX8hs98+xNadWEv9RtDQCyZiFj52SFSbifne/bT0emjCzw?=
 =?us-ascii?Q?49Mn4nAmcjoHeMj2Ht1HyncxD+RkDQ4FZZCZYo3QEmnQ+T8S9yYmSDbmwVsS?=
 =?us-ascii?Q?Pg9vJbRQIwR4EtFr3GxPZYVwUvLAbPjXi8l8fHKP6Fza99fuFy8N7OfxdUyb?=
 =?us-ascii?Q?8QzitmAg3pyuugoCgc5BfF5HejDzKTb/pG167F6tLv9j+qXWmQky+xNI2Tm/?=
 =?us-ascii?Q?RI29JiVCKD1fvHFCoJt4ArL7951FqMku3WOsqh7KU+m0tZX9xGGO5Fu3Rjsk?=
 =?us-ascii?Q?ZPsOsOf4Si8Hlo9OnuWUo8fRECKRw4R6Mjm1+P2gWPZNDd6Ch5BDNC4ia3Qp?=
 =?us-ascii?Q?J2M/BpfinywtzFedHRsk2nShDfI7BekO7yBiFStFh/wTOyNqtHPVB9WY7KnK?=
 =?us-ascii?Q?tXeYcx7jSV5g0vxeidfNFVk9a+5XkWzwWZL8xYkqx/Mn6703qS9x6oqaBeRX?=
 =?us-ascii?Q?jCmvDO4s96JgEe2mgG4kXZkvmGp2VtPV0oSQcw7oV+QZlEPOZbSEhUl2EHCv?=
 =?us-ascii?Q?uMdrX/lB1xEYhbBabmGoKtQwDFosCzQ6f9TfIC3A7G+biBqe2pjS9qF4UzqD?=
 =?us-ascii?Q?V9ud1N/yNsR+FnD78SSRZB79V3ygkrS9iK4W7bcDEI/PrEDNk5WB8IjrVjgh?=
 =?us-ascii?Q?N4Daoh2ZMno6FUHx8wKnm5w5rrfzt4Mmo9yHj7DxVOXSv1J3/QsUS3g+NXUr?=
 =?us-ascii?Q?LIN+3hZYbVIAZcp96wntfkOGbw9MDTPvmgRaxPx9PKMOH/1csLJyKZOmn6YE?=
 =?us-ascii?Q?1Snp+iWygoUd/S5FN4peT5IUAIsiyHrtgpLAR8EOwgEnQtCQ7fM12PaAX2o7?=
 =?us-ascii?Q?mAUmQmXhjkrCwcCfFv/BWv8E58OVH6Rnayb7C8Qnt5+zvpBQN9YJpPsqbdX4?=
 =?us-ascii?Q?nhTqfTXGt2tNhKiwJh7PRra5z6zayw+2CcNYxpk1L7Na7Fc/VSyvd659gnXx?=
 =?us-ascii?Q?vlJQaXPc4ERki0SG0wKXqIL96O+H8H77ifAidCMqkL7P/UG7JMxu8lG88sed?=
 =?us-ascii?Q?S/FnyP5378FojzQkYcAlsojxD91vEhV9tMIGw9JM3dKKHHywe5UO8wkuh5/v?=
 =?us-ascii?Q?8cgk7vmRdxHDjwQQ1Q5Wh0+XaXzimowA/z2A8sbrBFpdvTEPV9y+YleTRmcc?=
 =?us-ascii?Q?/ZW8acaRDibqyRg15Kd3x9IcIfYPWctF6bRkFdQM+evtSSsy8E599KSIunuU?=
 =?us-ascii?Q?Nw1e9jcnRPBwBBpaZd/1WI8ot8fG5GD8Ew6VYEREoaAiOsCALDt4OPkQbJeG?=
 =?us-ascii?Q?CA=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 732a9a9f-a6c5-413b-8af4-08dc2118e59e
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Jan 2024 22:23:19.6036
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: hhlQIEIeMgkMWW1BmS0vR0pBuDyp3PUCby80t1B/bUR4HCpMIJP3ia95ukDdTgQguGEWmMdDmsv9nP9PhzSOoDFSgk3CoQbMq0kknb4hm6s=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB6576
X-OriginatorOrg: intel.com

Fabio M. De Francesco wrote:
> Add cond_guard() to conditional guards.
> 
> cond_guard() stores the return value from _interruptible(), _killable(),
> and _try versions of locks to a variable whose address is passed as the
> second parameter, so that the value can later be checked and an action
> be taken accordingly (e.g., calling printk() in case of failure).

This does not look like it returns the result from the conditional
locking function, it looks like it returns whether the boolean of
whether the CLASS() constructor succeeded or failed:

 "*_ret = (!__guard_ptr(_name)(&scope)"

Even if it can't return the result from the lock call itself directly,
it would still be nice to be able to write something like:

    rc = cond_guard(rwsem_read_intr, -EINTR, &rwsem);
    if (rc)
        return rc;

..and know that the lock is taken of -EINTR is returned.

That would also mean being able to write "if (cond_guard())", which I
don't think would work with the current proposal.

> As the other guards, it avoids to open code the release of the lock
> after a goto to an 'out' label.
> 
> This is an RFC at least for two reasons...
> 
> 1) I'm not sure that this guard is needed unless we want to avoid the
> use of scoped_cond_guard() with its necessary indentation of the code
> in the successful path and/or we want to check the return value of the
> conditional lock.

Taking this argument to its logical conclusion would mean removing plain
guard() as well. There is definitely a code readability benefit for
having the option of guard() vs scoped_guard(), so it would be nice to
have similar flexibility in the conditional case.

> 2) By using cond_guard() it is not possible to release the lock before
> calling other functions from the current one. The lock is held until the
> current function exits. This is not always desirable.

No, that's not correct, the lock is only held until the exit from the
current scope, and if you call functions from within that scope the lock
remains held. So:

    func1()
    {
        guard(...)(lock1);
        if (condition) {
            guard(...)(lock2);
            func2();
        }
        func3();
    }

func2() is called with lock1 and lock2 held, func3() is only called with
lock1() held.