Re: [Xen-devel] [PATCH] xen/timers: Fix memory leak with cpu hot unplug

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 01/04/2019 09:27, Jan Beulich wrote:
>>>> On 29.03.19 at 17:57, <andrew.cooper3@xxxxxxxxxx> wrote:
>> 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 hot unplug and plug 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 hot unplugged and
>> replugged.  In the cpu notifier, free the heap after migrating all other
>> timers away.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Could I talk you into using online/offline instead of plug/unplug,
> as the latter is pretty tied to the physical operation of adding /
> removing a CPU to / from a system, whereas the issue here
> also surfaces for pure software actions like suspend/resume
> or the xen-hptool operations?

Ok.

>
>> --- a/xen/common/timer.c
>> +++ b/xen/common/timer.c
>> @@ -631,6 +631,10 @@ static int cpu_callback(
>>      case CPU_UP_CANCELED:
>>      case CPU_DEAD:
>>          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;
>>          break;
> I think it would be worthwhile to add a comment to clarify that
> the updating of per-CPU data here is not entirely pointless,
> despite the zeroing done when bringing a CPU back up.

What kind of comment do you think would be useful here? 

ts->heap obviously shouldn’t be left as a wild pointer, and I don't see
why this point is worthy of comment.

>
> Additionally - is this really the right thing to do uniformly during
> CPU_UP_CANCELED / CPU_DEAD? Shouldn't this be done
> conditionally upon park_offline_cpus there, and get done for
> CPU_REMOVE in the opposite case (like we do elsewhere, in
> particular in cpu_percpu_callback() itself)?

Its certainly the safest course of action, and obviously needs to follow
migrate_timers_from_cpu()

Do parked CPUs actually get entered into the online map?  It appears so.

Either way, the actual semantics of park_offline_cpus are undocumented,
and if the intended semantics are to use that bifurcated logic
everywhere, then I think the semantics want rethinking.

~Andrew

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