Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 28 Jul 2020 15:44:12 +0200
From:   peterz@infradead.org
To:     "Liang, Kan" <kan.liang@linux.intel.com>
Cc:     acme@redhat.com, mingo@kernel.org, linux-kernel@vger.kernel.org,
        jolsa@kernel.org, eranian@google.com,
        alexander.shishkin@linux.intel.com, ak@linux.intel.com,
        like.xu@linux.intel.com
Subject: Re: [PATCH V7 08/14] perf/x86/intel: Generic support for hardware
 TopDown metrics
Message-ID: <20200728134412.GX119549@hirez.programming.kicks-ass.net>
References: <20200723171117.9918-1-kan.liang@linux.intel.com>
 <20200723171117.9918-9-kan.liang@linux.intel.com>
 <20200724131906.GW119549@hirez.programming.kicks-ass.net>
 <20200724152755.GK43129@hirez.programming.kicks-ass.net>
 <d7ae2272-52ea-c5a9-2937-9a51c544ade8@linux.intel.com>
 <ddeae082-c38d-a961-4d90-1fbc1c9f2726@linux.intel.com>
 <20200728130956.GK10769@hirez.programming.kicks-ass.net>
 <25a630c6-2fa2-5dfd-bcd3-92e809c51d61@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <25a630c6-2fa2-5dfd-bcd3-92e809c51d61@linux.intel.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jul 28, 2020 at 09:28:39AM -0400, Liang, Kan wrote:
> 
> 
> On 7/28/2020 9:09 AM, Peter Zijlstra wrote:
> > On Fri, Jul 24, 2020 at 03:10:52PM -0400, Liang, Kan wrote:
> > 
> > > diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c
> > > index 6cb079e0c9d9..010ac74afc09 100644
> > > --- a/arch/x86/events/intel/core.c
> > > +++ b/arch/x86/events/intel/core.c
> > > @@ -2405,27 +2405,18 @@ static u64 icl_update_topdown_event(struct
> > > perf_event *event)
> > >   	return slots;
> > >   }
> > > 
> > > -static void intel_pmu_read_topdown_event(struct perf_event *event)
> > > +static void intel_pmu_read_event(struct perf_event *event)
> > >   {
> > > -	struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events);
> > > -
> > > -	/* Only need to call update_topdown_event() once for group read. */
> > > -	if ((cpuc->txn_flags & PERF_PMU_TXN_READ) &&
> > > -	    !is_slots_event(event))
> > >   		return;
> > > 
> > > -	perf_pmu_disable(event->pmu);
> > > -	x86_pmu.update_topdown_event(event);
> > > -	perf_pmu_enable(event->pmu);
> > > -}
> > > -
> > > -static void intel_pmu_read_event(struct perf_event *event)
> > > -{
> > >   	if (event->hw.flags & PERF_X86_EVENT_AUTO_RELOAD)
> > >   		intel_pmu_auto_reload_read(event);
> > > -	else if (is_topdown_count(event) && x86_pmu.update_topdown_event)
> > > -		intel_pmu_read_topdown_event(event);
> > > -	else
> > > +	else if (is_slots_count(event) && x86_pmu.update_topdown_event) {
> > > +		perf_pmu_disable(event->pmu);
> > > +		x86_pmu.update_topdown_event(event);
> > > +		perf_pmu_enable(event->pmu);
> > > +	} else
> > >   		x86_perf_event_update(event);
> > >   }
> > 
> > I'm a little puzzled by this; what happens if you:
> > 
> > 	fd = sys_perf_event_open(&attr_slots);
> > 	fd1 = sys_perf_event_open(&attr_metric, .group_fd=fd);
> > 
> > 	read(fd1);
> > 
> > ?
> > 
> 
> I did a quick test. It depends on the .read_format of attr_metric.
> If PERF_FORMAT_GROUP is applied for attr_metric, perf_read_group() will be
> invoked. The value of fd1 is updated correctly.
> If the flag is not applied, 0 will be returned.

Exactly :-), was that intentional? Because prior to this change that
would've worked just fine.

Now, I agree it's a daft thing, because that value is pretty useless
without also having the slots value, but I feel we should be explicit in
our choices here.

If for example, we would've had hardware provide us the raw metric
counters, instead of us having to reconstruct them, this would've been a
semi useful thing.

So I'm tempted to leave things as it, and keep this 'working'.