From: Dan Williams <dan.j.williams@intel.com>
To: Lukas Wunner <lukas@wunner.de>, Peter Zijlstra <peterz@infradead.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Dan Williams <dan.j.williams@intel.com>,
<linux-coco@lists.linux.dev>,
Andrew Morton <akpm@linux-foundation.org>, <x86@kernel.org>,
<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 4/5] mm/slab: Add __free() support for kvfree
Date: Thu, 4 Jan 2024 10:29:03 -0800 [thread overview]
Message-ID: <6596f8ef5fafe_8dc682949e@dwillia2-xfh.jf.intel.com.notmuch> (raw)
In-Reply-To: <20240104065744.GA6055@wunner.de>
Lukas Wunner wrote:
> On Mon, Aug 14, 2023 at 06:17:31PM +0200, Peter Zijlstra wrote:
> > On Mon, Aug 14, 2023 at 05:31:27PM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Aug 14, 2023 at 12:43:32AM -0700, Dan Williams wrote:
> > > > Allow for the declaration of variables that trigger kvfree() when they
> > > > go out of scope.
> > > >
> > > > Cc: Andrew Morton <akpm@linux-foundation.org>
> > > > Cc: Peter Zijlstra <peterz@infradead.org>
> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > > > ---
> > > > include/linux/slab.h | 2 ++
> > > > 1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/include/linux/slab.h b/include/linux/slab.h
> > > > index 848c7c82ad5a..241025367943 100644
> > > > --- a/include/linux/slab.h
> > > > +++ b/include/linux/slab.h
> > > > @@ -746,6 +746,8 @@ static inline __alloc_size(1, 2) void *kvcalloc(size_t n, size_t size, gfp_t fla
> > > > extern void *kvrealloc(const void *p, size_t oldsize, size_t newsize, gfp_t flags)
> > > > __realloc_size(3);
> > > > extern void kvfree(const void *addr);
> > > > +DEFINE_FREE(kvfree, void *, if (_T) kvfree(_T))
> > >
> > > No need to check _T before calling this, right (as was also pointed out
> > > earlier).
> >
> > Well, that does mean you get an unconditional call to kvfree() in the
> > success case. Linus argued against this.
> >
> > This way the compiler sees:
> >
> > buf = NULL;
> > if (buf)
> > kvfree(buf);
> >
> > and goes: 'let me clean that up for you'. And all is well.
>
> Have you actually verified that assumption in the generated Assembler code?
>
> The kernel is compiled with -fno-delete-null-pointer-checks since commit
> a3ca86aea507 ("Add '-fno-delete-null-pointer-checks' to gcc CFLAGS").
>
> So NULL pointer checks are *not* optimized away even if the compiler
> knows that a pointer is NULL.
Interesting, I am not sure how -fno-delete-null-pointer-checks plays
into this, but I can confirm that Peter's expectations are being met in
a routine with:
DEFINE_FREE(pci_dev_put, struct pci_dev *, if (_T) pci_dev_put(_T))
...without that conditional the assembly is:
0xffffffff819ad129 <+41>: call 0xffffffff81800840 <pci_get_domain_bus_and_slot>
0xffffffff819ad12e <+46>: mov %rax,%r12
0xffffffff819ad131 <+49>: test %rax,%rax
0xffffffff819ad134 <+52>: je 0xffffffff819ad154 <cxl_cper_event_call+84>
0xffffffff819ad136 <+54>: mov %rax,%rdi
0xffffffff819ad139 <+57>: call 0xffffffff817f5f10 <pci_dev_lock>
0xffffffff819ad13e <+62>: cmpq $0xffffffff82c681c0,0x80(%r12)
0xffffffff819ad14a <+74>: je 0xffffffff819ad160 <cxl_cper_event_call+96>
0xffffffff819ad14c <+76>: mov %r12,%rdi
0xffffffff819ad14f <+79>: call 0xffffffff817f5fa0 <pci_dev_unlock>
0xffffffff819ad154 <+84>: pop %rbx
0xffffffff819ad155 <+85>: mov %r12,%rdi
0xffffffff819ad158 <+88>: pop %rbp
0xffffffff819ad159 <+89>: pop %r12
0xffffffff819ad15b <+91>: jmp 0xffffffff817fe1e0 <pci_dev_put>
...i.e. the check for NULL at 0xffffffff819ad134 jumps to do an
unnecessary pci_dev_put(). With the conditional in the macro the
sequence is:
0xffffffff819ad129 <+41>: call 0xffffffff81800840 <pci_get_domain_bus_and_slot>
0xffffffff819ad12e <+46>: test %rax,%rax
0xffffffff819ad131 <+49>: je 0xffffffff819ad18c <cxl_cper_event_call+140>
0xffffffff819ad133 <+51>: mov %rax,%r12
0xffffffff819ad136 <+54>: mov %rax,%rdi
0xffffffff819ad139 <+57>: call 0xffffffff817f5f10 <pci_dev_lock>
0xffffffff819ad13e <+62>: cmpq $0xffffffff82c681c0,0x80(%r12)
0xffffffff819ad14a <+74>: je 0xffffffff819ad160 <cxl_cper_event_call+96>
0xffffffff819ad14c <+76>: mov %r12,%rdi
0xffffffff819ad14f <+79>: call 0xffffffff817f5fa0 <pci_dev_unlock>
...
0xffffffff819ad18c <+140>: pop %rbx
0xffffffff819ad18d <+141>: pop %rbp
0xffffffff819ad18e <+142>: pop %r12
0xffffffff819ad190 <+144>: jmp 0xffffffff81efc6a0 <__x86_return_thunk>
...i.e. optimize away the pci_dev_put() and return directly when @pdev
is already known to be NULL. So empirically
-fno-delete-null-pointer-checks still allows for redundant NULL checks
to be optimized.
next prev parent reply other threads:[~2024-01-04 18:29 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-14 7:43 [PATCH v2 0/5] tsm: Attestation Report ABI Dan Williams
2023-08-14 7:43 ` [PATCH v2 1/5] virt: coco: Add a coco/Makefile and coco/Kconfig Dan Williams
2023-08-14 7:43 ` [PATCH v2 2/5] tsm: Introduce a shared ABI for attestation reports Dan Williams
2023-08-14 8:24 ` Jeremi Piotrowski
2023-08-14 16:21 ` Dan Williams
2023-08-14 15:38 ` Greg Kroah-Hartman
2023-08-14 16:43 ` Dan Williams
2023-08-14 18:43 ` Greg Kroah-Hartman
2023-08-15 19:51 ` Tom Lendacky
2023-08-16 14:44 ` Tom Lendacky
2023-08-16 15:12 ` Dan Williams
2023-08-22 7:29 ` Roy Hopkins
2023-08-23 13:49 ` Samuel Ortiz
2023-08-28 10:46 ` Dr. Greg
2023-08-14 7:43 ` [PATCH v2 3/5] virt: sevguest: Prep for kernel internal {get, get_ext}_report() Dan Williams
2023-08-14 16:58 ` Dionna Amalie Glaze
2023-08-14 23:24 ` Dan Williams
2023-08-15 20:11 ` Tom Lendacky
2023-08-15 21:03 ` Dan Williams
2023-08-16 19:38 ` Dionna Amalie Glaze
2023-08-15 20:20 ` Tom Lendacky
2023-08-15 21:37 ` Dan Williams
2023-08-14 7:43 ` [PATCH v2 4/5] mm/slab: Add __free() support for kvfree Dan Williams
2023-08-14 15:31 ` Greg Kroah-Hartman
2023-08-14 16:17 ` Peter Zijlstra
2023-08-14 18:44 ` Greg Kroah-Hartman
2023-08-14 18:45 ` Greg Kroah-Hartman
2024-01-04 6:57 ` Lukas Wunner
2024-01-04 18:29 ` Dan Williams [this message]
2023-08-14 7:43 ` [PATCH v2 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT Dan Williams
2023-08-14 8:30 ` Jeremi Piotrowski
2023-08-14 16:22 ` Dan Williams
2023-08-14 11:21 ` Peter Zijlstra
2023-08-14 16:25 ` Dan Williams
2023-08-14 16:48 ` Peter Zijlstra
2023-08-14 22:15 ` Peter Zijlstra
2023-08-15 8:37 ` Peter Zijlstra
2023-08-15 20:50 ` Tom Lendacky
2023-08-15 21:40 ` Dan Williams
2023-08-14 9:04 ` [PATCH v2 0/5] tsm: Attestation Report ABI Jeremi Piotrowski
2023-08-14 17:12 ` Dan Williams
2023-08-15 14:27 ` Peter Gonda
2023-08-15 17:16 ` Dionna Amalie Glaze
2023-08-15 21:13 ` Dan Williams
2023-08-15 18:13 ` Dan Williams
2023-08-16 9:42 ` Jeremi Piotrowski
2023-08-23 11:21 ` Dr. Greg
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=6596f8ef5fafe_8dc682949e@dwillia2-xfh.jf.intel.com.notmuch \
--to=dan.j.williams@intel.com \
--cc=akpm@linux-foundation.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-coco@lists.linux.dev \
--cc=linux-kernel@vger.kernel.org \
--cc=lukas@wunner.de \
--cc=peterz@infradead.org \
--cc=x86@kernel.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).