From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751468AbdGRJ3i (ORCPT ); Tue, 18 Jul 2017 05:29:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:59302 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751349AbdGRJ3f (ORCPT ); Tue, 18 Jul 2017 05:29:35 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 16B267D0C5 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jolsa@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 16B267D0C5 Date: Tue, 18 Jul 2017 11:29:32 +0200 From: Jiri Olsa To: Peter Zijlstra Cc: Jiri Olsa , lkml , Ingo Molnar , Andi Kleen , Alexander Shishkin , Kan Liang Subject: Re: [PATCH] perf/x86/intel: Add proper condition to run sched_task callbacks Message-ID: <20170718092932.GA26157@krava> References: <20170717150156.11784-1-jolsa@kernel.org> <20170718091444.afqdtgjijcztv2mn@hirez.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170718091444.afqdtgjijcztv2mn@hirez.programming.kicks-ass.net> User-Agent: Mutt/1.8.3 (2017-05-23) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Tue, 18 Jul 2017 09:29:35 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jul 18, 2017 at 11:14:44AM +0200, Peter Zijlstra wrote: > On Mon, Jul 17, 2017 at 05:01:56PM +0200, Jiri Olsa wrote: > > The x86 pmu currently uses the sched_task callback for 2 functions: > > - PEBS drain > > - save/restore LBR data > > > > They are both triggered once the x86 pmu is registered with > > perf_sched_cb_inc call (within pmu::add callback), regardless > > if there's actually any PEBS or LBR event configured on the cpu. > > I don't understand. If we do pmu::add() we _are_ on the CPU. > > So you're saying intel_pmu_pebs_{add,del}() are doing it wrong? So why > not fix those? > > > diff --git a/arch/x86/events/intel/core.c b/arch/x86/events/intel/core.c > > index aa62437d1aa1..1f66356d8122 100644 > > --- a/arch/x86/events/intel/core.c > > +++ b/arch/x86/events/intel/core.c > > @@ -3265,9 +3265,11 @@ static void intel_pmu_cpu_dying(int cpu) > > static void intel_pmu_sched_task(struct perf_event_context *ctx, > > bool sched_in) > > { > > - if (x86_pmu.pebs_active) > > + struct cpu_hw_events *cpuc = this_cpu_ptr(&cpu_hw_events); > > + > > + if (intel_pmu_pebs_needs_sched_cb(cpuc)) > > intel_pmu_pebs_sched_task(ctx, sched_in); > > So I'm confused, if we'd not need this, how come we're here in the first > place? > because we have 2 places using the same callback - PEBS drain for free running counters - LBR save/store both of them called from intel_pmu_sched_task so let's say PEBS drain setup the callback for the event, but in the callback itself (intel_pmu_sched_task) we will also run the code for LBR save/restore, which we did not ask for, but the code in intel_pmu_sched_task does not check for that I'm adding conditions to recognize the work that needs to be done in the callback another option might be to add support for more x86_pmu::sched_task callbacks, which might be cleaner jirka