Re: [PATCH 1/1] perf/core: fix dangling cgroup pointer in cpuctx

[Yeoreum Yun <yeoreum.yun@arm.com>]

Hi David,
> > attach_state doesn't related for event->state change.
> > if one event already cleared PERF_ATTACH_CONTEXT, that event is called
> > via list_del_event()
>
> Maybe this concern could be clarified, what about other subtle impacts.
> The change should be thorough reviewed, if you want to push it further.
>
> It takes me more than a month to figure out a procedure to reproduce the kernel panic bug,
> It is  just very hard to capture a bug happens in rare situation.
>
> And your patch has a global impact, it changes behavior unnecessarily.

TBH, this patch just change of time of "event->state" while doing,
As my bad miss the disable cgorup perf.
I think there seems no other side effect for chaning state while in
removing event.
But, Let's wait for other people's review.

> >
> > Also, your patch couldn't solve a problem describe in
> > commit a3c3c6667("perf/core: Fix child_total_time_enabled accounting bug at task exit")
> > for INCATIVE event's total_enable_time.
>
> I do not think so.
> Correct me if I am making silly  mistakes,
> The patch, https://lore.kernel.org/lkml/20250603032651.3988-1-00107082@163.com/
> calls perf_event_set_state() based on DETACH_EXIT flag, which cover the INACTIVE state, right?
> If DETACH_EXIT is not used for this purpose? Then why should it exist at the first place?
> I think I does not revert the purpose of commit a3c3c6667.....But I could be wrong
> Would you show a call path where DETACH_EXIT is not set, but the changes in commit a3c3c6667 is still needed?

Sorry for my bad explaination without detail.
Think about cpu specific event and closed by task.
If there is specific child cpu event specified in cpu 0.
  1. cpu 0 -> active
  2. scheulded to cpu1 -> inactive
  3. close the cpu event from parent -> inactive close

Can be failed to count total_enable_time.

Thanks.
--
Sincerely,
Yeoreum Yun