Skip to contentSkip to conversation

xl: free bitmaps on exit

PATCH Superseded Olaf Hering 2 participants 6 messages Nov 27, 2018 – Nov 28, 2018

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Olaf Hering

author
#0 lore raw

PATCH v2 1/1 xl: free bitmaps on exit

Every invocation of xl via valgrind will show three leaks. Since libxl_bitmap_alloc uses NOGC, the caller has to free the memory after use. And since xl_ctx_free might be called before parse_global_config, also move the libxl_bitmap_init calls into xl_ctx_alloc.

Signed-off-by Olaf Hering <[email protected]>
---
 tools/xl/xl.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
tools/xl/xl.c +6 -3
index 7d2142f16f..f5a17cf0d1 100644
@@ -209,11 +209,8 @@ static void parse_global_config(const char *configfile,
     if (!xlu_cfg_get_long (config, "max_maptrack_frames", &l, 0))
         max_maptrack_frames = l;
 
-    libxl_bitmap_init(&global_vm_affinity_mask);
     libxl_cpu_bitmap_alloc(ctx, &global_vm_affinity_mask, 0);
-    libxl_bitmap_init(&global_hvm_affinity_mask);
     libxl_cpu_bitmap_alloc(ctx, &global_hvm_affinity_mask, 0);
-    libxl_bitmap_init(&global_pv_affinity_mask);
     libxl_cpu_bitmap_alloc(ctx, &global_pv_affinity_mask, 0);
 
     if (!xlu_cfg_get_string (config, "vm.cpumask", &buf, 0))

@@ -323,11 +320,17 @@ void xl_ctx_alloc(void) {
         exit(1);
     }
 
+    libxl_bitmap_init(&global_vm_affinity_mask);
+    libxl_bitmap_init(&global_hvm_affinity_mask);
+    libxl_bitmap_init(&global_pv_affinity_mask);
     libxl_childproc_setmode(ctx, &childproc_hooks, 0);
 }
 
 static void xl_ctx_free(void)
 {
+    libxl_bitmap_dispose(&global_pv_affinity_mask);
+    libxl_bitmap_dispose(&global_hvm_affinity_mask);
+    libxl_bitmap_dispose(&global_vm_affinity_mask);
     if (ctx) {
         libxl_ctx_free(ctx);
         ctx = NULL;

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

Acked-by on v2 1/1
#1 lore raw
Quoted 7 lines · Olaf Hering

Every invocation of xl via valgrind will show three leaks. Since libxl_bitmap_alloc uses NOGC, the caller has to free the memory after use. And since xl_ctx_free might be called before parse_global_config, also move the libxl_bitmap_init calls into xl_ctx_alloc.

Signed-off-by Olaf Hering <[email protected]>
Acked-by Wei Liu <[email protected]>

Thanks for fixing this.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#2 lore raw
In reply to Olaf Hering
Quoted 28 lines · Olaf Hering

Every invocation of xl via valgrind will show three leaks. Since libxl_bitmap_alloc uses NOGC, the caller has to free the memory after use. And since xl_ctx_free might be called before parse_global_config, also move the libxl_bitmap_init calls into xl_ctx_alloc.

Signed-off-by Olaf Hering <[email protected]>
---
 tools/xl/xl.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
tools/xl/xl.c +0 -3
index 7d2142f16f..f5a17cf0d1 100644
@@ -209,11 +209,8 @@ static void parse_global_config(const char *configfile,
     if (!xlu_cfg_get_long (config, "max_maptrack_frames", &l, 0))
         max_maptrack_frames = l;
 
-    libxl_bitmap_init(&global_vm_affinity_mask);
     libxl_cpu_bitmap_alloc(ctx, &global_vm_affinity_mask, 0);
-    libxl_bitmap_init(&global_hvm_affinity_mask);
     libxl_cpu_bitmap_alloc(ctx, &global_hvm_affinity_mask, 0);
-    libxl_bitmap_init(&global_pv_affinity_mask);
     libxl_cpu_bitmap_alloc(ctx, &global_pv_affinity_mask, 0);
 
     if (!xlu_cfg_get_string (config, "vm.cpumask", &buf, 0))

tools/xl/xl.c void xl_ctx_alloc(void) { · line 325

Quoted 6 lines
tools/xl/xl.c +3 -0
@@ -323,11 +320,17 @@ void xl_ctx_alloc(void) {
         exit(1);
     }
 
+    libxl_bitmap_init(&global_vm_affinity_mask);
+    libxl_bitmap_init(&global_hvm_affinity_mask);
+    libxl_bitmap_init(&global_pv_affinity_mask);

Looking more closely into this, these lines should be moved even earlier before the call to libxl_ctx_alloc -- there is an exit there.

Sorry for not having noticed this earlier before giving my ack. Please submit v3.

Wei.

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

author
#3 lore raw

Am Tue, 27 Nov 2018 16:55:38 +0000
schrieb Wei Liu <[email protected]>:

Quoted 2 lines

Looking more closely into this, these lines should be moved even earlier before the call to libxl_ctx_alloc -- there is an exit there.

Is it required to install the atexit handler before alloc()?
To me it looks like atexit(xl_ctx_free) should be done right after, or inside, xl_ctx_alloc().

Olaf

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel

#4 lore raw
Quoted 8 lines · Olaf Hering

Am Tue, 27 Nov 2018 16:55:38 +0000
schrieb Wei Liu <[email protected]>:

Quoted 2 lines

Looking more closely into this, these lines should be moved even earlier before the call to libxl_ctx_alloc -- there is an exit there.

Is it required to install the atexit handler before alloc()?
To me it looks like atexit(xl_ctx_free) should be done right after, or inside, xl_ctx_alloc().

Note that multiple calls to atexit creates a chain of functions to call, not overriding what is previously registered.

The xl_ctx_alloc function is also called in postfork. I think if you move atexit call into xl_ctx_alloc you will end up registering xl_ctx_free more than once, which is wrong.

The other option -- to call atexit right after -- looks plausible to me.

Wei.

Quoted 2 lines

Olaf

_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel