[PATCH v2] xen/timers: Fix memory leak with cpu unplug/plug

* [PATCH v2] xen/timers: Fix memory leak with cpu unplug/plug
@ 2019-04-08  9:39 Andrew Cooper
  2019-04-08  9:56 ` Jan Beulich
  2019-04-08 10:39 ` Julien Grall
  0 siblings, 2 replies; 9+ messages in thread
From: Andrew Cooper @ 2019-04-08  9:39 UTC (permalink / raw)
  To: Xen-devel
  Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
	George Dunlap, Andrew Cooper, Tim Deegan, Julien Grall,
	Jan Beulich, Ian Jackson

timer_softirq_action() realloc's itself a larger timer heap whenever
necessary, which includes bootstrapping from the empty dummy_heap.  Nothing
ever freed this allocation.

CPU plug and unplug has the side effect of zeroing the percpu data area, which
clears ts->heap.  This in turn causes new timers to be put on the list rather
than the heap, and for timer_softirq_action() to bootstrap itself again.

This in practice leaks ts->heap every time a CPU is unplugged and replugged.

Implement free_percpu_timers() which includes freeing ts->heap when
appropriate, and update the notifier callback with the recent cpu parking
logic and free-avoidance across suspend.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@citrix.com>
CC: Jan Beulich <JBeulich@suse.com>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Tim Deegan <tim@xen.org>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>

v2:
 * Rebase over Juergen's free-avoidance series.

This texturally depends on "xen/timers: Document and improve the
representation of the timer heap metadata" which was necessary to understand
the problem well enough to fix it, but isn't backporting over this change
isn't too complicated (should the cleanup patch not want to be backported).
---
 xen/common/timer.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/xen/common/timer.c b/xen/common/timer.c
index 98f2c48..f265a36 100644
--- a/xen/common/timer.c
+++ b/xen/common/timer.c
@@ -615,6 +615,22 @@ static void migrate_timers_from_cpu(unsigned int old_cpu)
  */
 static struct timer *dummy_heap[1];
 
+static void free_percpu_timers(unsigned int cpu)
+{
+    struct timers *ts = &per_cpu(timers, cpu);
+
+    migrate_timers_from_cpu(cpu);
+
+    ASSERT(heap_metadata(ts->heap)->size == 0);
+    if ( heap_metadata(ts->heap)->limit )
+    {
+        xfree(ts->heap);
+        ts->heap = dummy_heap;
+    }
+    else
+        ASSERT(ts->heap == dummy_heap);
+}
+
 static int cpu_callback(
     struct notifier_block *nfb, unsigned long action, void *hcpu)
 {
@@ -628,10 +644,19 @@ static int cpu_callback(
         spin_lock_init(&ts->lock);
         ts->heap = dummy_heap;
         break;
+
     case CPU_UP_CANCELED:
     case CPU_DEAD:
-        migrate_timers_from_cpu(cpu);
+    case CPU_RESUME_FAILED:
+        if ( !park_offline_cpus && system_state != SYS_STATE_suspend )
+            free_percpu_timers(cpu);
         break;
+
+    case CPU_REMOVE:
+        if ( park_offline_cpus )
+            free_percpu_timers(cpu);
+        break;
+
     default:
         break;
     }
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

^ permalink raw reply related	[flat|nested] 9+ messages in thread