linux-kernel - Re: [PATCH v3 1/3] perf/core: store context switch out type into Perf trace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180329133138.b3qe2zvje2ak7qe5@um.fi.intel.com>
Date:   Thu, 29 Mar 2018 16:31:38 +0300
From:   Alexander Shishkin <alexander.shishkin@...ux.intel.com>
To:     Alexey Budankov <alexey.budankov@...ux.intel.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        Andi Kleen <ak@...ux.intel.com>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/3] perf/core: store context switch out type into
 Perf trace

On Mon, Mar 26, 2018 at 12:20:32PM +0300, Alexey Budankov wrote:
> 
> Store thread context-switch-out event type into Perf trace as a part of 
> PERF_RECORD_SWITCH[_CPU_WIDE] records.
> 
> Introduced types of switch-out events assumed to be 
> a) preempt: task->state == TASK_RUNNING and b) !preempt;
> 
> New !preempt event type is encoded using new 
> PERF_RECORD_MISC_SWITCH_OUT_PREEMPT bit extending 
> existing PERF_RECORD_MISC_SWITCH_OUT bit of switch out event:
> 
>     misc &= PERF_RECORD_MISC_SWITCH_OUT | PERF_RECORD_MISC_SWITCH_OUT_PREEMPT

I'd like to offer some suggestions as to how to make the commit message
friendlier for reviewing.

Generally, for every patch, we want to explain the following: what we want,
why we want it and how we want to go about getting it. We also would prefer
to do it in english rather than in C, because for the latter we can just
look at the code.

So, my understanding of this patch translates into something like this.

What: we want to tell apart preemting and non-preempting context switches.
Why: I'm guessing it tells us something about the kind of workloads that
are running on the machine. This doesn't seem to be mentioned anywhere.
How: we add a new bit to the event header to indicate that the corresponding
sched-out is preempting.

> Signed-off-by: Alexey Budankov <alexey.budankov@...ux.intel.com>
> ---
>  include/uapi/linux/perf_event.h       | 4 ++++
>  kernel/events/core.c                  | 4 +++-
>  tools/include/uapi/linux/perf_event.h | 4 ++++

The last one probably wants to be a separate patch.

>  3 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h
> index 912b85b52344..cd6ad7e13824 100644
> --- a/include/uapi/linux/perf_event.h
> +++ b/include/uapi/linux/perf_event.h
> @@ -655,6 +655,10 @@ struct perf_event_mmap_page {
>   * perf_event_attr::precise_ip.
>   */
>  #define PERF_RECORD_MISC_EXACT_IP		(1 << 14)
> +/*
> + * Indicates that thread was preempted in TASK_RUNNING state
> + */
> +#define PERF_RECORD_MISC_SWITCH_OUT_PREEMPT	(1 << 14)
>  /*
>   * Reserve the last bit to indicate some extended misc field
>   */
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 74a6e8f12a3c..0d39192215bc 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -7556,6 +7556,8 @@ static void perf_event_switch(struct task_struct *task,
>  			      struct task_struct *next_prev, bool sched_in)
>  {
>  	struct perf_switch_event switch_event;
> +	__u16 switch_type = sched_in ? 0 : PERF_RECORD_MISC_SWITCH_OUT |
> +		(task->state == TASK_RUNNING ? PERF_RECORD_MISC_SWITCH_OUT_PREEMPT : 0);

This is also hard on the eyes. Can't we just

if (!sched_in) {
	misc = SWITCH_OUT;
	if (task->state == TASK_RUNNING)
		misc |= SWITCH_OUT_PREEMPT;
}

?

Thanks,
--
Alex