lkml.org 
[lkml]   [2019]   [Dec]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC PATCH 2/8] perf: Helpers for alloc/init/fini PMU specific data
From
Date


On 12/2/2019 8:16 AM, Peter Zijlstra wrote:
> On Thu, Nov 28, 2019 at 07:14:25AM -0800, kan.liang@linux.intel.com 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

\
 
 \ /
  Last update: 2019-12-02 21:36    [W:0.060 / U:4.628 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site