Re: [PATCH] perf/core: fix userpage->time_enabled of inactive events

[Peter Zijlstra]

On Thu, Sep 23, 2021 at 05:47:40PM +0000, Song Liu wrote:

> > 
> > So we already iterate all the right events. So I'm thinking we can do
> > something like the below, hmm?
> > 
> > 
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 0c000cb01eeb..4d1e962c2ebe 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -3707,6 +3712,28 @@ static noinline int visit_groups_merge(struct perf_cpu_context *cpuctx,
> > 	return 0;
> > }
> > 
> > +static inline bool event_update_userpage(struct perf_event *event)
> > +{
> > +	if (!atomic_read(&event->mmap_count))
> > +		return false;
> 
> Technically, the user could mmap a sibling but not the group leader, right?
> It is weird though. 

Yeah, I went with the same logic that a disabled leader disables the
whole group, I suppose we can revisit if anybody ever gets a sane
use-case for this.

> There is also a corner case we didn't cover. If the user enable the event
> before mmap it. The event is first scheduled in via:
> 
> __perf_event_enable (or __perf_install_in_context)
>    -> ctx_resched
>       -> perf_event_sched_in
>           -> ctx_sched_in
> 
> but it doesn't have mmap_count yet. And we won't call perf_event_update_userpage
> for it before the first rotation after mmap. As a result, the user page will
> contain garbage data before the first rotation. 

That was already a problem, because without the mmap, the event->rb will
be NULL and perf_event_update_userpage() will not actually do anything.

Should not the mmap() itself update the state before populating those
fields for the first time?

> Other than this corner case, this version works well in my tests. Shall I send 
> v2 in this version?

Please,