Re: [PATCH 1/8] sched: Consistent task-state printing

[Steven Rostedt]

On Fri, 29 Sep 2017 13:50:16 +0200
Peter Zijlstra  wrote:

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

I'm not picky, I just hate beginning underscores, and wish we can start
removing all functions that use them.

Acked-by: Steven Rostedt (VMware) 

-- Steve

Re: [PATCH 1/8] sched: Consistent task-state printing

[Steven Rostedt]

On Fri, 29 Sep 2017 13:50:16 +0200
Peter Zijlstra  wrote:

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

I'm not picky, I just hate beginning underscores, and wish we can start
removing all functions that use them.

Acked-by: Steven Rostedt (VMware) 

-- Steve

Re: [PATCH 1/8] sched: Consistent task-state printing

[Peter Zijlstra]

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

Re: [PATCH 1/8] sched: Consistent task-state printing

[Peter Zijlstra]

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(>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(>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);

Re: [PATCH 1/8] sched: Consistent task-state printing

[Steven Rostedt]

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

?

This way, the "__" wont keep making me think we used the wrong
function.

-- Steve

Re: [PATCH 1/8] sched: Consistent task-state printing

[Steven Rostedt]

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

?

This way, the "__" wont keep making me think we used the wrong
function.

-- Steve