Re: [RFC PATCH 2/8] perf: Helpers for alloc/init/fini PMU specific data

[Liang, Kan]

On 12/2/2019 8:16 AM, Peter Zijlstra wrote:
> On Thu, Nov 28, 2019 at 07:14:25AM -0800, kan.liang@xxxxxxxxxxxxxxx wrote:
>
> > +static int
> > +__alloc_task_ctx_data_rcu(struct task_struct *task,
> > +			  size_t ctx_size, gfp_t flags)
> > +{
> > +	struct perf_ctx_data *ctx_data = task->perf_ctx_data;
> > +	int ret;
> > +
> > +	lockdep_assert_held_once(&task->perf_ctx_data_lock);
> > +
> > +	ret = alloc_perf_ctx_data(ctx_size, flags, &ctx_data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ctx_data->refcount = 1;
> > +
> > +	rcu_assign_pointer(task->perf_ctx_data, ctx_data);
> > +
> > +	return 0;
> > +}
>
> > +static int
> > +__init_task_ctx_data_rcu(struct task_struct *task, size_t ctx_size, gfp_t flags)
> > +{
> > +	struct perf_ctx_data *ctx_data = task->perf_ctx_data;
> > +
> > +	lockdep_assert_held_once(&task->perf_ctx_data_lock);
> > +
> > +	if (ctx_data) {
> > +		ctx_data->refcount++;
> > +		return 0;
> > +	}
> > +
> > +	return __alloc_task_ctx_data_rcu(task, ctx_size, flags);
> > +}
>
> > +/**
> > + * Free perf_ctx_data RCU pointer for a task
> > + * @task:        Target Task
> > + * @force:       Unconditionally free perf_ctx_data
> > + *
> > + * If force is set, free perf_ctx_data unconditionally.
> > + * Otherwise, free perf_ctx_data when there are no users.
> > + * Lock is required to sync the writers of perf_ctx_data RCU pointer
> > + * and refcount.
> > + */
> > +static void
> > +fini_task_ctx_data_rcu(struct task_struct *task, bool force)
> > +{
> > +	struct perf_ctx_data *ctx_data;
> > +	unsigned long flags;
> > +
> > +	raw_spin_lock_irqsave(&task->perf_ctx_data_lock, flags);
> > +
> > +	ctx_data = task->perf_ctx_data;
> > +	if (!ctx_data)
> > +		goto unlock;
> > +
> > +	if (!force && --ctx_data->refcount)
> > +		goto unlock;
> > +
> > +	RCU_INIT_POINTER(task->perf_ctx_data, NULL);
> > +	call_rcu(&ctx_data->rcu_head, free_perf_ctx_data);
> > +
> > +unlock:
> > +	raw_spin_unlock_irqrestore(&task->perf_ctx_data_lock, flags);
> > +}
>
> All this refcount under lock is an anti-pattern. Also the naming is
> insane.

Could you please give me an example?

I think we do need something to protect the refcount. Are you suggesting 
atomic_*?

Thanks,
Kan