[Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family

[Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family

[Avi Kivity]

This makes the tracing infrastructure available to users of g_new().

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 qemu-common.h |    1 +
 qemu-malloc.c |   15 +++++++++++++++
 vl.c          |    1 +
 3 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/qemu-common.h b/qemu-common.h
index 74d5c4b..fbe2de0 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -180,6 +180,7 @@ const char *path(const char *pathname);
 #define qemu_isascii(c)		isascii((unsigned char)(c))
 #define qemu_toascii(c)		toascii((unsigned char)(c))
 
+void qemu_malloc_init(void);
 void *qemu_oom_check(void *ptr);
 void *qemu_malloc(size_t size);
 void *qemu_realloc(void *ptr, size_t size);
diff --git a/qemu-malloc.c b/qemu-malloc.c
index b9b3851..8b0c1ec 100644
--- a/qemu-malloc.c
+++ b/qemu-malloc.c
@@ -24,6 +24,21 @@
 #include "qemu-common.h"
 #include "trace.h"
 #include <stdlib.h>
+#include <glib.h>
+
+static GMemVTable gmemvtable = {
+    .malloc = qemu_malloc,
+    .realloc = qemu_realloc,
+    .free = qemu_free,
+};
+
+/**
+ * qemu_malloc_init: initialize memory management
+ */
+void qemu_malloc_init(void)
+{
+    g_mem_set_vtable(&gmemvtable);
+}
 
 void qemu_free(void *ptr)
 {
diff --git a/vl.c b/vl.c
index c714127..7c4f8da 100644
--- a/vl.c
+++ b/vl.c
@@ -2106,6 +2106,7 @@ int main(int argc, char **argv, char **envp)
 
     atexit(qemu_run_exit_notifiers);
     error_set_progname(argv[0]);
+    qemu_malloc_init();
 
     init_clocks();
 
-- 
1.7.6

Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family

[Stefan Hajnoczi]

On Thu, Aug 18, 2011 at 6:48 PM, Avi Kivity <avi@redhat.com> wrote:
> This makes the tracing infrastructure available to users of g_new().
>
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  qemu-common.h |    1 +
>  qemu-malloc.c |   15 +++++++++++++++
>  vl.c          |    1 +
>  3 files changed, 17 insertions(+), 0 deletions(-)

Seems useful :)

Stefan

Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family

[Peter Maydell]

On 18 August 2011 18:48, Avi Kivity <avi@redhat.com> wrote:
> +static GMemVTable gmemvtable = {
> +    .malloc = qemu_malloc,
> +    .realloc = qemu_realloc,
> +    .free = qemu_free,
> +};
> +
> +/**
> + * qemu_malloc_init: initialize memory management
> + */
> +void qemu_malloc_init(void)
> +{
> +    g_mem_set_vtable(&gmemvtable);
> +}

Does this mean you can now safely allocate with g_malloc
and free with qemu_free, or is mixing the two APIs like that
still a no-no ?

(I'm thinking about a situation where you might use a glib utility
function that returned g_malloc'd memory and want to pass that back
to your caller without having to either copy to qemu_malloc'd memory
or require your caller to care about the distinction.)

-- PMM

Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family

[Avi Kivity]

On 08/18/2011 09:54 PM, Peter Maydell wrote:
> On 18 August 2011 18:48, Avi Kivity<avi@redhat.com>  wrote:
> >  +static GMemVTable gmemvtable = {
> >  +    .malloc = qemu_malloc,
> >  +    .realloc = qemu_realloc,
> >  +    .free = qemu_free,
> >  +};
> >  +
> >  +/**
> >  + * qemu_malloc_init: initialize memory management
> >  + */
> >  +void qemu_malloc_init(void)
> >  +{
> >  +    g_mem_set_vtable(&gmemvtable);
> >  +}
>
> Does this mean you can now safely allocate with g_malloc
> and free with qemu_free, or is mixing the two APIs like that
> still a no-no ?

You can, but I'd forbid it.  Mixing layers can only lead to tears later on.

Best would be to convert qemu_malloc()s to g_new()s and g_malloc()s to 
reduce confusion.

>
> (I'm thinking about a situation where you might use a glib utility
> function that returned g_malloc'd memory and want to pass that back
> to your caller without having to either copy to qemu_malloc'd memory
> or require your caller to care about the distinction.)
>

Changing ownership of memory is rare, I hope.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family

[Blue Swirl]

On Fri, Aug 19, 2011 at 3:22 PM, Avi Kivity <avi@redhat.com> wrote:
> On 08/18/2011 09:54 PM, Peter Maydell wrote:
>>
>> On 18 August 2011 18:48, Avi Kivity<avi@redhat.com>  wrote:
>> >  +static GMemVTable gmemvtable = {
>> >  +    .malloc = qemu_malloc,
>> >  +    .realloc = qemu_realloc,
>> >  +    .free = qemu_free,
>> >  +};
>> >  +
>> >  +/**
>> >  + * qemu_malloc_init: initialize memory management
>> >  + */
>> >  +void qemu_malloc_init(void)
>> >  +{
>> >  +    g_mem_set_vtable(&gmemvtable);
>> >  +}
>>
>> Does this mean you can now safely allocate with g_malloc
>> and free with qemu_free, or is mixing the two APIs like that
>> still a no-no ?
>
> You can, but I'd forbid it.  Mixing layers can only lead to tears later on.
>
> Best would be to convert qemu_malloc()s to g_new()s and g_malloc()s to
> reduce confusion.

Also plain malloc() and friends, except in tracing back end for obvious reasons.

>> (I'm thinking about a situation where you might use a glib utility
>> function that returned g_malloc'd memory and want to pass that back
>> to your caller without having to either copy to qemu_malloc'd memory
>> or require your caller to care about the distinction.)
>>
>
> Changing ownership of memory is rare, I hope.
>
> --
> I have a truly marvellous patch that fixes the bug which this
> signature is too narrow to contain.
>
>

Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family

[Anthony Liguori]

On 08/20/2011 01:59 AM, Blue Swirl wrote:
> On Fri, Aug 19, 2011 at 3:22 PM, Avi Kivity<avi@redhat.com>  wrote:
>> On 08/18/2011 09:54 PM, Peter Maydell wrote:
>>>
>>> On 18 August 2011 18:48, Avi Kivity<avi@redhat.com>    wrote:
>>>>   +static GMemVTable gmemvtable = {
>>>>   +    .malloc = qemu_malloc,
>>>>   +    .realloc = qemu_realloc,
>>>>   +    .free = qemu_free,
>>>>   +};
>>>>   +
>>>>   +/**
>>>>   + * qemu_malloc_init: initialize memory management
>>>>   + */
>>>>   +void qemu_malloc_init(void)
>>>>   +{
>>>>   +    g_mem_set_vtable(&gmemvtable);
>>>>   +}
>>>
>>> Does this mean you can now safely allocate with g_malloc
>>> and free with qemu_free, or is mixing the two APIs like that
>>> still a no-no ?
>>
>> You can, but I'd forbid it.  Mixing layers can only lead to tears later on.
>>
>> Best would be to convert qemu_malloc()s to g_new()s and g_malloc()s to
>> reduce confusion.
>
> Also plain malloc() and friends, except in tracing back end for obvious reasons.

sed script:

s:qemu_mallocz\( *\)(:g_malloc0\1\(:g
s:qemu_malloc\( *\)(:g_malloc\1\(:g
s:qemu_free\( *\)(:g_free\1\(:g
s:qemu_strdup\( *\)(:g_strdup\1\(:g
s:qemu_strndup\( *\)(:g_strndup\1\(:g

It takes a little build magic too to make sure everything has access to 
glib.

The patch is way too large to post.  Please speak now if you have an 
objection otherwise I'll commit in a couple days.

http://repo.or.cz/w/qemu/aliguori.git/commit/5cc99cedb46b3916dae8a565d5ffc5fb2f2e9bd6

If anyone wants to try it out first.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family

[Blue Swirl]

On Sun, Aug 21, 2011 at 3:11 AM, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 08/20/2011 01:59 AM, Blue Swirl wrote:
>>
>> On Fri, Aug 19, 2011 at 3:22 PM, Avi Kivity<avi@redhat.com>  wrote:
>>>
>>> On 08/18/2011 09:54 PM, Peter Maydell wrote:
>>>>
>>>> On 18 August 2011 18:48, Avi Kivity<avi@redhat.com>    wrote:
>>>>>
>>>>>  +static GMemVTable gmemvtable = {
>>>>>  +    .malloc = qemu_malloc,
>>>>>  +    .realloc = qemu_realloc,
>>>>>  +    .free = qemu_free,
>>>>>  +};
>>>>>  +
>>>>>  +/**
>>>>>  + * qemu_malloc_init: initialize memory management
>>>>>  + */
>>>>>  +void qemu_malloc_init(void)
>>>>>  +{
>>>>>  +    g_mem_set_vtable(&gmemvtable);
>>>>>  +}
>>>>
>>>> Does this mean you can now safely allocate with g_malloc
>>>> and free with qemu_free, or is mixing the two APIs like that
>>>> still a no-no ?
>>>
>>> You can, but I'd forbid it.  Mixing layers can only lead to tears later
>>> on.
>>>
>>> Best would be to convert qemu_malloc()s to g_new()s and g_malloc()s to
>>> reduce confusion.
>>
>> Also plain malloc() and friends, except in tracing back end for obvious
>> reasons.
>
> sed script:
>
> s:qemu_mallocz\( *\)(:g_malloc0\1\(:g
> s:qemu_malloc\( *\)(:g_malloc\1\(:g
> s:qemu_free\( *\)(:g_free\1\(:g
> s:qemu_strdup\( *\)(:g_strdup\1\(:g
> s:qemu_strndup\( *\)(:g_strndup\1\(:g

nih--;

Too bad GLib does not provide a function which gives aligned memory,
then also qemu_memalign() and maybe qemu_vmalloc() could be replaced.

The next step (or merged with this step) should be to replace plain
libc malloc/free/asprintf/strdup with
g_malloc/g_free/g_strdup_printf/g_strdup.

HACKING should be updated to refer to g_* versions instead of qemu_* functions.

> It takes a little build magic too to make sure everything has access to
> glib.
>
> The patch is way too large to post.  Please speak now if you have an
> objection otherwise I'll commit in a couple days.
>
> http://repo.or.cz/w/qemu/aliguori.git/commit/5cc99cedb46b3916dae8a565d5ffc5fb2f2e9bd6
>
> If anyone wants to try it out first.

I didn't test it but looks reasonable.

Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family

[Anthony Liguori]

On 08/21/2011 02:17 AM, Blue Swirl wrote:
> On Sun, Aug 21, 2011 at 3:11 AM, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>> On 08/20/2011 01:59 AM, Blue Swirl wrote:
>>>
>>> On Fri, Aug 19, 2011 at 3:22 PM, Avi Kivity<avi@redhat.com>    wrote:
>>>>
>>>> On 08/18/2011 09:54 PM, Peter Maydell wrote:
>>>>>
>>>>> On 18 August 2011 18:48, Avi Kivity<avi@redhat.com>      wrote:
>>>>>>
>>>>>>   +static GMemVTable gmemvtable = {
>>>>>>   +    .malloc = qemu_malloc,
>>>>>>   +    .realloc = qemu_realloc,
>>>>>>   +    .free = qemu_free,
>>>>>>   +};
>>>>>>   +
>>>>>>   +/**
>>>>>>   + * qemu_malloc_init: initialize memory management
>>>>>>   + */
>>>>>>   +void qemu_malloc_init(void)
>>>>>>   +{
>>>>>>   +    g_mem_set_vtable(&gmemvtable);
>>>>>>   +}
>>>>>
>>>>> Does this mean you can now safely allocate with g_malloc
>>>>> and free with qemu_free, or is mixing the two APIs like that
>>>>> still a no-no ?
>>>>
>>>> You can, but I'd forbid it.  Mixing layers can only lead to tears later
>>>> on.
>>>>
>>>> Best would be to convert qemu_malloc()s to g_new()s and g_malloc()s to
>>>> reduce confusion.
>>>
>>> Also plain malloc() and friends, except in tracing back end for obvious
>>> reasons.
>>
>> sed script:
>>
>> s:qemu_mallocz\( *\)(:g_malloc0\1\(:g
>> s:qemu_malloc\( *\)(:g_malloc\1\(:g
>> s:qemu_free\( *\)(:g_free\1\(:g
>> s:qemu_strdup\( *\)(:g_strdup\1\(:g
>> s:qemu_strndup\( *\)(:g_strndup\1\(:g
>
> nih--;
>
> Too bad GLib does not provide a function which gives aligned memory,
> then also qemu_memalign() and maybe qemu_vmalloc() could be replaced.

Indeed :-/

>
> The next step (or merged with this step) should be to replace plain
> libc malloc/free/asprintf/strdup with
> g_malloc/g_free/g_strdup_printf/g_strdup.

There's not a lot of these but they need to be audited individual to 
make sure that the frees correspond to mallocs.

>
> HACKING should be updated to refer to g_* versions instead of qemu_* functions.

That's included in the series.

>> It takes a little build magic too to make sure everything has access to
>> glib.
>>
>> The patch is way too large to post.  Please speak now if you have an
>> objection otherwise I'll commit in a couple days.
>>
>> http://repo.or.cz/w/qemu/aliguori.git/commit/5cc99cedb46b3916dae8a565d5ffc5fb2f2e9bd6
>>
>> If anyone wants to try it out first.
>
> I didn't test it but looks reasonable.

I've pushed so qemu_malloc is no more.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family

[Paolo Bonzini]

On 08/21/2011 03:24 PM, Anthony Liguori wrote:
> There's not a lot of these but they need to be audited individual to
> make sure that the frees correspond to mallocs.

I had patches for these in the qemu_malloc world.  I'll try to apply the 
sed script to the patches.

Paolo

Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 1704 bytes --]

On 08/18/2011 12:48 PM, Avi Kivity wrote:
> This makes the tracing infrastructure available to users of g_new().
>
> Signed-off-by: Avi Kivity<avi@redhat.com>

Here's my version, adapted to a world with no qemu_malloc.

Regards,

Anthony Liguori

> ---
>   qemu-common.h |    1 +
>   qemu-malloc.c |   15 +++++++++++++++
>   vl.c          |    1 +
>   3 files changed, 17 insertions(+), 0 deletions(-)
>
> diff --git a/qemu-common.h b/qemu-common.h
> index 74d5c4b..fbe2de0 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -180,6 +180,7 @@ const char *path(const char *pathname);
>   #define qemu_isascii(c)		isascii((unsigned char)(c))
>   #define qemu_toascii(c)		toascii((unsigned char)(c))
>
> +void qemu_malloc_init(void);
>   void *qemu_oom_check(void *ptr);
>   void *qemu_malloc(size_t size);
>   void *qemu_realloc(void *ptr, size_t size);
> diff --git a/qemu-malloc.c b/qemu-malloc.c
> index b9b3851..8b0c1ec 100644
> --- a/qemu-malloc.c
> +++ b/qemu-malloc.c
> @@ -24,6 +24,21 @@
>   #include "qemu-common.h"
>   #include "trace.h"
>   #include<stdlib.h>
> +#include<glib.h>
> +
> +static GMemVTable gmemvtable = {
> +    .malloc = qemu_malloc,
> +    .realloc = qemu_realloc,
> +    .free = qemu_free,
> +};
> +
> +/**
> + * qemu_malloc_init: initialize memory management
> + */
> +void qemu_malloc_init(void)
> +{
> +    g_mem_set_vtable(&gmemvtable);
> +}
>
>   void qemu_free(void *ptr)
>   {
> diff --git a/vl.c b/vl.c
> index c714127..7c4f8da 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -2106,6 +2106,7 @@ int main(int argc, char **argv, char **envp)
>
>       atexit(qemu_run_exit_notifiers);
>       error_set_progname(argv[0]);
> +    qemu_malloc_init();
>
>       init_clocks();
>


[-- Attachment #2: 0001-Add-trace-points-for-g_malloc-g_free-functions.patch --]
[-- Type: text/x-patch, Size: 1598 bytes --]

>From fd639107f8ee2a489a9010e007cc3181732b1f06 Mon Sep 17 00:00:00 2001
From: Anthony Liguori <aliguori@us.ibm.com>
Date: Sat, 20 Aug 2011 22:38:31 -0500
Subject: [PATCH] Add trace points for g_malloc/g_free functions

Derived from a patch submitted by Avi Kivity.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 vl.c |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index e9e14a6..c98961c 100644
--- a/vl.c
+++ b/vl.c
@@ -2075,6 +2075,26 @@ static const QEMUOption *lookup_opt(int argc, char **argv,
     return popt;
 }
 
+static gpointer malloc_and_trace(gsize n_bytes)
+{
+    void *ptr = malloc(n_bytes);
+    trace_qemu_malloc(n_bytes, ptr);
+    return ptr;
+}
+
+static gpointer realloc_and_trace(gpointer mem, gsize n_bytes)
+{
+    void *ptr = realloc(mem, n_bytes);
+    trace_qemu_realloc(mem, n_bytes, ptr);
+    return ptr;
+}
+
+static void free_and_trace(gpointer mem)
+{
+    trace_qemu_free(mem);
+    free(mem);
+}
+
 int main(int argc, char **argv, char **envp)
 {
     const char *gdbstub_dev = NULL;
@@ -2103,10 +2123,17 @@ int main(int argc, char **argv, char **envp)
     const char *trace_file = NULL;
     const char *log_mask = NULL;
     const char *log_file = NULL;
+    GMemVTable mem_trace = {
+        .malloc = malloc_and_trace,
+        .realloc = realloc_and_trace,
+        .free = free_and_trace,
+    };
 
     atexit(qemu_run_exit_notifiers);
     error_set_progname(argv[0]);
 
+    g_mem_set_vtable(&mem_trace);
+
     init_clocks();
 
     qemu_cache_utils_init(envp);
-- 
1.7.4.1