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
Date: Mon, 12 Feb 2024 15:02:46 -0800 [thread overview]
Message-ID: <65caa3966caa_5a7f294cf@dwillia2-xfh.jf.intel.com.notmuch> (raw)
In-Reply-To: <CAHk-=wiqaENZFBiAihFxdLr2E+kSM4P64M3uPzwT4-K9NiVSmw@mail.gmail.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.
next prev parent reply other threads:[~2024-02-12 23:03 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-12 16:30 [PATCH v5 0/8] Introduce cpu_dcache_is_aliasing() to fix DAX regression Mathieu Desnoyers
2024-02-12 16:30 ` [PATCH v5 1/8] dax: alloc_dax() return ERR_PTR(-EOPNOTSUPP) for CONFIG_DAX=n Mathieu Desnoyers
2024-02-13 6:32 ` Lukas Wunner
2024-02-13 19:07 ` Dan Williams
2024-02-13 20:07 ` Mathieu Desnoyers
2024-02-12 16:30 ` [PATCH v5 2/8] nvdimm/pmem: Treat alloc_dax() -EOPNOTSUPP failure as non-fatal Mathieu Desnoyers
2024-02-12 16:30 ` [PATCH v5 3/8] dm: " Mathieu Desnoyers
2024-02-12 16:30 ` [PATCH v5 4/8] dcssblk: Handle alloc_dax() -EOPNOTSUPP failure Mathieu Desnoyers
2024-02-13 11:18 ` Heiko Carstens
2024-02-12 16:30 ` [PATCH v5 5/8] virtio: Treat alloc_dax() -EOPNOTSUPP failure as non-fatal Mathieu Desnoyers
2024-02-12 22:04 ` Dan Williams
2024-02-12 22:08 ` Linus Torvalds
2024-02-12 23:02 ` Dan Williams [this message]
2024-02-13 6:18 ` Lukas Wunner
2024-02-13 18:56 ` Dan Williams
2024-02-13 20:25 ` Mathieu Desnoyers
2024-02-13 6:25 ` Lukas Wunner
2024-02-13 19:46 ` Mathieu Desnoyers
2024-02-14 6:59 ` Greg Kroah-Hartman
2024-02-12 16:30 ` [PATCH v5 6/8] dax: Check for data cache aliasing at runtime Mathieu Desnoyers
2024-02-12 16:31 ` [PATCH v5 7/8] Introduce cpu_dcache_is_aliasing() across all architectures Mathieu Desnoyers
2024-02-12 16:31 ` [PATCH v5 8/8] dax: Fix incorrect list of data cache aliasing architectures Mathieu Desnoyers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=65caa3966caa_5a7f294cf@dwillia2-xfh.jf.intel.com.notmuch \
--to=dan.j.williams@intel.com \
--cc=agk@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=arnd@arndb.de \
--cc=dave.jiang@intel.com \
--cc=david@fromorbit.com \
--cc=dm-devel@lists.linux.dev \
--cc=linux-arch@vger.kernel.org \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=linux-s390@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=lukas@wunner.de \
--cc=mathieu.desnoyers@efficios.com \
--cc=mpatocka@redhat.com \
--cc=nvdimm@lists.linux.dev \
--cc=snitzer@kernel.org \
--cc=torvalds@linux-foundation.org \
--cc=vishal.l.verma@intel.com \
--cc=willy@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).