linux-kernel - Re: [PATCH 19/19] perf: Make perf_pmu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <19e1e59c-8a1b-4eb0-8e4a-222417a1bd4d@linux.intel.com>
Date: Tue, 5 Nov 2024 10:25:15 -0500
From: "Liang, Kan" <kan.liang@...ux.intel.com>
To: Peter Zijlstra <peterz@...radead.org>
Cc: mingo@...nel.org, lucas.demarchi@...el.com, linux-kernel@...r.kernel.org,
 willy@...radead.org, acme@...nel.org, namhyung@...nel.org,
 mark.rutland@....com, alexander.shishkin@...ux.intel.com, jolsa@...nel.org,
 irogers@...gle.com, adrian.hunter@...el.com
Subject: Re: [PATCH 19/19] perf: Make perf_pmu_unregister() useable



On 2024-11-05 10:16 a.m., Peter Zijlstra wrote:
> On Tue, Nov 05, 2024 at 10:08:54AM -0500, Liang, Kan wrote:
>>> @@ -2427,6 +2429,7 @@ __perf_remove_from_context(struct perf_e
>>>  			   void *info)
>>>  {
>>>  	struct perf_event_pmu_context *pmu_ctx = event->pmu_ctx;
>>> +	enum perf_event_state state = PERF_EVENT_STATE_OFF;
>>
>> Set the PERF_EVENT_STATE_OFF as default seems dangerous.
>> If the event was in an error state, the state will be overwritten to the
>> PERF_EVENT_STATE_OFF later.
>>
>> One example may be the perf_pmu_migrate_context(). After the migration,
>> it looks like all the error state will be cleared.
>>
>> Thanks,
>> Kan
>>
>>>  	unsigned long flags = (unsigned long)info;
>>>  
>>>  	ctx_time_update(cpuctx, ctx);
>>> @@ -2435,16 +2438,22 @@ __perf_remove_from_context(struct perf_e
>>>  	 * Ensure event_sched_out() switches to OFF, at the very least
>>>  	 * this avoids raising perf_pending_task() at this time.
>>>  	 */
>>> -	if (flags & DETACH_DEAD)
>>> +	if (flags & DETACH_EXIT)
>>> +		state = PERF_EVENT_STATE_EXIT;
>>> +	if (flags & DETACH_REVOKE)
>>> +		state = PERF_EVENT_STATE_REVOKED;
>>> +	if (flags & DETACH_DEAD) {
>>>  		event->pending_disable = 1;
>>> +		state = PERF_EVENT_STATE_DEAD;
>>> +	}
>>>  	event_sched_out(event, ctx);
>>>  	if (flags & DETACH_GROUP)
>>>  		perf_group_detach(event);
>>>  	if (flags & DETACH_CHILD)
>>>  		perf_child_detach(event);
>>>  	list_del_event(event, ctx);
>>> -	if (flags & DETACH_DEAD)
>>> -		event->state = PERF_EVENT_STATE_DEAD;
>>> +
>>> +	event->state = state;
> 
> How about we make this:
> 
> 	event->state = min(event->state, state);
> 
> ?

Yep, it looks good to me.

Thanks,
Kan