Re: [PATCH v2 24/24] perf: Make perf_pmu_unregister() useable

[Peter Zijlstra]

On Thu, Feb 13, 2025 at 01:22:55PM +0530, Ravi Bangoria wrote:
> > Does this work?
> > 
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -2303,6 +2303,7 @@ static void perf_child_detach(struct per
> >  
> >  	sync_child_event(event);
> >  	list_del_init(&event->child_list);
> > +	event->parent = NULL;
> >  }
> >  
> >  static bool is_orphaned_event(struct perf_event *event)
> 
> Apparently not, it ends up with:
> 
>   ------------[ cut here ]------------
>   WARNING: CPU: 145 PID: 5459 at kernel/events/core.c:281 event_function+0xd2/0xf0

Durr, do you have an updated test case?

> Something like below instead? I haven't tested it thoroughly though.
> 
> ---
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index d2b87a425e75..4e131b1c37ad 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -13645,20 +13645,25 @@ perf_event_exit_event(struct perf_event *event,
>  	unsigned long detach_flags = DETACH_EXIT;
>  
>  	if (parent_event) {
> -		/*
> -		 * Do not destroy the 'original' grouping; because of the
> -		 * context switch optimization the original events could've
> -		 * ended up in a random child task.
> -		 *
> -		 * If we were to destroy the original group, all group related
> -		 * operations would cease to function properly after this
> -		 * random child dies.
> -		 *
> -		 * Do destroy all inherited groups, we don't care about those
> -		 * and being thorough is better.
> -		 */
> -		detach_flags |= DETACH_GROUP | DETACH_CHILD;
>  		mutex_lock(&parent_event->child_mutex);
> +		if (event->attach_state & PERF_ATTACH_CHILD) {
> +			/*
> +			 * Do not destroy the 'original' grouping; because of the
> +			 * context switch optimization the original events could've
> +			 * ended up in a random child task.
> +			 *
> +			 * If we were to destroy the original group, all group related
> +			 * operations would cease to function properly after this
> +			 * random child dies.
> +			 *
> +			 * Do destroy all inherited groups, we don't care about those
> +			 * and being thorough is better.
> +			 */
> +			detach_flags |= DETACH_GROUP | DETACH_CHILD;
> +		} else {
> +			mutex_unlock(&parent_event->child_mutex);
> +			parent_event = NULL;
> +		}
>  	}

Yeah, that might do, but not really nice. But perhaps its the best we
can do. I'll give it some thought.