| Message ID | 20240202003942.647599-1-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: Swap order of actions in the FREE*() macros | expand |
On 02.02.2024 01:39, Andrew Cooper wrote: > Wherever possible, it is a good idea to NULL out the visible reference to an > object prior to freeing it. The FREE*() macros already collect together both > parts, making it easy to adjust. > > This has a marginal code generation improvement, as some of the calls to the > free() function can be tailcall optimised. > > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Acked-by: Jan Beulich <jbeulich@suse.com> However, ... > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -92,8 +92,9 @@ bool scrub_free_pages(void); > > /* Free an allocation, and zero the pointer to it. */ > #define FREE_XENHEAP_PAGES(p, o) do { \ > - free_xenheap_pages(p, o); \ > + void *_ptr_ = (p); \ ... why a trailing _and_ a leading underscore? Sooner or later we'll need to get rid of the leading ones anyway aiui (for Misra), here and elsewhere. With it omitted right away (also below): Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
Hi Andrew, > On 2 Feb 2024, at 00:39, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > Wherever possible, it is a good idea to NULL out the visible reference to an > object prior to freeing it. The FREE*() macros already collect together both > parts, making it easy to adjust. > > This has a marginal code generation improvement, as some of the calls to the > free() function can be tailcall optimised. > Could you improve a bit the commit message and explain a bit why it is a good idea and also how the code might be improved because i do not get it ? Cheers Bertrand > No functional change. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: George Dunlap <George.Dunlap@citrix.com> > CC: Jan Beulich <JBeulich@suse.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Wei Liu <wl@xen.org> > CC: Julien Grall <julien@xen.org> > --- > xen/include/xen/mm.h | 3 ++- > xen/include/xen/xmalloc.h | 7 ++++--- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h > index 3d9b2d05a5c8..044f3f3b19c8 100644 > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -92,8 +92,9 @@ bool scrub_free_pages(void); > > /* Free an allocation, and zero the pointer to it. */ > #define FREE_XENHEAP_PAGES(p, o) do { \ > - free_xenheap_pages(p, o); \ > + void *_ptr_ = (p); \ > (p) = NULL; \ > + free_xenheap_pages(_ptr_, o); \ > } while ( false ) > #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0) > > diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h > index 9ecddbff5e00..1b88a83be879 100644 > --- a/xen/include/xen/xmalloc.h > +++ b/xen/include/xen/xmalloc.h > @@ -66,9 +66,10 @@ > extern void xfree(void *p); > > /* Free an allocation, and zero the pointer to it. */ > -#define XFREE(p) do { \ > - xfree(p); \ > - (p) = NULL; \ > +#define XFREE(p) do { \ > + void *_ptr_ = (p); \ > + (p) = NULL; \ > + xfree(_ptr_); \ > } while ( false ) > > /* Underlying functions */ > > base-commit: 3f819af8a796c0e2f798dd301ec8c3f8cccbc9fc > -- > 2.30.2 > >
diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index 3d9b2d05a5c8..044f3f3b19c8 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -92,8 +92,9 @@ bool scrub_free_pages(void); /* Free an allocation, and zero the pointer to it. */ #define FREE_XENHEAP_PAGES(p, o) do { \ - free_xenheap_pages(p, o); \ + void *_ptr_ = (p); \ (p) = NULL; \ + free_xenheap_pages(_ptr_, o); \ } while ( false ) #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0) diff --git a/xen/include/xen/xmalloc.h b/xen/include/xen/xmalloc.h index 9ecddbff5e00..1b88a83be879 100644 --- a/xen/include/xen/xmalloc.h +++ b/xen/include/xen/xmalloc.h @@ -66,9 +66,10 @@ extern void xfree(void *p); /* Free an allocation, and zero the pointer to it. */ -#define XFREE(p) do { \ - xfree(p); \ - (p) = NULL; \ +#define XFREE(p) do { \ + void *_ptr_ = (p); \ + (p) = NULL; \ + xfree(_ptr_); \ } while ( false ) /* Underlying functions */
Wherever possible, it is a good idea to NULL out the visible reference to an object prior to freeing it. The FREE*() macros already collect together both parts, making it easy to adjust. This has a marginal code generation improvement, as some of the calls to the free() function can be tailcall optimised. No functional change. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: George Dunlap <George.Dunlap@citrix.com> CC: Jan Beulich <JBeulich@suse.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Wei Liu <wl@xen.org> CC: Julien Grall <julien@xen.org> --- xen/include/xen/mm.h | 3 ++- xen/include/xen/xmalloc.h | 7 ++++--- 2 files changed, 6 insertions(+), 4 deletions(-) base-commit: 3f819af8a796c0e2f798dd301ec8c3f8cccbc9fc