* [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family
@ 2011-08-18 17:48 Avi Kivity
2011-08-19 4:25 ` Stefan Hajnoczi
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Avi Kivity @ 2011-08-18 17:48 UTC (permalink / raw)
To: Anthony Liguori, Blue Swirl; +Cc: qemu-devel
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family
2011-08-18 17:48 [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family Avi Kivity
@ 2011-08-19 4:25 ` Stefan Hajnoczi
2011-08-19 4:54 ` Peter Maydell
2011-08-21 3:40 ` Anthony Liguori
2 siblings, 0 replies; 10+ messages in thread
From: Stefan Hajnoczi @ 2011-08-19 4:25 UTC (permalink / raw)
To: Avi Kivity; +Cc: Blue Swirl, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family
2011-08-18 17:48 [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family Avi Kivity
2011-08-19 4:25 ` Stefan Hajnoczi
@ 2011-08-19 4:54 ` Peter Maydell
2011-08-19 15:22 ` Avi Kivity
2011-08-21 3:40 ` Anthony Liguori
2 siblings, 1 reply; 10+ messages in thread
From: Peter Maydell @ 2011-08-19 4:54 UTC (permalink / raw)
To: Avi Kivity; +Cc: Blue Swirl, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family
2011-08-19 4:54 ` Peter Maydell
@ 2011-08-19 15:22 ` Avi Kivity
2011-08-20 6:59 ` Blue Swirl
0 siblings, 1 reply; 10+ messages in thread
From: Avi Kivity @ 2011-08-19 15:22 UTC (permalink / raw)
To: Peter Maydell; +Cc: Blue Swirl, qemu-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family
2011-08-19 15:22 ` Avi Kivity
@ 2011-08-20 6:59 ` Blue Swirl
2011-08-21 3:11 ` Anthony Liguori
0 siblings, 1 reply; 10+ messages in thread
From: Blue Swirl @ 2011-08-20 6:59 UTC (permalink / raw)
To: Avi Kivity; +Cc: Peter Maydell, qemu-devel
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.
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family
2011-08-20 6:59 ` Blue Swirl
@ 2011-08-21 3:11 ` Anthony Liguori
2011-08-21 7:17 ` Blue Swirl
0 siblings, 1 reply; 10+ messages in thread
From: Anthony Liguori @ 2011-08-21 3:11 UTC (permalink / raw)
To: Blue Swirl; +Cc: Peter Maydell, Avi Kivity, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family
2011-08-18 17:48 [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family Avi Kivity
2011-08-19 4:25 ` Stefan Hajnoczi
2011-08-19 4:54 ` Peter Maydell
@ 2011-08-21 3:40 ` Anthony Liguori
2 siblings, 0 replies; 10+ messages in thread
From: Anthony Liguori @ 2011-08-21 3:40 UTC (permalink / raw)
To: Avi Kivity; +Cc: Blue Swirl, qemu-devel
[-- 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family
2011-08-21 3:11 ` Anthony Liguori
@ 2011-08-21 7:17 ` Blue Swirl
2011-08-21 13:24 ` Anthony Liguori
0 siblings, 1 reply; 10+ messages in thread
From: Blue Swirl @ 2011-08-21 7:17 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Peter Maydell, Avi Kivity, qemu-devel
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family
2011-08-21 7:17 ` Blue Swirl
@ 2011-08-21 13:24 ` Anthony Liguori
2011-08-22 6:53 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Anthony Liguori @ 2011-08-21 13:24 UTC (permalink / raw)
To: Blue Swirl; +Cc: Peter Maydell, Avi Kivity, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family
2011-08-21 13:24 ` Anthony Liguori
@ 2011-08-22 6:53 ` Paolo Bonzini
0 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2011-08-22 6:53 UTC (permalink / raw)
To: Anthony Liguori; +Cc: Blue Swirl, Peter Maydell, Avi Kivity, qemu-devel
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-08-22 6:53 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-08-18 17:48 [Qemu-devel] [PATCH] Wire g_new() and friends to the qemu_malloc() family Avi Kivity
2011-08-19 4:25 ` Stefan Hajnoczi
2011-08-19 4:54 ` Peter Maydell
2011-08-19 15:22 ` Avi Kivity
2011-08-20 6:59 ` Blue Swirl
2011-08-21 3:11 ` Anthony Liguori
2011-08-21 7:17 ` Blue Swirl
2011-08-21 13:24 ` Anthony Liguori
2011-08-22 6:53 ` Paolo Bonzini
2011-08-21 3:40 ` Anthony Liguori
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 NNTP newsgroup(s).