| Message ID | 20220218164646.132112-2-damien.hedde@greensocs.com |
|---|---|
| State | New |
| Headers | show |
| Series | RiscV cleanups for user-related life cycles | expand |
On Fri, 18 Feb 2022 at 16:53, Damien Hedde <damien.hedde@greensocs.com> wrote: > > The array is dynamically allocated by realize() depending on the > number of harts. > > This clean-up removes memory leaks which would happen in the > 'init->finalize' life-cycle use-case (happening when user creation > is allowed). If the allocation happens in realize, then it won't hapen in an init->finalize cycle, only in init->realize->unrealize->finalize... -- PMM
On 2/18/22 18:23, Peter Maydell wrote: > On Fri, 18 Feb 2022 at 16:53, Damien Hedde <damien.hedde@greensocs.com> wrote: >> >> The array is dynamically allocated by realize() depending on the >> number of harts. >> >> This clean-up removes memory leaks which would happen in the >> 'init->finalize' life-cycle use-case (happening when user creation >> is allowed). > > If the allocation happens in realize, then it won't hapen > in an init->finalize cycle, only in init->realize->unrealize->finalize... > > -- PMM You're right. I was confused when re-writing the message. This leaks happen on init -> realize-failure -> finalize Because the array is allocated, then every cpu is initialized (and an error failure may happen for any of them). Thanks, -- Damien
On Fri, 18 Feb 2022 at 17:39, Damien Hedde <damien.hedde@greensocs.com> wrote: > You're right. I was confused when re-writing the message. > This leaks happen on > init -> realize-failure -> finalize > Because the array is allocated, then every cpu is initialized (and an > error failure may happen for any of them). "Failure during realize" is one of those cases I don't think we handle very well. I'd like to see a view by one of our QOM experts on what the best way to handle this is -- should one do the cleanup in realize itself, or in instance_finalize? Do the sub-objects that are being initialized and realized need to be manually cleaned up in the realize-is-failing case, or is that part automatic? Which is to say that maybe this patch is the best way to do this, but it would be nice to be sure about that... thanks -- PMM
On 2/18/22 18:46, Peter Maydell wrote: > On Fri, 18 Feb 2022 at 17:39, Damien Hedde <damien.hedde@greensocs.com> wrote: >> You're right. I was confused when re-writing the message. >> This leaks happen on >> init -> realize-failure -> finalize >> Because the array is allocated, then every cpu is initialized (and an >> error failure may happen for any of them). > > "Failure during realize" is one of those cases I don't think we > handle very well. I'd like to see a view by one of our QOM experts > on what the best way to handle this is -- should one do the > cleanup in realize itself, or in instance_finalize? Do the > sub-objects that are being initialized and realized need to > be manually cleaned up in the realize-is-failing case, or is > that part automatic? > > Which is to say that maybe this patch is the best way to do this, > but it would be nice to be sure about that... > Right now we have only 3 life cycles for coldplug (for hotplug it may be a bit different, I don't know but that's not the case of devices in this series): + user help: init -> finalize + failed creation: init -> realize-failure -> finalize + normal cycle: init -> realize-success -> ... (maybe finalize at qemu end) I'm not even sure unrealize() is called on the normal cycle, at least it is not done by DEVICE's finalize() method. We could try to be clean at the end of realize failure, but anyway it's better to free memory at the end. So we'll have to do it in finalize() too (or in a unrealize(), if we have guarantee it will be called). It look simplier to do it in finalize(), it catches all use cases. Note that we also have two cases of memory allocation: + the allocated space hosts some children objects (like in the riscv_array of this patch) + the allocated space hosts simple C things (other patches of this series) First case is bit a tricky because obviously we cannot free a child's memory until the child is also finalized. To answer your last question, cleaning of a child is automatic when the reference is removed at the beginning the finalize process of the parent. Writing the last paragraph makes me realize we have probably no guarantee this is the last reference (but it should). So if we want to be sure we don't free a still-in-use memory space: we should use object_new() instead of object_initialize_child() and let the child free it's own memory and avoid this specific issue. PS: Added qom maintainers/reviewers in cc. Thanks, -- Damien
diff --git a/hw/riscv/riscv_hart.c b/hw/riscv/riscv_hart.c index 613ea2aaa0..4aed6c2a59 100644 --- a/hw/riscv/riscv_hart.c +++ b/hw/riscv/riscv_hart.c @@ -66,6 +66,13 @@ static void riscv_harts_realize(DeviceState *dev, Error **errp) } } +static void riscv_harts_finalize(Object *obj) +{ + RISCVHartArrayState *s = RISCV_HART_ARRAY(obj); + + g_free(s->harts); +} + static void riscv_harts_class_init(ObjectClass *klass, void *data) { DeviceClass *dc = DEVICE_CLASS(klass); @@ -79,6 +86,7 @@ static const TypeInfo riscv_harts_info = { .parent = TYPE_SYS_BUS_DEVICE, .instance_size = sizeof(RISCVHartArrayState), .class_init = riscv_harts_class_init, + .instance_finalize = riscv_harts_finalize, }; static void riscv_harts_register_types(void)
The array is dynamically allocated by realize() depending on the number of harts. This clean-up removes memory leaks which would happen in the 'init->finalize' life-cycle use-case (happening when user creation is allowed). Signed-off-by: Damien Hedde <damien.hedde@greensocs.com> --- hw/riscv/riscv_hart.c | 8 ++++++++ 1 file changed, 8 insertions(+)