Re: [RFC PATCH 3/8] perf: Init/fini PMU specific data

[Liang, Kan]

On 12/2/2019 2:15 PM, Andi Kleen wrote:
> On Mon, Dec 02, 2019 at 05:21:52PM +0100, Peter Zijlstra wrote:
> > On Mon, Dec 02, 2019 at 06:59:57AM -0800, Andi Kleen wrote:
> > > > This is atricous crap. Also it is completely broken for -RT.
> > >
> > > Well can you please suggest how you would implement it instead?
> >
> > I don't think that is on me; at best I get to explain why it is
>
> Normally code review is expected to be constructive.
>
> > completely unacceptible to have O(nr_tasks) and allocations under a
> > raw_spinlock_t, but I was thinking you'd already know that.
>
> Ok if that's the only problem then a lock breaker + retry
> if rescheduling is needed + some limit against live lock
> should be sufficient.

OK. I will move the allocation out of critical sections.
Here is some pseudo code.

if (atomic_read(&nr_task_data_events))
	return;

//get current number of threads
read_lock(&tasklist_lock);
for_each_process_thread(g, p)
	num_thread++;
read_unlock(&tasklist_lock);

//allocate the space for them
for (i = 0; i < num_thread; i++)
	data[i] = kzalloc(ctx_size, flags);
i = 0;

/*
 * Assign the space to tasks
 * There may be some new threads created when we allocate space.
 * new_task will track its number.
 */
raw_spin_lock_irqsave(&task_data_events_lock, flags);

if (atomic_inc_return(&nr_task_data_events) > 1)
	goto unlock;

for_each_process_thread(g, p) {
	if (i < num_thread)
		p->perf_ctx_data = data[i++];
	else
		new_task++;
}
raw_spin_unlock_irqrestore(&task_data_events_lock, flags);

if (i < num_thread)
	goto end;

/*
 * Try again to allocate the space for the task created when
 * we first allocate space.
 * We don't need to worry about the task created after
 * atomic_inc_return(). It will be handled in perf_event_fork().
 * Retry one is enough.
 */
for (i = 0; i < new_task; i++)
	data[i] = kzalloc(ctx_size, flags);

raw_spin_lock_irqsave(&task_data_events_lock, flags);

for_each_process_thread(g, p) {
	if (i < unallocated)
		p->perf_ctx_data = data[i++];
	else
		WARN_ON
}
raw_spin_unlock_irqrestore(&task_data_events_lock, flags);

unlock:
	raw_spin_unlock_irqrestore(&task_data_events_lock, flags);

end:
	free unused data[]

Thanks,
Kan