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

[Alexey Budankov]

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