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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 01.04.19 at 18:59, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 01/04/2019 09:27, Jan Beulich wrote:
>>>>> On 29.03.19 at 17:57, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> --- 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.

Oh, I wasn't suggesting to comment the freeing. It's the storing
of dummy_heap's address which might look unnecessary when
being aware of the clearing of per-CPU data.

>> 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.

No, they don't: It's a full cpu_down() they go through. That's
why the new CPU_REMOVE notification was introduced, for
handlers to be able to free per-CPU data at the appropriate
point in time.

> 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.

Well, I don't mind finding a different, perhaps easier to use
model. Back then, when introducing parking, I couldn't think
of one.

Jan


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