On 08/30/2018 10:28 AM, Juergen Gross wrote:
> On 30/08/18 10:26, Jan Beulich wrote:
>>>>> On 30.08.18 at 09:52, <jgr...@suse.com> wrote:
>>> @@ -202,7 +202,7 @@ static int alloc_trace_bufs(unsigned int pages)
>>> * Allocate buffers for all of the cpus.
>>> * If any fails, deallocate what you have so far and exit.
>>> */
>>> - for_each_online_cpu(cpu)
>>> + for_each_present_cpu(cpu)
>>> {
>>> offset = t_info_first_offset + (cpu * pages);
>>> t_info->mfn_offset[cpu] = offset;
>>
>> Doesn't this go a little too far? Why would you allocate buffers for CPUs
>> which can never be brought online? There ought to be a middle ground,
>> where online-able CPUs have buffers allocated, but non-online-able ones
>> won't. On larger systems I guess the difference may be quite noticable.
>
> According to the comments in include/xen/cpumask.h cpu_present_map
> represents the populated cpus.
>
> I know that currently there is no support for onlining a parked cpu
> again, but I think having to think about Xentrace buffer allocation in
> case onlining of parked cpus is added would be a nearly 100% chance to
> introduce a bug.
>
> Xentrace is used for testing purposes only. So IMHO allocating some more
> memory is acceptable.
On the other hand, size of buffers can be a significant factor in
whether you can manage to catch the behavior you want or whether there
will be gaps due to dropped records; anything that can make those
allocations go farther will be helpful.
Anyone bringing a parked cpu back up should have to go through all
instances of per_cpu() and figure out if they need to do something; that
should catch this issue, if it happens.
So although it's a bit of a tough decision, I'd leave this
for_each_online_cpu(), as Jan suggests.
Thanks,
-George
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
