Messages in this thread | | | Subject | Re: [PATCH v3 1/3] perf/core: store context switch out type into Perf trace | From | Alexey Budankov <> | Date | Thu, 29 Mar 2018 16:57:21 +0300 |
| |
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@linux.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.
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 > >
| |