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:   Mon, 9 Nov 2020 10:52:35 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     kan.liang@linux.intel.com
Cc:     mingo@kernel.org, linux-kernel@vger.kernel.org,
        namhyung@kernel.org, eranian@google.com, irogers@google.com,
        gmx@google.com, acme@kernel.org, jolsa@redhat.com,
        ak@linux.intel.com
Subject: Re: [PATCH 1/3] perf/core: Flush PMU internal buffers for per-CPU
 events
Message-ID: <20201109095235.GC2594@hirez.programming.kicks-ass.net>
References: <20201106212935.28943-1-kan.liang@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201106212935.28943-1-kan.liang@linux.intel.com>
Precedence: bulk

On Fri, Nov 06, 2020 at 01:29:33PM -0800, kan.liang@linux.intel.com wrote:
> From: Kan Liang <kan.liang@linux.intel.com>
> 
> Sometimes the PMU internal buffers have to be flushed for per-CPU events
> during a context switch, e.g., large PEBS. Otherwise, the perf tool may
> report samples in locations that do not belong to the process where the
> samples are processed in, because PEBS does not tag samples with PID/TID.
> 
> The current code only flush the buffers for a per-task event. It doesn't
> check a per-CPU event.
> 
> Add a new event state flag, PERF_ATTACH_SCHED_CB, to indicate that the
> PMU internal buffers have to be flushed for this event during a context
> switch.
> 
> Add sched_cb_entry and perf_sched_cb_usages back to track the PMU/cpuctx
> which is required to be flushed.
> 
> Only need to invoke the sched_task() for per-CPU events in this patch.
> The per-task events have been handled in perf_event_context_sched_in/out
> already.
> 
> Fixes: 9c964efa4330 ("perf/x86/intel: Drain the PEBS buffer during context switches")

Are you sure? In part this patch looks like a revert of:

  44fae179ce73a26733d9e2d346da4e1a1cb94647
  556cccad389717d6eb4f5a24b45ff41cad3aaabf


> +static void perf_pmu_sched_task(struct task_struct *prev,
> +				struct task_struct *next,
> +				bool sched_in)
> +{
> +	struct perf_cpu_context *cpuctx;
> +
> +	if (prev == next)
> +		return;
> +
> +	list_for_each_entry(cpuctx, this_cpu_ptr(&sched_cb_list), sched_cb_entry) {
> +		/* will be handled in perf_event_context_sched_in/out */
> +		if (cpuctx->task_ctx)
> +			continue;

This seems wrong; cpuctx->task_ctx merely indicates that there is a
task-ctx for this CPU. Not that the event you're interested in is in
fact there.

So consider the case where the event is on the CPU context, but we also
have a task context. Then we'll not issue this call.

> +
> +		__perf_pmu_sched_task(cpuctx, sched_in);
> +	}
> +}