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(-)

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

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
> 
> 

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