Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 21 Oct 2019 09:59:42 +0200
From:   Ingo Molnar <mingo@kernel.org>
To:     Alexey Budankov <alexey.budankov@linux.intel.com>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Ingo Molnar <mingo@redhat.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Andi Kleen <ak@linux.intel.com>,
        Kan Liang <kan.liang@linux.intel.com>,
        Stephane Eranian <eranian@google.com>,
        Ian Rogers <irogers@google.com>,
        Song Liu <songliubraving@fb.com>,
        linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v3 4/4] perf/core,x86: synchronize PMU task contexts on
 optimized context switches
Message-ID: <20191021075942.GA8809@gmail.com>
References: <0b20a07f-d074-d3da-7551-c9a4a94fe8e3@linux.intel.com>
 <f3253a36-c174-8051-a462-9728ef721766@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <f3253a36-c174-8051-a462-9728ef721766@linux.intel.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


* Alexey Budankov <alexey.budankov@linux.intel.com> wrote:

> +			/*
> +			 * PMU specific parts of task perf context may require
> +			 * additional synchronization, at least for proper Intel
> +			 * LBR callstack data profiling;
> +			 */
> +			pmu->sync_task_ctx(ctx->task_ctx_data,
> +					   next_ctx->task_ctx_data);

Firstly, I'm pretty sure you never run this on a CPU where 
pmu->sync_task_ctx is NULL, right? ;-)

Secondly, even on Intel CPUs in many cases we'll just call into a ~2 deep 
function pointer based call hierarchy, just to find that nothing needs to 
be done, because there's no LBR call stack maintained:

+       if (!one || !another)
+               return;

So while it's technically a layering violation, it might make sense to 
elevate this check to the generic layer and say that synchronization 
calls by the core layer will always provide two valid pointers?

Thanks,

	Ingo