LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: Steven Rostedt <rostedt@goodmis.org>
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
Date: Fri, 29 Sep 2017 13:50:16 +0200
Message-ID: <20170929115016.pzlqc7ss3ccystyg@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <20170925160109.0086eccb@vmware.local.home>

On Mon, Sep 25, 2017 at 04:01:09PM -0400, Steven Rostedt wrote:
> On Mon, 25 Sep 2017 14:07:48 +0200
> Peter Zijlstra <peterz@infradead.org> 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))

  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-25 12:07 [PATCH 0/8] sched: Rework task state printing Peter Zijlstra
2017-09-25 12:07 ` [PATCH 1/8] sched: Consistent task-state printing Peter Zijlstra
2017-09-25 20:01   ` Steven Rostedt
2017-09-29 11:50     ` Peter Zijlstra [this message]
2017-09-29 13:00       ` Steven Rostedt
2017-10-10 10:55       ` [tip:sched/core] sched/debug: Rename task-state printing helpers tip-bot for Peter Zijlstra
2017-09-25 12:07 ` [PATCH 2/8] sched: Convert TASK_state to hex Peter Zijlstra
2017-09-25 12:07 ` [PATCH 3/8] sched/debug: Remove unused variable Peter Zijlstra
2017-09-25 12:07 ` [PATCH 4/8] sched/trace: Fix trace_sched_switch task-state printing Peter Zijlstra
2017-09-25 12:07 ` [PATCH 5/8] sched/trace: Use common task-state helpers Peter Zijlstra
2017-09-25 12:07 ` [PATCH 6/8] sched: Add explicit TASK_IDLE printing Peter Zijlstra
2017-09-25 12:07 ` [PATCH 7/8] sched: Ignore TASK_IDLE for SysRq-W Peter Zijlstra
2017-09-25 12:07 ` [PATCH 8/8] sched: Add explicit TASK_PARKED printing Peter Zijlstra
2017-09-25 13:41 ` [PATCH 0/8] sched: Rework task state printing Thomas Gleixner

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170929115016.pzlqc7ss3ccystyg@hirez.programming.kicks-ass.net \
    --to=peterz@infradead.org \
    --cc=ebiederm@xmission.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=markus@trippelsdorf.de \
    --cc=mcgrof@kernel.org \
    --cc=mingo@kernel.org \
    --cc=paulmck@linux.vnet.ibm.com \
    --cc=rostedt@goodmis.org \
    --cc=tglx@linutronix.de \
    --cc=tj@kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git