From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752250AbdI2Lw1 (ORCPT ); Fri, 29 Sep 2017 07:52:27 -0400 Received: from merlin.infradead.org ([205.233.59.134]:41756 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751906AbdI2LuZ (ORCPT ); Fri, 29 Sep 2017 07:50:25 -0400 Date: Fri, 29 Sep 2017 13:50:16 +0200 From: Peter Zijlstra To: Steven Rostedt Cc: mingo@kernel.org, markus@trippelsdorf.de, tj@kernel.org, mcgrof@kernel.org, ebiederm@xmission.com, paulmck@linux.vnet.ibm.com, torvalds@linux-foundation.org, tglx@linutronix.de, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/8] sched: Consistent task-state printing Message-ID: <20170929115016.pzlqc7ss3ccystyg@hirez.programming.kicks-ass.net> References: <20170925120747.125098571@infradead.org> <20170925121116.927982688@infradead.org> <20170925160109.0086eccb@vmware.local.home> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170925160109.0086eccb@vmware.local.home> User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Sep 25, 2017 at 04:01:09PM -0400, Steven Rostedt wrote: > On Mon, 25 Sep 2017 14:07:48 +0200 > Peter Zijlstra wrote: > > > +static inline char __task_state_to_char(unsigned int state) > > +{ > > + static const char state_char[] = "RSDTtXZ"; > > + > > + BUILD_BUG_ON(1 + ilog2(TASK_REPORT) != sizeof(state_char) - 2); > > > > - return state < sizeof(stat_nam) - 1 ? stat_nam[state] : '?'; > > + return state_char[state]; > > +} > > + > > +static inline char task_state_to_char(struct task_struct *tsk) > > +{ > > + return __task_state_to_char(__get_task_state(tsk)); > > } > > > > So far I'm fine with the patch set, but I hate the non descriptive "__" > prefix of __task_state_to_char(). Can we make this a bit more > descriptive, because every time I see it in other patches, I go back to > this patch to see if we are using the right function. > > What about something like: > > task_state_to_state_char(unsigned int state); > > task_to_state_char(struct task_struct *tsk); > > ? Hurmph.. naming. How about something like so? --- a/fs/proc/array.c +++ b/fs/proc/array.c @@ -137,7 +137,7 @@ static const char * const task_state_arr static inline const char *get_task_state(struct task_struct *tsk) { BUILD_BUG_ON(1 + ilog2(TASK_REPORT_MAX) != ARRAY_SIZE(task_state_array)); - return task_state_array[__get_task_state(tsk)]; + return task_state_array[task_state_index(tsk)]; } static inline int get_task_umask(struct task_struct *tsk) --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1245,7 +1245,7 @@ static inline pid_t task_pgrp_nr(struct #define TASK_REPORT_IDLE (TASK_REPORT + 1) #define TASK_REPORT_MAX (TASK_REPORT_IDLE << 1) -static inline unsigned int __get_task_state(struct task_struct *tsk) +static inline unsigned int task_state_index(struct task_struct *tsk) { unsigned int tsk_state = READ_ONCE(tsk->state); unsigned int state = (tsk_state | tsk->exit_state) & TASK_REPORT; @@ -1258,7 +1258,7 @@ static inline unsigned int __get_task_st return fls(state); } -static inline char __task_state_to_char(unsigned int state) +static inline char task_index_to_char(unsigned int state) { static const char state_char[] = "RSDTtXZPI"; @@ -1269,7 +1269,7 @@ static inline char __task_state_to_char( static inline char task_state_to_char(struct task_struct *tsk) { - return __task_state_to_char(__get_task_state(tsk)); + return task_index_to_char(task_state_index(tsk)); } /** --- a/include/trace/events/sched.h +++ b/include/trace/events/sched.h @@ -117,7 +117,7 @@ static inline long __trace_sched_switch_ if (preempt) return TASK_STATE_MAX; - return __get_task_state(p); + return task_state_index(p); } #endif /* CREATE_TRACE_POINTS */ --- a/kernel/trace/trace_output.c +++ b/kernel/trace/trace_output.c @@ -921,8 +921,8 @@ static enum print_line_t trace_ctxwake_p trace_assign_type(field, iter->ent); - T = __task_state_to_char(field->next_state); - S = __task_state_to_char(field->prev_state); + T = task_index_to_char(field->next_state); + S = task_index_to_char(field->prev_state); trace_find_cmdline(field->next_pid, comm); trace_seq_printf(&iter->seq, " %5d:%3d:%c %s [%03d] %5d:%3d:%c %s\n", @@ -957,8 +957,8 @@ static int trace_ctxwake_raw(struct trac trace_assign_type(field, iter->ent); if (!S) - S = __task_state_to_char(field->prev_state); - T = __task_state_to_char(field->next_state); + S = task_index_to_char(field->prev_state); + T = task_index_to_char(field->next_state); trace_seq_printf(&iter->seq, "%d %d %c %d %d %d %c\n", field->prev_pid, field->prev_prio, @@ -993,8 +993,8 @@ static int trace_ctxwake_hex(struct trac trace_assign_type(field, iter->ent); if (!S) - S = __task_state_to_char(field->prev_state); - T = __task_state_to_char(field->next_state); + S = task_index_to_char(field->prev_state); + T = task_index_to_char(field->next_state); SEQ_PUT_HEX_FIELD(s, field->prev_pid); SEQ_PUT_HEX_FIELD(s, field->prev_prio); --- a/kernel/trace/trace_sched_wakeup.c +++ b/kernel/trace/trace_sched_wakeup.c @@ -397,10 +397,10 @@ tracing_sched_switch_trace(struct trace_ entry = ring_buffer_event_data(event); entry->prev_pid = prev->pid; entry->prev_prio = prev->prio; - entry->prev_state = __get_task_state(prev); + entry->prev_state = task_state_index(prev); entry->next_pid = next->pid; entry->next_prio = next->prio; - entry->next_state = __get_task_state(next); + entry->next_state = task_state_index(next); entry->next_cpu = task_cpu(next); if (!call_filter_check_discard(call, entry, buffer, event)) @@ -425,10 +425,10 @@ tracing_sched_wakeup_trace(struct trace_ entry = ring_buffer_event_data(event); entry->prev_pid = curr->pid; entry->prev_prio = curr->prio; - entry->prev_state = __get_task_state(curr); + entry->prev_state = task_state_index(curr); entry->next_pid = wakee->pid; entry->next_prio = wakee->prio; - entry->next_state = __get_task_state(wakee); + entry->next_state = task_state_index(wakee); entry->next_cpu = task_cpu(wakee); if (!call_filter_check_discard(call, entry, buffer, event))