1. Patchew
  2. QEMU
  3. View series

[Qemu-devel] [RFC PATCH for 4.1] linux-user: unparent CPU object before unref

Alex Bennée posted 1 patch 4 years, 9 months ago
Test FreeBSD passed
Test docker-mingw@fedora passed
Test docker-clang@ubuntu passed
Test checkpatch passed
Test s390x passed
Test asan passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20190716140133.8578-1-alex.bennee@linaro.org
linux-user/syscall.c | 1 +
1 file changed, 1 insertion(+)
[Qemu-devel] [RFC PATCH for 4.1] linux-user: unparent CPU object before unref
Posted by Alex Bennée 4 years, 9 months ago 
When a CPU object is created it is parented during it's realize stage.
If we don't unparent before the "final" unref we will never finzalize
the object leading to a memory leak. For most setups you probably
won't notice but with anything that creates and destroys a lot of
threads this will add up. This goes especially for architectures which
allocate a lot of memory in their CPU structures.

Fixes: https://bugs.launchpad.net/qemu/+bug/1836558
Cc: 1836558@bugs.launchpad.net
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 linux-user/syscall.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 39a37496fed..4c9313fd9d0 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -7183,6 +7183,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
                           NULL, NULL, 0);
             }
             thread_cpu = NULL;
+            object_unparent(OBJECT(cpu));
             object_unref(OBJECT(cpu));
             g_free(ts);
             rcu_unregister_thread();
-- 
2.20.1
Re: [Qemu-devel] [RFC PATCH for 4.1] linux-user: unparent CPU object before unref
Posted by Philippe Mathieu-Daudé 4 years, 9 months ago 
On 7/16/19 4:01 PM, Alex Bennée wrote:
> When a CPU object is created it is parented during it's realize stage.
> If we don't unparent before the "final" unref we will never finzalize

"finalize"

> the object leading to a memory leak. For most setups you probably
> won't notice but with anything that creates and destroys a lot of
> threads this will add up. This goes especially for architectures which
> allocate a lot of memory in their CPU structures.
> 
> Fixes: https://bugs.launchpad.net/qemu/+bug/1836558
> Cc: 1836558@bugs.launchpad.net
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  linux-user/syscall.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 39a37496fed..4c9313fd9d0 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7183,6 +7183,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>                            NULL, NULL, 0);
>              }
>              thread_cpu = NULL;
> +            object_unparent(OBJECT(cpu));
>              object_unref(OBJECT(cpu));
>              g_free(ts);
>              rcu_unregister_thread();
>
Re: [Qemu-devel] [RFC PATCH for 4.1] linux-user: unparent CPU object before unref
Posted by Peter Maydell 4 years, 9 months ago 
Ccing the QOM maintainers to make sure we have the
QOM lifecycle operations right here...

On Tue, 16 Jul 2019 at 15:02, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> When a CPU object is created it is parented during it's realize stage.
> If we don't unparent before the "final" unref we will never finzalize
> the object leading to a memory leak. For most setups you probably
> won't notice but with anything that creates and destroys a lot of
> threads this will add up. This goes especially for architectures which
> allocate a lot of memory in their CPU structures.
>
> Fixes: https://bugs.launchpad.net/qemu/+bug/1836558
> Cc: 1836558@bugs.launchpad.net
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  linux-user/syscall.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 39a37496fed..4c9313fd9d0 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -7183,6 +7183,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>                            NULL, NULL, 0);
>              }
>              thread_cpu = NULL;
> +            object_unparent(OBJECT(cpu));
>              object_unref(OBJECT(cpu));
>              g_free(ts);
>              rcu_unregister_thread();

I think (as I mentioned on IRC) that we also need to unrealize
the CPU object, because target/ppc at least does some freeing
of memory in its unrealize method. I think we do that by
setting the QOM "realize" property back to "false" -- but that
might barf if we try it on a CPU that isn't hotpluggable...

thanks
-- PMM
Re: [Qemu-devel] [RFC PATCH for 4.1] linux-user: unparent CPU object before unref
Posted by Alex Bennée 4 years, 9 months ago 
Peter Maydell <peter.maydell@linaro.org> writes:

> Ccing the QOM maintainers to make sure we have the
> QOM lifecycle operations right here...
>
> On Tue, 16 Jul 2019 at 15:02, Alex Bennée <alex.bennee@linaro.org> wrote:
>>
>> When a CPU object is created it is parented during it's realize stage.
>> If we don't unparent before the "final" unref we will never finzalize
>> the object leading to a memory leak. For most setups you probably
>> won't notice but with anything that creates and destroys a lot of
>> threads this will add up. This goes especially for architectures which
>> allocate a lot of memory in their CPU structures.
>>
>> Fixes: https://bugs.launchpad.net/qemu/+bug/1836558
>> Cc: 1836558@bugs.launchpad.net
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>> ---
>>  linux-user/syscall.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 39a37496fed..4c9313fd9d0 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -7183,6 +7183,7 @@ static abi_long do_syscall1(void *cpu_env, int num, abi_long arg1,
>>                            NULL, NULL, 0);
>>              }
>>              thread_cpu = NULL;
>> +            object_unparent(OBJECT(cpu));
>>              object_unref(OBJECT(cpu));
>>              g_free(ts);
>>              rcu_unregister_thread();
>
> I think (as I mentioned on IRC) that we also need to unrealize
> the CPU object, because target/ppc at least does some freeing
> of memory in its unrealize method. I think we do that by
> setting the QOM "realize" property back to "false" -- but that
> might barf if we try it on a CPU that isn't hotpluggable...

I have tried:

             thread_cpu = NULL;
+            object_unparent(OBJECT(cpu));
+            object_property_set_bool(OBJECT(cpu), false, "realized", NULL);
             object_unref(OBJECT(cpu));

but it didn't manifestly change anything (i.e. both with and without
setting realized the thread allocated stuff is freed). Valgrind still
complains about:

==22483== 6,656 bytes in 26 blocks are possibly lost in loss record 1,639 of 1,654
==22483==    at 0x483577F: malloc (vg_replace_malloc.c:299)
==22483==    by 0x4D7F8D0: g_malloc (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.5800.3)
==22483==    by 0x27D692: create_new_table (translate_init.inc.c:9252)
==22483==    by 0x27D7CD: register_ind_in_table (translate_init.inc.c:9291)
==22483==    by 0x27D975: register_dblind_insn (translate_init.inc.c:9337)
==22483==    by 0x27DBBB: register_insn (translate_init.inc.c:9384)
==22483==    by 0x27DE4E: create_ppc_opcodes (translate_init.inc.c:9449)
==22483==    by 0x27E79C: ppc_cpu_realize (translate_init.inc.c:9818)
==22483==    by 0x2C6FE8: device_set_realized (qdev.c:834)
==22483==    by 0x2D1E3D: property_set_bool (object.c:2076)
==22483==    by 0x2D00B3: object_property_set (object.c:1268)
==22483==    by 0x2D3185: object_property_set_qobject (qom-qobject.c:26)

But I don't know if that is just because the final exit_group of the
main CPU just exits without attempting to clean-up. However it is better
than it was.

--
Alex Bennée

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.