Re: [PATCH 1/4] xen/dmalloc: Introduce dmalloc() APIs

[Julien Grall <julien@xen.org>]

Hi Andrew,

On 23/12/2020 16:34, Andrew Cooper wrote:
> Wrappers for xmalloc() and friends, which track allocations tied to a specific
> domain.
> 
> Check for any leaked memory at domain destruction time.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> CC: Tamas K Lengyel <tamas@tklengyel.com>
> 
> RFC:
>   * This probably wants to be less fatal in release builds
>   * In an ideal world, we'd also want to count the total number of bytes
>     allocated from the xmalloc heap, which would be interesting to print in the
>     'q' debugkey.  However, that data is fairly invasive to obtain.
>   * More complicated logic could track the origins of each allocation, and be
>     able to identify which one(s) leaked.
> ---
>   xen/common/Makefile       |  1 +
>   xen/common/dmalloc.c      | 19 +++++++++++++++++++
>   xen/common/domain.c       |  6 ++++++
>   xen/include/xen/dmalloc.h | 29 +++++++++++++++++++++++++++++
>   xen/include/xen/sched.h   |  2 ++
>   5 files changed, 57 insertions(+)
>   create mode 100644 xen/common/dmalloc.c
>   create mode 100644 xen/include/xen/dmalloc.h
> 
> diff --git a/xen/common/Makefile b/xen/common/Makefile
> index 7a4e652b57..c5d9c23fd1 100644
> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -5,6 +5,7 @@ obj-$(CONFIG_CORE_PARKING) += core_parking.o
>   obj-y += cpu.o
>   obj-$(CONFIG_DEBUG_TRACE) += debugtrace.o
>   obj-$(CONFIG_HAS_DEVICE_TREE) += device_tree.o
> +obj-y += dmalloc.o
>   obj-y += domain.o
>   obj-y += event_2l.o
>   obj-y += event_channel.o
> diff --git a/xen/common/dmalloc.c b/xen/common/dmalloc.c
> new file mode 100644
> index 0000000000..e3a0e546c2
> --- /dev/null
> +++ b/xen/common/dmalloc.c
> @@ -0,0 +1,19 @@
> +#include <xen/dmalloc.h>
> +#include <xen/sched.h>
> +#include <xen/xmalloc.h>
> +
> +void dfree(struct domain *d, void *ptr)
> +{
> +    atomic_dec(&d->dalloc_heap);
> +    xfree(ptr);
> +}
> +
> +void *_dzalloc(struct domain *d, size_t size, size_t align)
> +{
> +    void *ptr = _xmalloc(size, align);

The 'z' in _dzalloc() implies the memory will want to be zeroed. But 
here you are using _xmalloc().

This also explains the "memory leak" you reported. By switching to 
"_xzalloc()" and the problem disappears (at least on my setup).

Cheers,

-- 
Julien Grall