| Message ID | 20160719085432.4572-19-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 07/19/2016 11:54 AM, marcandre.lureau@redhat.com wrote: > From: Marc-André Lureau <marcandre.lureau@redhat.com> > > The free_ranges array is used as a temporary pointer array, the segment > should still be freed, Right. If I understand, this is the leak. however, it shouldn't free the elements themself. And it didn't, right? otherwise it would not work since these ranges are used later. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/i386/acpi-build.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index fbba461..f4ba3a4 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -761,7 +761,7 @@ static gint crs_range_compare(gconstpointer a, gconstpointer b) > static void crs_replace_with_free_ranges(GPtrArray *ranges, > uint64_t start, uint64_t end) > { > - GPtrArray *free_ranges = g_ptr_array_new_with_free_func(crs_range_free); > + GPtrArray *free_ranges = g_ptr_array_new(); Indeed, we are not going to free the ranges in this array, adding the GDestroyNotify here is not needed. > uint64_t free_base = start; > int i; > > @@ -785,7 +785,7 @@ static void crs_replace_with_free_ranges(GPtrArray *ranges, > g_ptr_array_add(ranges, g_ptr_array_index(free_ranges, i)); > } > > - g_ptr_array_free(free_ranges, false); > + g_ptr_array_free(free_ranges, true); This *is* scary since "true" means delete everything, but looking at documentation: "If array contents point to dynamically-allocated memory, they should be freed separately if free_seg is TRUE and no GDestroyNotify function has been set for array." So your approach should work. I think I understand the leak. Previous approach deleted the GArray wrapper, preserved the pointers (which we need), but also the inner array which we don't. One question: how did you test that it still works :) ? Did you run something like -device pxb,id=pxb,bus_nr=0x80,bus=pci.0 -device e1000,bus=pxb and see the device e100 device gets the required resources? Thanks, Marcel > } > > /* >
Hi On Thu, Jul 21, 2016 at 6:52 PM, Marcel Apfelbaum <marcel.apfelbaum@gmail.com> wrote: > On 07/19/2016 11:54 AM, marcandre.lureau@redhat.com wrote: >> >> From: Marc-André Lureau <marcandre.lureau@redhat.com> >> >> The free_ranges array is used as a temporary pointer array, the segment >> should still be freed, > > > Right. If I understand, this is the leak. > > however, it shouldn't free the elements themself. > > And it didn't, right? otherwise it would not work since these ranges > are used later. > >> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >> --- >> hw/i386/acpi-build.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >> index fbba461..f4ba3a4 100644 >> --- a/hw/i386/acpi-build.c >> +++ b/hw/i386/acpi-build.c >> @@ -761,7 +761,7 @@ static gint crs_range_compare(gconstpointer a, >> gconstpointer b) >> static void crs_replace_with_free_ranges(GPtrArray *ranges, >> uint64_t start, uint64_t end) >> { >> - GPtrArray *free_ranges = >> g_ptr_array_new_with_free_func(crs_range_free); >> + GPtrArray *free_ranges = g_ptr_array_new(); > > > Indeed, we are not going to free the ranges in this array, adding the > GDestroyNotify > here is not needed. > >> uint64_t free_base = start; >> int i; >> >> @@ -785,7 +785,7 @@ static void crs_replace_with_free_ranges(GPtrArray >> *ranges, >> g_ptr_array_add(ranges, g_ptr_array_index(free_ranges, i)); >> } >> >> - g_ptr_array_free(free_ranges, false); >> + g_ptr_array_free(free_ranges, true); > > > This *is* scary since "true" means delete everything, but looking at > documentation: > "If array contents point to dynamically-allocated memory, > they should be freed separately if free_seg is TRUE and > no GDestroyNotify function has been set for array." > So your approach should work. > > I think I understand the leak. Previous approach deleted the GArray wrapper, > preserved the pointers (which we need), but also the inner array which we > don't. yes, it's only the inner array we need to free. > > One question: how did you test that it still works :) ? > Did you run something like -device pxb,id=pxb,bus_nr=0x80,bus=pci.0 -device > e1000,bus=pxb and see the device > e100 device gets the required resources? If you run this under it valgrind you get, after the patch the leak is gone: ==20313== 32 bytes in 1 blocks are definitely lost in loss record 5,326 of 10,190 ==20313== at 0x4C2BBAD: malloc (vg_replace_malloc.c:299) ==20313== by 0x1E16AE58: g_malloc (in /usr/lib64/libglib-2.0.so.0.4800.1) ==20313== by 0x1E181D42: g_slice_alloc (in /usr/lib64/libglib-2.0.so.0.4800.1) ==20313== by 0x1E139880: g_ptr_array_sized_new (in /usr/lib64/libglib-2.0.so.0.4800.1) ==20313== by 0x1E13991D: g_ptr_array_new_with_free_func (in /usr/lib64/libglib-2.0.so.0.4800.1) ==20313== by 0x3E8BE8: crs_range_merge (acpi-build.c:797) ==20313== by 0x3E8FE6: build_crs (acpi-build.c:918) ==20313== by 0x3ED857: build_dsdt (acpi-build.c:2014) ==20313== by 0x3EF659: acpi_build (acpi-build.c:2590) ==20313== by 0x3EFE61: acpi_setup (acpi-build.c:2793) ==20313== by 0x3D810D: pc_machine_done (pc.c:1270) ==20313== by 0x7E6A23: notifier_list_notify (notify.c:40) > > > Thanks, > Marcel > >> } >> >> /* >> > >
On 07/21/2016 06:48 PM, Marc-André Lureau wrote: > Hi > > On Thu, Jul 21, 2016 at 6:52 PM, Marcel Apfelbaum > <marcel.apfelbaum@gmail.com> wrote: >> On 07/19/2016 11:54 AM, marcandre.lureau@redhat.com wrote: >>> >>> From: Marc-André Lureau <marcandre.lureau@redhat.com> >>> >>> The free_ranges array is used as a temporary pointer array, the segment >>> should still be freed, >> >> >> Right. If I understand, this is the leak. >> >> however, it shouldn't free the elements themself. >> >> And it didn't, right? otherwise it would not work since these ranges >> are used later. >> >>> >>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>> --- >>> hw/i386/acpi-build.c | 4 ++-- >>> 1 file changed, 2 insertions(+), 2 deletions(-) >>> >>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >>> index fbba461..f4ba3a4 100644 >>> --- a/hw/i386/acpi-build.c >>> +++ b/hw/i386/acpi-build.c >>> @@ -761,7 +761,7 @@ static gint crs_range_compare(gconstpointer a, >>> gconstpointer b) >>> static void crs_replace_with_free_ranges(GPtrArray *ranges, >>> uint64_t start, uint64_t end) >>> { >>> - GPtrArray *free_ranges = >>> g_ptr_array_new_with_free_func(crs_range_free); >>> + GPtrArray *free_ranges = g_ptr_array_new(); >> >> >> Indeed, we are not going to free the ranges in this array, adding the >> GDestroyNotify >> here is not needed. >> >>> uint64_t free_base = start; >>> int i; >>> >>> @@ -785,7 +785,7 @@ static void crs_replace_with_free_ranges(GPtrArray >>> *ranges, >>> g_ptr_array_add(ranges, g_ptr_array_index(free_ranges, i)); >>> } >>> >>> - g_ptr_array_free(free_ranges, false); >>> + g_ptr_array_free(free_ranges, true); >> >> >> This *is* scary since "true" means delete everything, but looking at >> documentation: >> "If array contents point to dynamically-allocated memory, >> they should be freed separately if free_seg is TRUE and >> no GDestroyNotify function has been set for array." >> So your approach should work. >> >> I think I understand the leak. Previous approach deleted the GArray wrapper, >> preserved the pointers (which we need), but also the inner array which we >> don't. > > yes, it's only the inner array we need to free. > >> >> One question: how did you test that it still works :) ? >> Did you run something like -device pxb,id=pxb,bus_nr=0x80,bus=pci.0 -device >> e1000,bus=pxb and see the device >> e100 device gets the required resources? > > If you run this under it valgrind you get, after the patch the leak is gone: > > ==20313== 32 bytes in 1 blocks are definitely lost in loss record > 5,326 of 10,190 > ==20313== at 0x4C2BBAD: malloc (vg_replace_malloc.c:299) > ==20313== by 0x1E16AE58: g_malloc (in /usr/lib64/libglib-2.0.so.0.4800.1) > ==20313== by 0x1E181D42: g_slice_alloc (in > /usr/lib64/libglib-2.0.so.0.4800.1) > ==20313== by 0x1E139880: g_ptr_array_sized_new (in > /usr/lib64/libglib-2.0.so.0.4800.1) > ==20313== by 0x1E13991D: g_ptr_array_new_with_free_func (in > /usr/lib64/libglib-2.0.so.0.4800.1) > ==20313== by 0x3E8BE8: crs_range_merge (acpi-build.c:797) > ==20313== by 0x3E8FE6: build_crs (acpi-build.c:918) > ==20313== by 0x3ED857: build_dsdt (acpi-build.c:2014) > ==20313== by 0x3EF659: acpi_build (acpi-build.c:2590) > ==20313== by 0x3EFE61: acpi_setup (acpi-build.c:2793) > ==20313== by 0x3D810D: pc_machine_done (pc.c:1270) > ==20313== by 0x7E6A23: notifier_list_notify (notify.c:40) > > > I am sure the leak is gone :), but the devices attached to PXB still work? I'll test it and get back to you. Thanks, Marcel >> >> >> Thanks, >> Marcel >> >>> } >>> >>> /* >>> >> >> > > >
On 07/21/2016 06:51 PM, Marcel Apfelbaum wrote: > On 07/21/2016 06:48 PM, Marc-André Lureau wrote: >> Hi >> >> On Thu, Jul 21, 2016 at 6:52 PM, Marcel Apfelbaum >> <marcel.apfelbaum@gmail.com> wrote: >>> On 07/19/2016 11:54 AM, marcandre.lureau@redhat.com wrote: >>>> >>>> From: Marc-André Lureau <marcandre.lureau@redhat.com> >>>> >>>> The free_ranges array is used as a temporary pointer array, the segment >>>> should still be freed, >>> >>> >>> Right. If I understand, this is the leak. >>> >>> however, it shouldn't free the elements themself. >>> >>> And it didn't, right? otherwise it would not work since these ranges >>> are used later. >>> >>>> >>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> >>>> --- >>>> hw/i386/acpi-build.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c >>>> index fbba461..f4ba3a4 100644 >>>> --- a/hw/i386/acpi-build.c >>>> +++ b/hw/i386/acpi-build.c >>>> @@ -761,7 +761,7 @@ static gint crs_range_compare(gconstpointer a, >>>> gconstpointer b) >>>> static void crs_replace_with_free_ranges(GPtrArray *ranges, >>>> uint64_t start, uint64_t end) >>>> { >>>> - GPtrArray *free_ranges = >>>> g_ptr_array_new_with_free_func(crs_range_free); >>>> + GPtrArray *free_ranges = g_ptr_array_new(); >>> >>> >>> Indeed, we are not going to free the ranges in this array, adding the >>> GDestroyNotify >>> here is not needed. >>> >>>> uint64_t free_base = start; >>>> int i; >>>> >>>> @@ -785,7 +785,7 @@ static void crs_replace_with_free_ranges(GPtrArray >>>> *ranges, >>>> g_ptr_array_add(ranges, g_ptr_array_index(free_ranges, i)); >>>> } >>>> >>>> - g_ptr_array_free(free_ranges, false); >>>> + g_ptr_array_free(free_ranges, true); >>> >>> >>> This *is* scary since "true" means delete everything, but looking at >>> documentation: >>> "If array contents point to dynamically-allocated memory, >>> they should be freed separately if free_seg is TRUE and >>> no GDestroyNotify function has been set for array." >>> So your approach should work. >>> >>> I think I understand the leak. Previous approach deleted the GArray wrapper, >>> preserved the pointers (which we need), but also the inner array which we >>> don't. >> >> yes, it's only the inner array we need to free. >> >>> >>> One question: how did you test that it still works :) ? >>> Did you run something like -device pxb,id=pxb,bus_nr=0x80,bus=pci.0 -device >>> e1000,bus=pxb and see the device >>> e100 device gets the required resources? >> >> If you run this under it valgrind you get, after the patch the leak is gone: >> >> ==20313== 32 bytes in 1 blocks are definitely lost in loss record >> 5,326 of 10,190 >> ==20313== at 0x4C2BBAD: malloc (vg_replace_malloc.c:299) >> ==20313== by 0x1E16AE58: g_malloc (in /usr/lib64/libglib-2.0.so.0.4800.1) >> ==20313== by 0x1E181D42: g_slice_alloc (in >> /usr/lib64/libglib-2.0.so.0.4800.1) >> ==20313== by 0x1E139880: g_ptr_array_sized_new (in >> /usr/lib64/libglib-2.0.so.0.4800.1) >> ==20313== by 0x1E13991D: g_ptr_array_new_with_free_func (in >> /usr/lib64/libglib-2.0.so.0.4800.1) >> ==20313== by 0x3E8BE8: crs_range_merge (acpi-build.c:797) >> ==20313== by 0x3E8FE6: build_crs (acpi-build.c:918) >> ==20313== by 0x3ED857: build_dsdt (acpi-build.c:2014) >> ==20313== by 0x3EF659: acpi_build (acpi-build.c:2590) >> ==20313== by 0x3EFE61: acpi_setup (acpi-build.c:2793) >> ==20313== by 0x3D810D: pc_machine_done (pc.c:1270) >> ==20313== by 0x7E6A23: notifier_list_notify (notify.c:40) >> >> >> > > I am sure the leak is gone :), but the devices attached to PXB still work? > I'll test it and get back to you. Everything works, thank you! Tested-by: Marcel Apfelbaum <marcel@redhat.com> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com> > > Thanks, > Marcel > > > >>> >>> >>> Thanks, >>> Marcel >>> >>>> } >>>> >>>> /* >>>> >>> >>> >> >> >> > --
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index fbba461..f4ba3a4 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -761,7 +761,7 @@ static gint crs_range_compare(gconstpointer a, gconstpointer b) static void crs_replace_with_free_ranges(GPtrArray *ranges, uint64_t start, uint64_t end) { - GPtrArray *free_ranges = g_ptr_array_new_with_free_func(crs_range_free); + GPtrArray *free_ranges = g_ptr_array_new(); uint64_t free_base = start; int i; @@ -785,7 +785,7 @@ static void crs_replace_with_free_ranges(GPtrArray *ranges, g_ptr_array_add(ranges, g_ptr_array_index(free_ranges, i)); } - g_ptr_array_free(free_ranges, false); + g_ptr_array_free(free_ranges, true); } /*