Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [RFC PATCH 2/8] perf: Helpers for alloc/init/fini PMU specific
 data
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     mingo@redhat.com, acme@kernel.org, tglx@linutronix.de,
        bp@alien8.de, linux-kernel@vger.kernel.org, eranian@google.com,
        alexey.budankov@linux.intel.com, vitaly.slobodskoy@intel.com,
        ak@linux.intel.com
References: <1574954071-6321-1-git-send-email-kan.liang@linux.intel.com>
 <1574954071-6321-2-git-send-email-kan.liang@linux.intel.com>
 <20191202131646.GD2827@hirez.programming.kicks-ass.net>
From:   "Liang, Kan" <kan.liang@linux.intel.com>
Message-ID: <2d036aa5-542a-8c01-762c-3b68136887f5@linux.intel.com>
Date:   Mon, 2 Dec 2019 15:35:00 -0500
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.9.1
MIME-Version: 1.0
In-Reply-To: <20191202131646.GD2827@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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