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

From: Alexey Budankov
Date: Thu Mar 29 2018 - 09:57:33 EST


On 29.03.2018 16:31, Alexander Shishkin wrote:
> 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.

It makes sense. Thanks for sharing your opinion and valuable guidance.

>
> 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.

My bad. That is missing from the description but you got the rationale just right.

> 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@xxxxxxxxxxxxxxx>
>> ---
>> 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.

Yes. It could be. Tried to minimize amount of patches in the series,
at the same time remembering about successful git bisect integration.

>
>> 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;
> }
>
> ?

Sure. Let me take care of all your comments.

Thanks,
Alexey

>
> Thanks,
> --
> Alex
>
>