Messages in this thread | | | Subject | Re: [RFC PATCH 2/8] perf: Helpers for alloc/init/fini PMU specific data | From | "Liang, Kan" <> | Date | Mon, 2 Dec 2019 15:35:00 -0500 |
| |
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
| |