linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH-next] sched/headers: Clean up <linux/sched.h>
@ 2018-02-15 15:43 Christopher Diaz Riveros
  2018-02-15 16:52 ` Peter Zijlstra
  2018-02-15 17:49 ` [PATCH-next] sched/headers: Clean " Randy Dunlap
  0 siblings, 2 replies; 9+ messages in thread
From: Christopher Diaz Riveros @ 2018-02-15 15:43 UTC (permalink / raw)
  To: mingo, peterz; +Cc: linux-kernel, kernel-janitors

Trivial clean up making  comments fit in 80 columns and keeping the same comment style.

Signed-off-by: Christopher Diaz Riveros <chrisadr@gentoo.org>
---
 include/linux/sched.h | 54 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 19 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index b161ef8a902e..c752a0d48944 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -183,7 +183,7 @@ extern void io_schedule_finish(int token);
 extern long io_schedule_timeout(long timeout);
 extern void io_schedule(void);
 
-/**
+/*
  * struct prev_cputime - snapshot of system and user cputime
  * @utime: time spent in user mode
  * @stime: time spent in system mode
@@ -200,7 +200,7 @@ struct prev_cputime {
 #endif
 };
 
-/**
+/*
  * struct task_cputime - collected CPU time counts
  * @utime:		time spent in user mode, in nanoseconds
  * @stime:		time spent in kernel mode, in nanoseconds
@@ -437,20 +437,28 @@ struct sched_dl_entity {
 	 * during sched_setattr(), they will remain the same until
 	 * the next sched_setattr().
 	 */
-	u64				dl_runtime;	/* Maximum runtime for each instance	*/
-	u64				dl_deadline;	/* Relative deadline of each instance	*/
-	u64				dl_period;	/* Separation of two instances (period) */
-	u64				dl_bw;		/* dl_runtime / dl_period		*/
-	u64				dl_density;	/* dl_runtime / dl_deadline		*/
+	/* Maximum runtime for each instance	*/
+	u64				dl_runtime;
+	/* Relative deadline of each instance	*/
+	u64				dl_deadline;
+	/* Separation of two instances (period) */
+	u64				dl_period;
+	/* dl_runtime / dl_period		*/
+	u64				dl_bw;
+	/* dl_runtime / dl_deadline		*/
+	u64				dl_density;
 
 	/*
 	 * Actual scheduling parameters. Initialized with the values above,
 	 * they are continously updated during task execution. Note that
 	 * the remaining runtime could be < 0 in case we are in overrun.
 	 */
-	s64				runtime;	/* Remaining runtime for this instance	*/
-	u64				deadline;	/* Absolute deadline for this instance	*/
-	unsigned int			flags;		/* Specifying the scheduler behaviour	*/
+	/* Remaining runtime for this instance	*/
+	s64				runtime;
+	/* Absolute deadline for this instance	*/
+	u64				deadline;
+	/* Specifying the scheduler behaviour	*/
+	unsigned int			flags;	
 
 	/*
 	 * Some bool flags:
@@ -666,7 +674,8 @@ struct task_struct {
 	unsigned			no_cgroup_migration:1;
 #endif
 
-	unsigned long			atomic_flags; /* Flags requiring atomic access. */
+	/* Flags requiring atomic access. */
+	unsigned long			atomic_flags;
 
 	struct restart_block		restart_block;
 
@@ -678,8 +687,9 @@ struct task_struct {
 	unsigned long			stack_canary;
 #endif
 	/*
-	 * Pointers to the (original) parent process, youngest child, younger sibling,
-	 * older sibling, respectively.  (p->father can be replaced with
+	 * Pointers to the (original) parent process, youngest child,
+	 * younger sibling, older sibling, respectively.
+	 * (p->father can be replaced with
 	 * p->real_parent->pid)
 	 */
 
@@ -743,7 +753,10 @@ struct task_struct {
 	/* Boot based time in nsecs: */
 	u64				real_start_time;
 
-	/* MM fault and swap info: this can arguably be seen as either mm-specific or thread-specific: */
+	/* 
+	 * MM fault and swap info: this can arguably be seen as either
+	 * mm-specific or thread-specific: 
+	 */
 	unsigned long			min_flt;
 	unsigned long			maj_flt;
 
@@ -815,7 +828,10 @@ struct task_struct {
 	u32				parent_exec_id;
 	u32				self_exec_id;
 
-	/* Protection against (de-)allocation: mm, files, fs, tty, keyrings, mems_allowed, mempolicy: */
+	/*
+	 * Protection against (de-)allocation: mm, files, fs, tty,
+	 * keyrings, mems_allowed, mempolicy: 
+	 */
 	spinlock_t			alloc_lock;
 
 	/* Protection of the PI data structures: */
@@ -1176,7 +1192,7 @@ static inline pid_t task_tgid_nr(struct task_struct *tsk)
 	return tsk->tgid;
 }
 
-/**
+/*
  * pid_alive - check that a task structure is not stale
  * @p: Task structure to be checked.
  *
@@ -1275,7 +1291,7 @@ static inline char task_state_to_char(struct task_struct *tsk)
 	return task_index_to_char(task_state_index(tsk));
 }
 
-/**
+/*
  * is_global_init - check if a task structure is init. Since init
  * is free to have sub-threads we need to check tgid.
  * @tsk: Task structure to be checked.
@@ -1422,7 +1438,7 @@ extern int yield_to(struct task_struct *p, bool preempt);
 extern void set_user_nice(struct task_struct *p, long nice);
 extern int task_prio(const struct task_struct *p);
 
-/**
+/*
  * task_nice - return the nice value of a given task.
  * @p: the task in question.
  *
@@ -1442,7 +1458,7 @@ extern int sched_setattr(struct task_struct *, const struct sched_attr *);
 extern int sched_setattr_nocheck(struct task_struct *, const struct sched_attr *);
 extern struct task_struct *idle_task(int cpu);
 
-/**
+/*
  * is_idle_task - is the specified task an idle task?
  * @p: the task in question.
  *
-- 
2.16.1

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH-next] sched/headers: Clean up <linux/sched.h>
  2018-02-15 15:43 [PATCH-next] sched/headers: Clean up <linux/sched.h> Christopher Diaz Riveros
@ 2018-02-15 16:52 ` Peter Zijlstra
  2018-02-15 17:10   ` Christopher Díaz Riveros
  2018-02-16  9:44   ` Juri Lelli
  2018-02-15 17:49 ` [PATCH-next] sched/headers: Clean " Randy Dunlap
  1 sibling, 2 replies; 9+ messages in thread
From: Peter Zijlstra @ 2018-02-15 16:52 UTC (permalink / raw)
  To: Christopher Diaz Riveros; +Cc: mingo, linux-kernel, kernel-janitors

On Thu, Feb 15, 2018 at 10:43:18AM -0500, Christopher Diaz Riveros wrote:
> @@ -183,7 +183,7 @@ extern void io_schedule_finish(int token);
>  extern long io_schedule_timeout(long timeout);
>  extern void io_schedule(void);
>  
> -/**
> +/*
>   * struct prev_cputime - snapshot of system and user cputime
>   * @utime: time spent in user mode
>   * @stime: time spent in system mode
> @@ -200,7 +200,7 @@ struct prev_cputime {
>  #endif
>  };
>  
> -/**
> +/*
>   * struct task_cputime - collected CPU time counts
>   * @utime:		time spent in user mode, in nanoseconds
>   * @stime:		time spent in kernel mode, in nanoseconds

Why, are those not valid kerneldoc comments?

> @@ -437,20 +437,28 @@ struct sched_dl_entity {
>  	 * during sched_setattr(), they will remain the same until
>  	 * the next sched_setattr().
>  	 */
> -	u64				dl_runtime;	/* Maximum runtime for each instance	*/
> -	u64				dl_deadline;	/* Relative deadline of each instance	*/
> -	u64				dl_period;	/* Separation of two instances (period) */
> -	u64				dl_bw;		/* dl_runtime / dl_period		*/
> -	u64				dl_density;	/* dl_runtime / dl_deadline		*/
> +	/* Maximum runtime for each instance	*/
> +	u64				dl_runtime;
> +	/* Relative deadline of each instance	*/
> +	u64				dl_deadline;
> +	/* Separation of two instances (period) */
> +	u64				dl_period;
> +	/* dl_runtime / dl_period		*/
> +	u64				dl_bw;
> +	/* dl_runtime / dl_deadline		*/
> +	u64				dl_density;

That's a whole lot less readable :/

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH-next] sched/headers: Clean up <linux/sched.h>
  2018-02-15 16:52 ` Peter Zijlstra
@ 2018-02-15 17:10   ` Christopher Díaz Riveros
  2018-02-16  9:44   ` Juri Lelli
  1 sibling, 0 replies; 9+ messages in thread
From: Christopher Díaz Riveros @ 2018-02-15 17:10 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: mingo, linux-kernel, kernel-janitors

Hi Peter, thanks for the reply.


El jue, 15-02-2018 a las 17:52 +0100, Peter Zijlstra escribió:
> On Thu, Feb 15, 2018 at 10:43:18AM -0500, Christopher Diaz Riveros
> wrote:
> > @@ -183,7 +183,7 @@ extern void io_schedule_finish(int token);
> >  extern long io_schedule_timeout(long timeout);
> >  extern void io_schedule(void);
> >  
> > -/**
> > +/*
> >   * struct prev_cputime - snapshot of system and user cputime
> >   * @utime: time spent in user mode
> >   * @stime: time spent in system mode
> > @@ -200,7 +200,7 @@ struct prev_cputime {
> >  #endif
> >  };
> >  
> > -/**
> > +/*
> >   * struct task_cputime - collected CPU time counts
> >   * @utime:		time spent in user mode, in nanoseconds
> >   * @stime:		time spent in kernel mode, in
> > nanoseconds
> 
> Why, are those not valid kerneldoc comments?

Following the same structure from most of the multiline comments in the
file, they tend to be 

/*
 *
 */

I thought it was a typo, and maybe this makes the whole file have a
uniform multiline comment style.

> 
> > @@ -437,20 +437,28 @@ struct sched_dl_entity {
> >  	 * during sched_setattr(), they will remain the same until
> >  	 * the next sched_setattr().
> >  	 */
> > -	u64				dl_runtime;	/*
> > Maximum runtime for each instance	*/
> > -	u64				dl_deadline;	/*
> > Relative deadline of each instance	*/
> > -	u64				dl_period;	/*
> > Separation of two instances (period) */
> > -	u64				dl_bw;		/
> > * dl_runtime / dl_period		*/
> > -	u64				dl_density;	/*
> > dl_runtime / dl_deadline		*/
> > +	/* Maximum runtime for each instance	*/
> > +	u64				dl_runtime;
> > +	/* Relative deadline of each instance	*/
> > +	u64				dl_deadline;
> > +	/* Separation of two instances (period) */
> > +	u64				dl_period;
> > +	/* dl_runtime / dl_period		*/
> > +	u64				dl_bw;
> > +	/* dl_runtime / dl_deadline		*/
> > +	u64				dl_density;
> 
> That's a whole lot less readable :/

Well, while reading the file on a 80 columns width terminal, it breaks
lines producing results like:

/* Maximum runtime for e
ach instance */

/* dl_runtime / dl_deadl
ine */

among others.

In fact, most of the comments in the file follow the structure:

/* comment about var */
type 			identifier

You can see this structure in lines:

631
689
740
781
820
among others.

-- 
Christopher Díaz Riveros
Gentoo Linux Developer
GPG Fingerprint: E517 5ECB 8152 98E4 FEBC  2BAA 4DBB D10F 0FDD 2547

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH-next] sched/headers: Clean up <linux/sched.h>
  2018-02-15 15:43 [PATCH-next] sched/headers: Clean up <linux/sched.h> Christopher Diaz Riveros
  2018-02-15 16:52 ` Peter Zijlstra
@ 2018-02-15 17:49 ` Randy Dunlap
  2018-02-15 18:10   ` Christopher Díaz Riveros
  1 sibling, 1 reply; 9+ messages in thread
From: Randy Dunlap @ 2018-02-15 17:49 UTC (permalink / raw)
  To: Christopher Diaz Riveros, mingo, peterz; +Cc: linux-kernel, kernel-janitors

On 02/15/2018 07:43 AM, Christopher Diaz Riveros wrote:
> Trivial clean up making  comments fit in 80 columns and keeping the same comment style.

Why change the /** (indicates kernel-doc notation) to just /* ?

Is scripts/kernel-doc complaining with warnings or errors?


> Signed-off-by: Christopher Diaz Riveros <chrisadr@gentoo.org>
> ---
>  include/linux/sched.h | 54 +++++++++++++++++++++++++++++++++------------------
>  1 file changed, 35 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index b161ef8a902e..c752a0d48944 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -183,7 +183,7 @@ extern void io_schedule_finish(int token);
>  extern long io_schedule_timeout(long timeout);
>  extern void io_schedule(void);
>  
> -/**
> +/*
>   * struct prev_cputime - snapshot of system and user cputime
>   * @utime: time spent in user mode
>   * @stime: time spent in system mode
> @@ -200,7 +200,7 @@ struct prev_cputime {
>  #endif
>  };
>  
> -/**
> +/*
>   * struct task_cputime - collected CPU time counts
>   * @utime:		time spent in user mode, in nanoseconds
>   * @stime:		time spent in kernel mode, in nanoseconds
> @@ -437,20 +437,28 @@ struct sched_dl_entity {
>  	 * during sched_setattr(), they will remain the same until
>  	 * the next sched_setattr().
>  	 */
> -	u64				dl_runtime;	/* Maximum runtime for each instance	*/
> -	u64				dl_deadline;	/* Relative deadline of each instance	*/
> -	u64				dl_period;	/* Separation of two instances (period) */
> -	u64				dl_bw;		/* dl_runtime / dl_period		*/
> -	u64				dl_density;	/* dl_runtime / dl_deadline		*/
> +	/* Maximum runtime for each instance	*/
> +	u64				dl_runtime;
> +	/* Relative deadline of each instance	*/
> +	u64				dl_deadline;
> +	/* Separation of two instances (period) */
> +	u64				dl_period;
> +	/* dl_runtime / dl_period		*/
> +	u64				dl_bw;
> +	/* dl_runtime / dl_deadline		*/
> +	u64				dl_density;
>  
>  	/*
>  	 * Actual scheduling parameters. Initialized with the values above,
>  	 * they are continously updated during task execution. Note that
>  	 * the remaining runtime could be < 0 in case we are in overrun.
>  	 */
> -	s64				runtime;	/* Remaining runtime for this instance	*/
> -	u64				deadline;	/* Absolute deadline for this instance	*/
> -	unsigned int			flags;		/* Specifying the scheduler behaviour	*/
> +	/* Remaining runtime for this instance	*/
> +	s64				runtime;
> +	/* Absolute deadline for this instance	*/
> +	u64				deadline;
> +	/* Specifying the scheduler behaviour	*/
> +	unsigned int			flags;	
>  
>  	/*
>  	 * Some bool flags:
> @@ -666,7 +674,8 @@ struct task_struct {
>  	unsigned			no_cgroup_migration:1;
>  #endif
>  
> -	unsigned long			atomic_flags; /* Flags requiring atomic access. */
> +	/* Flags requiring atomic access. */
> +	unsigned long			atomic_flags;
>  
>  	struct restart_block		restart_block;
>  
> @@ -678,8 +687,9 @@ struct task_struct {
>  	unsigned long			stack_canary;
>  #endif
>  	/*
> -	 * Pointers to the (original) parent process, youngest child, younger sibling,
> -	 * older sibling, respectively.  (p->father can be replaced with
> +	 * Pointers to the (original) parent process, youngest child,
> +	 * younger sibling, older sibling, respectively.
> +	 * (p->father can be replaced with
>  	 * p->real_parent->pid)
>  	 */
>  
> @@ -743,7 +753,10 @@ struct task_struct {
>  	/* Boot based time in nsecs: */
>  	u64				real_start_time;
>  
> -	/* MM fault and swap info: this can arguably be seen as either mm-specific or thread-specific: */
> +	/* 
> +	 * MM fault and swap info: this can arguably be seen as either
> +	 * mm-specific or thread-specific: 
> +	 */
>  	unsigned long			min_flt;
>  	unsigned long			maj_flt;
>  
> @@ -815,7 +828,10 @@ struct task_struct {
>  	u32				parent_exec_id;
>  	u32				self_exec_id;
>  
> -	/* Protection against (de-)allocation: mm, files, fs, tty, keyrings, mems_allowed, mempolicy: */
> +	/*
> +	 * Protection against (de-)allocation: mm, files, fs, tty,
> +	 * keyrings, mems_allowed, mempolicy: 
> +	 */
>  	spinlock_t			alloc_lock;
>  
>  	/* Protection of the PI data structures: */
> @@ -1176,7 +1192,7 @@ static inline pid_t task_tgid_nr(struct task_struct *tsk)
>  	return tsk->tgid;
>  }
>  
> -/**
> +/*
>   * pid_alive - check that a task structure is not stale
>   * @p: Task structure to be checked.
>   *
> @@ -1275,7 +1291,7 @@ static inline char task_state_to_char(struct task_struct *tsk)
>  	return task_index_to_char(task_state_index(tsk));
>  }
>  
> -/**
> +/*
>   * is_global_init - check if a task structure is init. Since init
>   * is free to have sub-threads we need to check tgid.
>   * @tsk: Task structure to be checked.
> @@ -1422,7 +1438,7 @@ extern int yield_to(struct task_struct *p, bool preempt);
>  extern void set_user_nice(struct task_struct *p, long nice);
>  extern int task_prio(const struct task_struct *p);
>  
> -/**
> +/*
>   * task_nice - return the nice value of a given task.
>   * @p: the task in question.
>   *
> @@ -1442,7 +1458,7 @@ extern int sched_setattr(struct task_struct *, const struct sched_attr *);
>  extern int sched_setattr_nocheck(struct task_struct *, const struct sched_attr *);
>  extern struct task_struct *idle_task(int cpu);
>  
> -/**
> +/*
>   * is_idle_task - is the specified task an idle task?
>   * @p: the task in question.
>   *
> 


-- 
~Randy

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH-next] sched/headers: Clean up <linux/sched.h>
  2018-02-15 17:49 ` [PATCH-next] sched/headers: Clean " Randy Dunlap
@ 2018-02-15 18:10   ` Christopher Díaz Riveros
  0 siblings, 0 replies; 9+ messages in thread
From: Christopher Díaz Riveros @ 2018-02-15 18:10 UTC (permalink / raw)
  To: Randy Dunlap, mingo, peterz; +Cc: linux-kernel, kernel-janitors

El jue, 15-02-2018 a las 09:49 -0800, Randy Dunlap escribió:
> On 02/15/2018 07:43 AM, Christopher Diaz Riveros wrote:
> > Trivial clean up making  comments fit in 80 columns and keeping the
> > same comment style.
> 
> Why change the /** (indicates kernel-doc notation) to just /* ?
> 
> Is scripts/kernel-doc complaining with warnings or errors?
> 

If that's the case, then all the /** were correct and it's just me not
being aware of the meaning of /**.

Thanks Randy for clarifying it to me.

That leaves dl_* comments and a couple of oneline comments that may be
better in multiline format. I can prepare a v2 of that if you consider
it necessary.

> 
> > Signed-off-by: Christopher Diaz Riveros <chrisadr@gentoo.org>
> > ---
> >  include/linux/sched.h | 54 +++++++++++++++++++++++++++++++++----
> > --------------
> >  1 file changed, 35 insertions(+), 19 deletions(-)
> > 
> > diff --git a/include/linux/sched.h b/include/linux/sched.h
> > index b161ef8a902e..c752a0d48944 100644
> > --- a/include/linux/sched.h
> > +++ b/include/linux/sched.h
> > @@ -183,7 +183,7 @@ extern void io_schedule_finish(int token);
> >  extern long io_schedule_timeout(long timeout);
> >  extern void io_schedule(void);
> >  
> > -/**
> > +/*
> >   * struct prev_cputime - snapshot of system and user cputime
> >   * @utime: time spent in user mode
> >   * @stime: time spent in system mode
> > @@ -200,7 +200,7 @@ struct prev_cputime {
> >  #endif
> >  };
> >  
> > -/**
> > +/*
> >   * struct task_cputime - collected CPU time counts
> >   * @utime:		time spent in user mode, in nanoseconds
> >   * @stime:		time spent in kernel mode, in
> > nanoseconds
> > @@ -437,20 +437,28 @@ struct sched_dl_entity {
> >  	 * during sched_setattr(), they will remain the same until
> >  	 * the next sched_setattr().
> >  	 */
> > -	u64				dl_runtime;	/*
> > Maximum runtime for each instance	*/
> > -	u64				dl_deadline;	/*
> > Relative deadline of each instance	*/
> > -	u64				dl_period;	/*
> > Separation of two instances (period) */
> > -	u64				dl_bw;		/
> > * dl_runtime / dl_period		*/
> > -	u64				dl_density;	/*
> > dl_runtime / dl_deadline		*/
> > +	/* Maximum runtime for each instance	*/
> > +	u64				dl_runtime;
> > +	/* Relative deadline of each instance	*/
> > +	u64				dl_deadline;
> > +	/* Separation of two instances (period) */
> > +	u64				dl_period;
> > +	/* dl_runtime / dl_period		*/
> > +	u64				dl_bw;
> > +	/* dl_runtime / dl_deadline		*/
> > +	u64				dl_density;
> >  
> >  	/*
> >  	 * Actual scheduling parameters. Initialized with the
> > values above,
> >  	 * they are continously updated during task execution.
> > Note that
> >  	 * the remaining runtime could be < 0 in case we are in
> > overrun.
> >  	 */
> > -	s64				runtime;	/*
> > Remaining runtime for this instance	*/
> > -	u64				deadline;	/*
> > Absolute deadline for this instance	*/
> > -	unsigned int			flags;		
> > /* Specifying the scheduler behaviour	*/
> > +	/* Remaining runtime for this instance	*/
> > +	s64				runtime;
> > +	/* Absolute deadline for this instance	*/
> > +	u64				deadline;
> > +	/* Specifying the scheduler behaviour	*/
> > +	unsigned int			flags;	
> >  
> >  	/*
> >  	 * Some bool flags:
> > @@ -666,7 +674,8 @@ struct task_struct {
> >  	unsigned			no_cgroup_migration:1;
> >  #endif
> >  
> > -	unsigned long			atomic_flags; /*
> > Flags requiring atomic access. */
> > +	/* Flags requiring atomic access. */
> > +	unsigned long			atomic_flags;
> >  
> >  	struct restart_block		restart_block;
> >  
> > @@ -678,8 +687,9 @@ struct task_struct {
> >  	unsigned long			stack_canary;
> >  #endif
> >  	/*
> > -	 * Pointers to the (original) parent process, youngest
> > child, younger sibling,
> > -	 * older sibling, respectively.  (p->father can be
> > replaced with
> > +	 * Pointers to the (original) parent process, youngest
> > child,
> > +	 * younger sibling, older sibling, respectively.
> > +	 * (p->father can be replaced with
> >  	 * p->real_parent->pid)
> >  	 */
> >  
> > @@ -743,7 +753,10 @@ struct task_struct {
> >  	/* Boot based time in nsecs: */
> >  	u64				real_start_time;
> >  
> > -	/* MM fault and swap info: this can arguably be seen as
> > either mm-specific or thread-specific: */
> > +	/* 
> > +	 * MM fault and swap info: this can arguably be seen as
> > either
> > +	 * mm-specific or thread-specific: 
> > +	 */
> >  	unsigned long			min_flt;
> >  	unsigned long			maj_flt;
> >  
> > @@ -815,7 +828,10 @@ struct task_struct {
> >  	u32				parent_exec_id;
> >  	u32				self_exec_id;
> >  
> > -	/* Protection against (de-)allocation: mm, files, fs, tty,
> > keyrings, mems_allowed, mempolicy: */
> > +	/*
> > +	 * Protection against (de-)allocation: mm, files, fs, tty,
> > +	 * keyrings, mems_allowed, mempolicy: 
> > +	 */
> >  	spinlock_t			alloc_lock;
> >  
> >  	/* Protection of the PI data structures: */
> > @@ -1176,7 +1192,7 @@ static inline pid_t task_tgid_nr(struct
> > task_struct *tsk)
> >  	return tsk->tgid;
> >  }
> >  
> > -/**
> > +/*
> >   * pid_alive - check that a task structure is not stale
> >   * @p: Task structure to be checked.
> >   *
> > @@ -1275,7 +1291,7 @@ static inline char task_state_to_char(struct
> > task_struct *tsk)
> >  	return task_index_to_char(task_state_index(tsk));
> >  }
> >  
> > -/**
> > +/*
> >   * is_global_init - check if a task structure is init. Since init
> >   * is free to have sub-threads we need to check tgid.
> >   * @tsk: Task structure to be checked.
> > @@ -1422,7 +1438,7 @@ extern int yield_to(struct task_struct *p,
> > bool preempt);
> >  extern void set_user_nice(struct task_struct *p, long nice);
> >  extern int task_prio(const struct task_struct *p);
> >  
> > -/**
> > +/*
> >   * task_nice - return the nice value of a given task.
> >   * @p: the task in question.
> >   *
> > @@ -1442,7 +1458,7 @@ extern int sched_setattr(struct task_struct
> > *, const struct sched_attr *);
> >  extern int sched_setattr_nocheck(struct task_struct *, const
> > struct sched_attr *);
> >  extern struct task_struct *idle_task(int cpu);
> >  
> > -/**
> > +/*
> >   * is_idle_task - is the specified task an idle task?
> >   * @p: the task in question.
> >   *
> > 
> 
> 
-- 
Christopher Díaz Riveros
Gentoo Linux Developer
GPG Fingerprint: E517 5ECB 8152 98E4 FEBC  2BAA 4DBB D10F 0FDD 2547

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH-next] sched/headers: Clean up <linux/sched.h>
  2018-02-15 16:52 ` Peter Zijlstra
  2018-02-15 17:10   ` Christopher Díaz Riveros
@ 2018-02-16  9:44   ` Juri Lelli
  2018-02-16 13:25     ` Christopher Díaz Riveros
  1 sibling, 1 reply; 9+ messages in thread
From: Juri Lelli @ 2018-02-16  9:44 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Christopher Diaz Riveros, mingo, linux-kernel, kernel-janitors

On 15/02/18 17:52, Peter Zijlstra wrote:
> On Thu, Feb 15, 2018 at 10:43:18AM -0500, Christopher Diaz Riveros wrote:

[...]

> > @@ -437,20 +437,28 @@ struct sched_dl_entity {
> >  	 * during sched_setattr(), they will remain the same until
> >  	 * the next sched_setattr().
> >  	 */
> > -	u64				dl_runtime;	/* Maximum runtime for each instance	*/
> > -	u64				dl_deadline;	/* Relative deadline of each instance	*/
> > -	u64				dl_period;	/* Separation of two instances (period) */
> > -	u64				dl_bw;		/* dl_runtime / dl_period		*/
> > -	u64				dl_density;	/* dl_runtime / dl_deadline		*/
> > +	/* Maximum runtime for each instance	*/
> > +	u64				dl_runtime;
> > +	/* Relative deadline of each instance	*/
> > +	u64				dl_deadline;
> > +	/* Separation of two instances (period) */
> > +	u64				dl_period;
> > +	/* dl_runtime / dl_period		*/
> > +	u64				dl_bw;
> > +	/* dl_runtime / dl_deadline		*/
> > +	u64				dl_density;
> 
> That's a whole lot less readable :/

Yep. :(

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH-next] sched/headers: Clean up <linux/sched.h>
  2018-02-16  9:44   ` Juri Lelli
@ 2018-02-16 13:25     ` Christopher Díaz Riveros
  2018-02-21  8:09       ` Juri Lelli
  2018-02-21 16:56       ` [RFC] sched/headers: comments clean " Christopher Diaz Riveros
  0 siblings, 2 replies; 9+ messages in thread
From: Christopher Díaz Riveros @ 2018-02-16 13:25 UTC (permalink / raw)
  To: Juri Lelli, Peter Zijlstra; +Cc: mingo, linux-kernel, kernel-janitors

El vie, 16-02-2018 a las 10:44 +0100, Juri Lelli escribió:
> On 15/02/18 17:52, Peter Zijlstra wrote:
> > On Thu, Feb 15, 2018 at 10:43:18AM -0500, Christopher Diaz Riveros
> > wrote:
> 
> [...]
> 
> > > @@ -437,20 +437,28 @@ struct sched_dl_entity {
> > >  	 * during sched_setattr(), they will remain the same
> > > until
> > >  	 * the next sched_setattr().
> > >  	 */
> > > -	u64				dl_runtime;	/*
> > > Maximum runtime for each instance	*/
> > > -	u64				dl_deadline;	/
> > > * Relative deadline of each instance	*/
> > > -	u64				dl_period;	/*
> > > Separation of two instances (period) */
> > > -	u64				dl_bw;		
> > > /* dl_runtime / dl_period		*/
> > > -	u64				dl_density;	/*
> > > dl_runtime / dl_deadline		*/
> > > +	/* Maximum runtime for each instance	*/
> > > +	u64				dl_runtime;
> > > +	/* Relative deadline of each instance	*/
> > > +	u64				dl_deadline;
> > > +	/* Separation of two instances (period) */
> > > +	u64				dl_period;
> > > +	/* dl_runtime / dl_period		*/
> > > +	u64				dl_bw;
> > > +	/* dl_runtime / dl_deadline		*/
> > > +	u64				dl_density;
> > 
> > That's a whole lot less readable :/
> 
> Yep. :(

Thank you all for the feedback, I'll consider this patch as NACK. Sorry
 for wasting time in a low quality patch. I'll prepare a better one
next time :)

Regards,
-- 
Christopher Díaz Riveros
Gentoo Linux Developer
GPG Fingerprint: E517 5ECB 8152 98E4 FEBC  2BAA 4DBB D10F 0FDD 2547

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH-next] sched/headers: Clean up <linux/sched.h>
  2018-02-16 13:25     ` Christopher Díaz Riveros
@ 2018-02-21  8:09       ` Juri Lelli
  2018-02-21 16:56       ` [RFC] sched/headers: comments clean " Christopher Diaz Riveros
  1 sibling, 0 replies; 9+ messages in thread
From: Juri Lelli @ 2018-02-21  8:09 UTC (permalink / raw)
  To: Christopher Díaz Riveros
  Cc: Peter Zijlstra, mingo, linux-kernel, kernel-janitors

On 16/02/18 08:25, Christopher Díaz Riveros wrote:
> El vie, 16-02-2018 a las 10:44 +0100, Juri Lelli escribió:
> > On 15/02/18 17:52, Peter Zijlstra wrote:
> > > On Thu, Feb 15, 2018 at 10:43:18AM -0500, Christopher Diaz Riveros
> > > wrote:
> > 
> > [...]
> > 
> > > > @@ -437,20 +437,28 @@ struct sched_dl_entity {
> > > >  	 * during sched_setattr(), they will remain the same
> > > > until
> > > >  	 * the next sched_setattr().
> > > >  	 */
> > > > -	u64				dl_runtime;	/*
> > > > Maximum runtime for each instance	*/
> > > > -	u64				dl_deadline;	/
> > > > * Relative deadline of each instance	*/
> > > > -	u64				dl_period;	/*
> > > > Separation of two instances (period) */
> > > > -	u64				dl_bw;		
> > > > /* dl_runtime / dl_period		*/
> > > > -	u64				dl_density;	/*
> > > > dl_runtime / dl_deadline		*/
> > > > +	/* Maximum runtime for each instance	*/
> > > > +	u64				dl_runtime;
> > > > +	/* Relative deadline of each instance	*/
> > > > +	u64				dl_deadline;
> > > > +	/* Separation of two instances (period) */
> > > > +	u64				dl_period;
> > > > +	/* dl_runtime / dl_period		*/
> > > > +	u64				dl_bw;
> > > > +	/* dl_runtime / dl_deadline		*/
> > > > +	u64				dl_density;
> > > 
> > > That's a whole lot less readable :/
> > 
> > Yep. :(
> 
> Thank you all for the feedback, I'll consider this patch as NACK. Sorry
>  for wasting time in a low quality patch. I'll prepare a better one
> next time :)

No problem, thanks actually to seeing if things can be cleaned up. :)

While going through that struct again I was thinking that we might want
to completely remove inline comments and put them in the above comment
block(s), as we already have for bool flags:

  /*
   * Some bool flags:
   *
   * @dl_throttled tells if we exhausted the runtime. If so, the
   * task has to wait for a replenishment to be performed at the
   * next firing of dl_timer.
   [...]

Would it be OK and any better?

Thanks,

- Juri

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [RFC] sched/headers: comments clean up <linux/sched.h>
  2018-02-16 13:25     ` Christopher Díaz Riveros
  2018-02-21  8:09       ` Juri Lelli
@ 2018-02-21 16:56       ` Christopher Diaz Riveros
  1 sibling, 0 replies; 9+ messages in thread
From: Christopher Diaz Riveros @ 2018-02-21 16:56 UTC (permalink / raw)
  To: juri.lelli, peterz, mingo; +Cc: linux-kernel, kernel-janitors

Remove inline comments to merge them into comment blocks.

Enhance readability from source code by giving descriptions from the
structures and data in comment blocks instead from inline comments.

Signed-off-by: Christopher Diaz Riveros <chrisadr@gentoo.org>
---
 include/linux/sched.h | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index b161ef8a902e..693a6e51ed0c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -221,12 +221,21 @@ struct task_cputime {
 #define prof_exp			stime
 #define sched_exp			sum_exec_runtime
 
+/*
+ * vtime_state states:
+ *
+ * @VTIME_INACTIVE tells if task is sleeping or running in a CPU with
+ * VTIME inactive.
+ *
+ * @VTIME_USER tells if task is running in userspace in a CPU with
+ * VTIME active.
+ *
+ * @VTIME_SYS tells if task is running in kernelspace in a CPU with
+ * VTIME active.
+ */
 enum vtime_state {
-	/* Task is sleeping or running in a CPU with VTIME inactive: */
 	VTIME_INACTIVE = 0,
-	/* Task runs in userspace in a CPU with VTIME active: */
 	VTIME_USER,
-	/* Task runs in kernelspace in a CPU with VTIME active: */
 	VTIME_SYS,
 };
 
-- 
2.16.2

^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2018-02-21 16:56 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-15 15:43 [PATCH-next] sched/headers: Clean up <linux/sched.h> Christopher Diaz Riveros
2018-02-15 16:52 ` Peter Zijlstra
2018-02-15 17:10   ` Christopher Díaz Riveros
2018-02-16  9:44   ` Juri Lelli
2018-02-16 13:25     ` Christopher Díaz Riveros
2018-02-21  8:09       ` Juri Lelli
2018-02-21 16:56       ` [RFC] sched/headers: comments clean " Christopher Diaz Riveros
2018-02-15 17:49 ` [PATCH-next] sched/headers: Clean " Randy Dunlap
2018-02-15 18:10   ` Christopher Díaz Riveros

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).