Received-SPF: pass (google.com: domain of linux-kernel+bounces-62516-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 12 Feb 2024 15:02:46 -0800
From: Dan Williams <dan.j.williams@intel.com>
To: Linus Torvalds <torvalds@linux-foundation.org>, Dan Williams
	<dan.j.williams@intel.com>
CC: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, Arnd Bergmann
	<arnd@arndb.de>, Dave Chinner <david@fromorbit.com>,
	<linux-kernel@vger.kernel.org>, Andrew Morton <akpm@linux-foundation.org>,
	Vishal Verma <vishal.l.verma@intel.com>, Dave Jiang <dave.jiang@intel.com>,
	Matthew Wilcox <willy@infradead.org>, Russell King <linux@armlinux.org.uk>,
	<linux-arch@vger.kernel.org>, <linux-cxl@vger.kernel.org>,
	<linux-fsdevel@vger.kernel.org>, <linux-mm@kvack.org>,
	<linux-xfs@vger.kernel.org>, <dm-devel@lists.linux.dev>,
	<nvdimm@lists.linux.dev>, <linux-s390@vger.kernel.org>, Alasdair Kergon
	<agk@redhat.com>, Mike Snitzer <snitzer@kernel.org>, Mikulas Patocka
	<mpatocka@redhat.com>, <lukas@wunner.de>
Subject: Re: [PATCH v5 5/8] virtio: Treat alloc_dax() -EOPNOTSUPP failure as
 non-fatal
Message-ID: <65caa3966caa_5a7f294cf@dwillia2-xfh.jf.intel.com.notmuch>
References: <20240212163101.19614-1-mathieu.desnoyers@efficios.com>
 <20240212163101.19614-6-mathieu.desnoyers@efficios.com>
 <65ca95d086dfd_d2d429470@dwillia2-xfh.jf.intel.com.notmuch>
 <CAHk-=wiqaENZFBiAihFxdLr2E+kSM4P64M3uPzwT4-K9NiVSmw@mail.gmail.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <CAHk-=wiqaENZFBiAihFxdLr2E+kSM4P64M3uPzwT4-K9NiVSmw@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?6PvYMpR4fB5LSnp2a4B1qjnyRVhrUuIL6waIb3bKxQpZneqH6TeNuHEF9Q0A?=
 =?us-ascii?Q?iHE0PGhNqCIVqJY0X8BWFKGUBnB3783nxFP4T8HfifxXymjfz/FF9rWc4F1z?=
 =?us-ascii?Q?on91kstKu8PHq8P2e5cY58/slYziN3+l9ZAG+N7yDTSkb8f72EV4ZFSCl+e3?=
 =?us-ascii?Q?+5M+JuRSaGYUJMRF8clmIkaD6snobTCzfu/nPk2J/iDYXBA+DLn5CPlZKvkV?=
 =?us-ascii?Q?9txwUcyuwwBomKyz8mMsf3Zzl0ntMUu2o9jfvWMSwpS7HdFATHndTGIuYnEP?=
 =?us-ascii?Q?wh/JweFAa0lDHIMX7rg8TUITf9EWigJFkK1tuaJOSxtUnp/1cqQhzX6C+0lu?=
 =?us-ascii?Q?n5iyP1Hu1RrSNCJdAwwwut1mRGvipyEgCWxntB+9lm6ECStymLtb+ZNOyH2E?=
 =?us-ascii?Q?aShEX0hcxkApUP5zUw4vc3LPdut1casr2uyOKGi4NmxHbcnVkvI06etGMGE4?=
 =?us-ascii?Q?5+ulFSfc2P0UvbX1j6G+YywVNA8KwEHOYC+brRPMCdhP6TeDTvMtqJy1daHm?=
 =?us-ascii?Q?K0cqUruEnRue99AScg5VMdYkcFkz79OxD0dqJ1pPspUGyf8vdSKkM6jVY5ah?=
 =?us-ascii?Q?LztKHE72/u1LmniRsIPkcpzgRuoYpJMV6eNdzhtGxRVs6A1RhCSUa+DweJjm?=
 =?us-ascii?Q?WoJ4AJOLOxKC/kdUR3QkmmWbEAoRGBhNAx2v5TTOnCRzL/WEjd4j0l/Paefs?=
 =?us-ascii?Q?1mJgs95noPPxM+GA5ulqaeBa6Pe2CJz+vqp80uZ+BUtoQ7Cwfd7zlUse31XG?=
 =?us-ascii?Q?erZCdw40nFWa96qrJ5qCmN4ZEMiA/E3p4Coz79VUQPV8GnE4OJH5BymAoKOl?=
 =?us-ascii?Q?nSHrMf3ybGMdUbebXrATtEbzUKa8NAjSqCgRRw9SB/uE3gQduz96eZEYgLZt?=
 =?us-ascii?Q?WWIjXMFjDeTCqxPtqXy/V1NTaH0miRsq7hbQZZduXd/2HyJeakML9pWTqCen?=
 =?us-ascii?Q?sxWoM7nsZ8WSMm1hVmxqwMA0rkf0vRcBHKwYsUF0I6u9NJIj/8MD8quF5lBt?=
 =?us-ascii?Q?1342x0RpZq7u8JU57kdrb+UbcfGM3mb5lrWjMST26acGySruOTjPw5MqIVZv?=
 =?us-ascii?Q?9QFz2DLmb1rI4rakx+/BMO0Pqte8xHsF5MbSdAVHZJfP/dri2pNGRDCpyWVA?=
 =?us-ascii?Q?olAoUPVaD+yUrpZl7Jxkzycsordc4pWOqA+J11ZVYTUewrF6jixX+BjDBftp?=
 =?us-ascii?Q?yMxIXbkL7lIYOMf+w0d6mkggvmPMt8jPMmc5f9um8A/MqDdqlCaTo9VCiyZW?=
 =?us-ascii?Q?GFDbO1bzbNxkwJAoFgOUdm+oly3Use4aEH0AaEQmOcKw63LEPx/uJgXeMn8Q?=
 =?us-ascii?Q?jaRooegDH/rVucog7Fo2WMU3W76j2zGhFBExJkNUt/V4WMV5/swLT0K2ZhtK?=
 =?us-ascii?Q?ntRxGU5ZNcwb+nVkMs3t4edEzujwXYoU2LINiaR6eSieFLnu8w2Dt218EG21?=
 =?us-ascii?Q?wD6vXlwBCtYaIq88jTq8VQ507scIrZBjq9LsQv10i8eEM6F3H9PhGtUE8lLG?=
 =?us-ascii?Q?99x2ABjyJxeJfJh4fboa+nVxMlgRIiuf5bx3p5fpFmRtl72uGfpPeuoPz7Qe?=
 =?us-ascii?Q?X3tgnodhqrz5yjfmp/D4WtnknLGGb6T7++x0MWFhtTsxJ1zmRTH9ehx4R5IX?=
 =?us-ascii?Q?IQ=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 037317e4-9f9d-4571-7b70-08dc2c1ebbce
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Feb 2024 23:02:49.2787
 (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: yIhD1EByiOH8Y0YtK6g5ovC/SxzZUzGmrYsO1AcUpsh97rG/cjcfcAMzSCB1ijHxVeSagOq46xybzBme1ob/oUM2B3e14C817bll7TMDSFk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR11MB6293
X-OriginatorOrg: intel.com

[ add Lukas ]

Linus Torvalds wrote:
> On Mon, 12 Feb 2024 at 14:04, Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > This works because the internals of virtio_fs_cleanup_dax(), "kill_dax()
> > and put_dax()", know how to handle a NULL @dax_dev. It is still early
> > days with the "cleanup" helpers, but I wonder if anyone else cares that
> > the DEFINE_FREE() above does not check for NULL?
> 
> Well, the main reason for DEFINE_FREE() to check for NULL is not
> correctness, but code generation. See the comment about kfree() in
> <linux/cleanup.h>:
> 
>  * NOTE: the DEFINE_FREE()'s @free expression includes a NULL test even though
>  * kfree() is fine to be called with a NULL value. This is on purpose. This way
>  * the compiler sees the end of our alloc_obj() function as [...]
> 
> with the full explanation there.
> 
> Now, whether the code wants to actually use the cleanup() helpers for
> a single use-case is debatable.
> 
> But yes, if it does, I suspect it should use !IS_ERR_OR_NULL(ptr).o

I am trying to arrive at a common recommendation given Lukas found that
IS_ERR_OR_NULL() resulted in unwanted NULL checks emitted in the
assembly [1].

He is doing something similar:

http://lore.kernel.org/r/4143b15418c4ecf87ddeceb36813943c3ede17aa.1707734526.git.lukas@wunner.de

..and introduced an assume() helper.

However, Lukas, I think Linus is right, your DEFINE_FREE() should use
IS_ERR_OR_NULL(). I.e. the problem is trying to use
__free(x509_free_certificate) in x509_cert_parse().

> --- a/crypto/asymmetric_keys/x509_cert_parser.c
> +++ b/crypto/asymmetric_keys/x509_cert_parser.c
> @@ -60,24 +60,24 @@ void x509_free_certificate(struct x509_certificate *cert)
>   */
>  struct x509_certificate *x509_cert_parse(const void *data, size_t datalen)
>  {
> -       struct x509_certificate *cert;
> -       struct x509_parse_context *ctx;
> +       struct x509_certificate *cert __free(x509_free_certificate);

..make this:

    struct x509_certificate *cert __free(kfree);

..and Mathieu, this should be IS_ERR_OR_NULL() to skip an unnecessary
call to virtio_fs_cleanup_dax() at function exit that the compiler
should elide.