[PATCH v2 1/3] kernel: pid: Update documentation.

[PATCH v2 1/3] kernel: pid: Update documentation.

[Varsha Rao]

This patch adds comments to update documentation.

Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
---
Changes in v2:
- No change.

 kernel/pid.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/kernel/pid.c b/kernel/pid.c
index 0143ac0..0b0aad4 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -43,6 +43,7 @@
 #define pid_hashfn(nr, ns)	\
 	hash_long((unsigned long)nr + (unsigned long)ns, pidhash_shift)
 static struct hlist_head *pid_hash;
+/* Doubly linked hash lists to find PID in given namespace. */
 static unsigned int pidhash_shift = 4;
 struct pid init_struct_pid = INIT_STRUCT_PID;
 
@@ -53,6 +54,7 @@ int pid_max = PID_MAX_DEFAULT;
 int pid_max_min = RESERVED_PIDS + 1;
 int pid_max_max = PID_MAX_LIMIT;
 
+/* mk_pid() returns distance between map and offset pidmap. */
 static inline int mk_pid(struct pid_namespace *pid_ns,
 		struct pidmap *map, int off)
 {
@@ -101,6 +103,7 @@ EXPORT_SYMBOL_GPL(init_pid_ns);
 
 static  __cacheline_aligned_in_smp DEFINE_SPINLOCK(pidmap_lock);
 
+/* free_pidmap() : It frees process id. */
 static void free_pidmap(struct upid *upid)
 {
 	int nr = upid->nr;
@@ -150,6 +153,9 @@ static void set_last_pid(struct pid_namespace *pid_ns, int base, int pid)
 	} while ((prev != last_write) && (pid_before(base, last_write, pid)));
 }
 
+/* alloc_pidmap(): Increments last pid by one and returns it if new pid
+ * is valid.
+ */
 static int alloc_pidmap(struct pid_namespace *pid_ns)
 {
 	int i, offset, max_scan, pid, last = pid_ns->last_pid;
@@ -212,6 +218,9 @@ static int alloc_pidmap(struct pid_namespace *pid_ns)
 	return -EAGAIN;
 }
 
+/* next_pidmap() is used to find the first pid which is greater than previous
+ * last allocated pid.
+ */
 int next_pidmap(struct pid_namespace *pid_ns, unsigned int last)
 {
 	int offset;
@@ -233,6 +242,7 @@ int next_pidmap(struct pid_namespace *pid_ns, unsigned int last)
 	return -1;
 }
 
+/* put_pid() checks whether pid can be reallocated. */
 void put_pid(struct pid *pid)
 {
 	struct pid_namespace *ns;
@@ -249,6 +259,9 @@ void put_pid(struct pid *pid)
 }
 EXPORT_SYMBOL_GPL(put_pid);
 
+/* delayed_put_pid(): Finds the real location of struct pid in the
+ * memory and checks whether it can be reallocated with put_pid().
+ */
 static void delayed_put_pid(struct rcu_head *rhp)
 {
 	struct pid *pid = container_of(rhp, struct pid, rcu);
@@ -293,6 +306,9 @@ void free_pid(struct pid *pid)
 	call_rcu(&pid->rcu, delayed_put_pid);
 }
 
+/* alloc_pid(): Allocates local pid for each multiple namespaces
+ * in which new process created is visible.
+ */
 struct pid *alloc_pid(struct pid_namespace *ns)
 {
 	struct pid *pid;
-- 
2.9.3

[PATCH v2 2/3] kernel: pid: Change function return value to bool.

[Varsha Rao]

Remove unsign cast and add bool as return type for pid_before()
function. As the function is called only once in condition for do
while loop. This patch fixes the checkpatch issue.

Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
---
Changes in v2:
- Modified commit message.
- Changed return type of pid_before() function.

 kernel/pid.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/pid.c b/kernel/pid.c
index 0b0aad4..c6f7b89 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -117,7 +117,7 @@ static void free_pidmap(struct upid *upid)
 /*
  * If we started walking pids at 'base', is 'a' seen before 'b'?
  */
-static int pid_before(int base, int a, int b)
+static bool pid_before(int base, int a, int b)
 {
 	/*
 	 * This is the same as saying
@@ -125,7 +125,7 @@ static int pid_before(int base, int a, int b)
 	 * (a - base + MAXUINT) % MAXUINT < (b - base + MAXUINT) % MAXUINT
 	 * and that mapping orders 'a' and 'b' with respect to 'base'.
 	 */
-	return (unsigned)(a - base) < (unsigned)(b - base);
+	return (a - base) < (b - base);
 }
 
 /*
-- 
2.9.3

[PATCH v2 3/3] kernel: pid: Add blank line after declarations.

[Varsha Rao]

Add a blank line after declarations, to fix the checkpatch issue.

Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
---
Changes in v2:
- No changes.

 kernel/pid.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/kernel/pid.c b/kernel/pid.c
index c6f7b89..7eeb20b 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -147,6 +147,7 @@ static void set_last_pid(struct pid_namespace *pid_ns, int base, int pid)
 {
 	int prev;
 	int last_write = base;
+
 	do {
 		prev = last_write;
 		last_write = cmpxchg(&pid_ns->last_pid, prev, pid);
@@ -265,6 +266,7 @@ EXPORT_SYMBOL_GPL(put_pid);
 static void delayed_put_pid(struct rcu_head *rhp)
 {
 	struct pid *pid = container_of(rhp, struct pid, rcu);
+
 	put_pid(pid);
 }
 
@@ -404,6 +406,7 @@ EXPORT_SYMBOL_GPL(find_vpid);
 void attach_pid(struct task_struct *task, enum pid_type type)
 {
 	struct pid_link *link = &task->pids[type];
+
 	hlist_add_head_rcu(&link->node, &link->pid->tasks[type]);
 }
 
@@ -450,8 +453,10 @@ void transfer_pid(struct task_struct *old, struct task_struct *new,
 struct task_struct *pid_task(struct pid *pid, enum pid_type type)
 {
 	struct task_struct *result = NULL;
+
 	if (pid) {
 		struct hlist_node *first;
+
 		first = rcu_dereference_check(hlist_first_rcu(&pid->tasks[type]),
 					      lockdep_tasklist_lock_is_held());
 		if (first)
@@ -479,6 +484,7 @@ struct task_struct *find_task_by_vpid(pid_t vnr)
 struct pid *get_task_pid(struct task_struct *task, enum pid_type type)
 {
 	struct pid *pid;
+
 	rcu_read_lock();
 	if (type != PIDTYPE_PID)
 		task = task->group_leader;
@@ -491,6 +497,7 @@ EXPORT_SYMBOL_GPL(get_task_pid);
 struct task_struct *get_pid_task(struct pid *pid, enum pid_type type)
 {
 	struct task_struct *result;
+
 	rcu_read_lock();
 	result = pid_task(pid, type);
 	if (result)
-- 
2.9.3

Re: [Outreachy kernel] [PATCH v2 2/3] kernel: pid: Change function return value to bool.

[Julia Lawall]

On Mon, 27 Mar 2017, Varsha Rao wrote:

> Remove unsign cast and add bool as return type for pid_before()
> function.

unsign -> unsigned.  But are you sure that the change is correct?  For
example -1 < 1 but (unsigned) -1 > (unsigned) 1.

julia

> As the function is called only once in condition for do
> while loop. This patch fixes the checkpatch issue.
>
> Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
> ---
> Changes in v2:
> - Modified commit message.
> - Changed return type of pid_before() function.
>
>  kernel/pid.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 0b0aad4..c6f7b89 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -117,7 +117,7 @@ static void free_pidmap(struct upid *upid)
>  /*
>   * If we started walking pids at 'base', is 'a' seen before 'b'?
>   */
> -static int pid_before(int base, int a, int b)
> +static bool pid_before(int base, int a, int b)
>  {
>  	/*
>  	 * This is the same as saying
> @@ -125,7 +125,7 @@ static int pid_before(int base, int a, int b)
>  	 * (a - base + MAXUINT) % MAXUINT < (b - base + MAXUINT) % MAXUINT
>  	 * and that mapping orders 'a' and 'b' with respect to 'base'.
>  	 */
> -	return (unsigned)(a - base) < (unsigned)(b - base);
> +	return (a - base) < (b - base);
>  }
>
>  /*
> --
> 2.9.3
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/58d87c7c.c240630a.d8622.c768%40mx.google.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH v2 1/3] kernel: pid: Update documentation.

[Julia Lawall]

On Mon, 27 Mar 2017, Varsha Rao wrote:

> This patch adds comments to update documentation.

The format used is not consistent.  There is

fn(): text

fn() text

fn() : text

Acrually, the standard approach seems to be:

fn() - text

Also, you can use the imperative for the descriptions.  Examples below.

>
> Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
> ---
> Changes in v2:
> - No change.
>
>  kernel/pid.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 0143ac0..0b0aad4 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -43,6 +43,7 @@
>  #define pid_hashfn(nr, ns)	\
>  	hash_long((unsigned long)nr + (unsigned long)ns, pidhash_shift)
>  static struct hlist_head *pid_hash;
> +/* Doubly linked hash lists to find PID in given namespace. */
>  static unsigned int pidhash_shift = 4;
>  struct pid init_struct_pid = INIT_STRUCT_PID;
>
> @@ -53,6 +54,7 @@ int pid_max = PID_MAX_DEFAULT;
>  int pid_max_min = RESERVED_PIDS + 1;
>  int pid_max_max = PID_MAX_LIMIT;
>
> +/* mk_pid() returns distance between map and offset pidmap. */

For example:

/* mk_pid() - return distance between map and offset pidmap. */

>  static inline int mk_pid(struct pid_namespace *pid_ns,
>  		struct pidmap *map, int off)
>  {
> @@ -101,6 +103,7 @@ EXPORT_SYMBOL_GPL(init_pid_ns);
>
>  static  __cacheline_aligned_in_smp DEFINE_SPINLOCK(pidmap_lock);
>
> +/* free_pidmap() : It frees process id. */

/* free_pidmap() - free process id */

etc.

julia

>  static void free_pidmap(struct upid *upid)
>  {
>  	int nr = upid->nr;
> @@ -150,6 +153,9 @@ static void set_last_pid(struct pid_namespace *pid_ns, int base, int pid)
>  	} while ((prev != last_write) && (pid_before(base, last_write, pid)));
>  }
>
> +/* alloc_pidmap(): Increments last pid by one and returns it if new pid
> + * is valid.
> + */
>  static int alloc_pidmap(struct pid_namespace *pid_ns)
>  {
>  	int i, offset, max_scan, pid, last = pid_ns->last_pid;
> @@ -212,6 +218,9 @@ static int alloc_pidmap(struct pid_namespace *pid_ns)
>  	return -EAGAIN;
>  }
>
> +/* next_pidmap() is used to find the first pid which is greater than previous
> + * last allocated pid.
> + */
>  int next_pidmap(struct pid_namespace *pid_ns, unsigned int last)
>  {
>  	int offset;
> @@ -233,6 +242,7 @@ int next_pidmap(struct pid_namespace *pid_ns, unsigned int last)
>  	return -1;
>  }
>
> +/* put_pid() checks whether pid can be reallocated. */
>  void put_pid(struct pid *pid)
>  {
>  	struct pid_namespace *ns;
> @@ -249,6 +259,9 @@ void put_pid(struct pid *pid)
>  }
>  EXPORT_SYMBOL_GPL(put_pid);
>
> +/* delayed_put_pid(): Finds the real location of struct pid in the
> + * memory and checks whether it can be reallocated with put_pid().
> + */
>  static void delayed_put_pid(struct rcu_head *rhp)
>  {
>  	struct pid *pid = container_of(rhp, struct pid, rcu);
> @@ -293,6 +306,9 @@ void free_pid(struct pid *pid)
>  	call_rcu(&pid->rcu, delayed_put_pid);
>  }
>
> +/* alloc_pid(): Allocates local pid for each multiple namespaces
> + * in which new process created is visible.
> + */
>  struct pid *alloc_pid(struct pid_namespace *ns)
>  {
>  	struct pid *pid;
> --
> 2.9.3
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/58d87ab9.5318620a.e4705.ca98%40mx.google.com.
> For more options, visit https://groups.google.com/d/optout.
>

Re: [Outreachy kernel] [PATCH v2 1/3] kernel: pid: Update documentation.

[Julia Lawall]

On Mon, 27 Mar 2017, Julia Lawall wrote:

> On Mon, 27 Mar 2017, Varsha Rao wrote:
>
> > This patch adds comments to update documentation.
>
> The format used is not consistent.  There is
>
> fn(): text
>
> fn() text
>
> fn() : text
>
> Acrually, the standard approach seems to be:
>
> fn() - text
>
> Also, you can use the imperative for the descriptions.  Examples below.

Documentation also has a different comment notation:

/**
 * text
 */

julia

>
> >
> > Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
> > ---
> > Changes in v2:
> > - No change.
> >
> >  kernel/pid.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/kernel/pid.c b/kernel/pid.c
> > index 0143ac0..0b0aad4 100644
> > --- a/kernel/pid.c
> > +++ b/kernel/pid.c
> > @@ -43,6 +43,7 @@
> >  #define pid_hashfn(nr, ns)	\
> >  	hash_long((unsigned long)nr + (unsigned long)ns, pidhash_shift)
> >  static struct hlist_head *pid_hash;
> > +/* Doubly linked hash lists to find PID in given namespace. */
> >  static unsigned int pidhash_shift = 4;
> >  struct pid init_struct_pid = INIT_STRUCT_PID;
> >
> > @@ -53,6 +54,7 @@ int pid_max = PID_MAX_DEFAULT;
> >  int pid_max_min = RESERVED_PIDS + 1;
> >  int pid_max_max = PID_MAX_LIMIT;
> >
> > +/* mk_pid() returns distance between map and offset pidmap. */
>
> For example:
>
> /* mk_pid() - return distance between map and offset pidmap. */
>
> >  static inline int mk_pid(struct pid_namespace *pid_ns,
> >  		struct pidmap *map, int off)
> >  {
> > @@ -101,6 +103,7 @@ EXPORT_SYMBOL_GPL(init_pid_ns);
> >
> >  static  __cacheline_aligned_in_smp DEFINE_SPINLOCK(pidmap_lock);
> >
> > +/* free_pidmap() : It frees process id. */
>
> /* free_pidmap() - free process id */
>
> etc.
>
> julia
>
> >  static void free_pidmap(struct upid *upid)
> >  {
> >  	int nr = upid->nr;
> > @@ -150,6 +153,9 @@ static void set_last_pid(struct pid_namespace *pid_ns, int base, int pid)
> >  	} while ((prev != last_write) && (pid_before(base, last_write, pid)));
> >  }
> >
> > +/* alloc_pidmap(): Increments last pid by one and returns it if new pid
> > + * is valid.
> > + */
> >  static int alloc_pidmap(struct pid_namespace *pid_ns)
> >  {
> >  	int i, offset, max_scan, pid, last = pid_ns->last_pid;
> > @@ -212,6 +218,9 @@ static int alloc_pidmap(struct pid_namespace *pid_ns)
> >  	return -EAGAIN;
> >  }
> >
> > +/* next_pidmap() is used to find the first pid which is greater than previous
> > + * last allocated pid.
> > + */
> >  int next_pidmap(struct pid_namespace *pid_ns, unsigned int last)
> >  {
> >  	int offset;
> > @@ -233,6 +242,7 @@ int next_pidmap(struct pid_namespace *pid_ns, unsigned int last)
> >  	return -1;
> >  }
> >
> > +/* put_pid() checks whether pid can be reallocated. */
> >  void put_pid(struct pid *pid)
> >  {
> >  	struct pid_namespace *ns;
> > @@ -249,6 +259,9 @@ void put_pid(struct pid *pid)
> >  }
> >  EXPORT_SYMBOL_GPL(put_pid);
> >
> > +/* delayed_put_pid(): Finds the real location of struct pid in the
> > + * memory and checks whether it can be reallocated with put_pid().
> > + */
> >  static void delayed_put_pid(struct rcu_head *rhp)
> >  {
> >  	struct pid *pid = container_of(rhp, struct pid, rcu);
> > @@ -293,6 +306,9 @@ void free_pid(struct pid *pid)
> >  	call_rcu(&pid->rcu, delayed_put_pid);
> >  }
> >
> > +/* alloc_pid(): Allocates local pid for each multiple namespaces
> > + * in which new process created is visible.
> > + */
> >  struct pid *alloc_pid(struct pid_namespace *ns)
> >  {
> >  	struct pid *pid;
> > --
> > 2.9.3
> >
> > --
> > You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> > To post to this group, send email to outreachy-kernel@googlegroups.com.
> > To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/58d87ab9.5318620a.e4705.ca98%40mx.google.com.
> > For more options, visit https://groups.google.com/d/optout.
> >
>

Re: [Outreachy kernel] [PATCH v2 2/3] kernel: pid: Change function return value to bool.

[Varsha Rao]

[-- Attachment #1.1: Type: text/plain, Size: 424 bytes --]



> On Mon, 27 Mar 2017, Varsha Rao wrote: 
>
> > Remove unsign cast and add bool as return type for pid_before() 
> > function. 
>
> unsign -> unsigned.  But are you sure that the change is correct?  For 
> example -1 < 1 but (unsigned) -1 > (unsigned) 1. 
>
>
   The change is incorrect. I will make both the differences as absolute 
   value, I guess then the change will be correct. 
    
   Regards,
   Varsha Rao
    

[-- Attachment #1.2: Type: text/html, Size: 659 bytes --]

Re: [Outreachy kernel] [PATCH v2 2/3] kernel: pid: Change function return value to bool.

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1235 bytes --]



On Mon, 27 Mar 2017, Varsha Rao wrote:

>
>       On Mon, 27 Mar 2017, Varsha Rao wrote:
>
>       > Remove unsign cast and add bool as return type for
>       pid_before()
>       > function.
>
>       unsign -> unsigned.  But are you sure that the change is
>       correct?  For
>       example -1 < 1 but (unsigned) -1 > (unsigned) 1.
>
>
>    The change is incorrect. I will make both the differences as absolute
>    value, I guess then the change will be correct.

Not sure.  You would have to provide an argument as to why the change is
correct.  Absolute value also requires some computation, while casting
requires none.

julia

>    
>    Regards,
>    Varsha Rao
>    
>
> --
> You received this message because you are subscribed to the Google Groups
> "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visithttps://groups.google.com/d/msgid/outreachy-kernel/6874a8e7-7574-487b-9423-
> 89bad7e1145a%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
>

Re: [Outreachy kernel] [PATCH v2 2/3] kernel: pid: Change function return value to bool.

[Varsha Rao]

[-- Attachment #1.1: Type: text/plain, Size: 1167 bytes --]



On Monday, March 27, 2017 at 1:35:06 PM UTC+5:30, Julia Lawall wrote:
>
>
>
> On Mon, 27 Mar 2017, Varsha Rao wrote: 
>
> > 
> >       On Mon, 27 Mar 2017, Varsha Rao wrote: 
> > 
> >       > Remove unsign cast and add bool as return type for 
> >       pid_before() 
> >       > function. 
> > 
> >       unsign -> unsigned.  But are you sure that the change is 
> >       correct?  For 
> >       example -1 < 1 but (unsigned) -1 > (unsigned) 1. 
> > 
> > 
> >    The change is incorrect. I will make both the differences as absolute 
> >    value, I guess then the change will be correct. 
>
> Not sure.  You would have to provide an argument as to why the change is 
> correct.  Absolute value also requires some computation, while casting 
> requires none. 
>

The usage of unsigned is to increase range and consider positive value.
It would be better to use unsigned int only.

If unsigned int is used, a - base won't exceed MAXUINT value and therefore
there will be no negative result. As a, b and base are of type int. 
(a - base + MAXUINT) % MAXUINT < (b - base + MAXUINT) % MAXUINT

If abs is considered then the range would decrease.

Thanks,
Varsha Rao

[-- Attachment #1.2: Type: text/html, Size: 1510 bytes --]

Re: [Outreachy kernel] [PATCH v2 2/3] kernel: pid: Change function return value to bool.

[Varsha Rao]

[-- Attachment #1.1: Type: text/plain, Size: 591 bytes --]


The return type can be bool but with the unsigned int casting the
differences. Then the change would make sense.

+static bool pid_before(int base, int a, int b)
 {
        /*
         * This is the same as saying
@@ -125,7 +125,7 @@ static int pid_before(int base, int a, int b)
         * (a - base + MAXUINT) % MAXUINT < (b - base + MAXUINT) % MAXUINT
         * and that mapping orders 'a' and 'b' with respect to 'base'.
         */
-       return (unsigned)(a - base) < (unsigned)(b - base);
+       return (unsigned int)(a - base) < (unsigned int)(b - base);
 }


Thanks,
Varsha Rao

[-- Attachment #1.2: Type: text/html, Size: 755 bytes --]

RE: [PATCH v2 1/3] kernel: pid: Update documentation.

[Matthew Wilcox]

You're inconsistent in where you put the documentation.  Traditionally, it's either on the same line as or immediately preceding the thing you're documenting, not on the line after.

Also, if you're adding documentation which is useful to the user of the interface (as opposed to documentation useful for people working on the implementation), it's a good idea to use the kernel-doc format, documented in Documentation/doc-guide/kernel-doc.rst

> -----Original Message-----
> From: Varsha Rao [mailto:rvarsha016@gmail.com]
> Sent: Sunday, March 26, 2017 10:37 PM
> To: Matthew Wilcox <mawilcox@microsoft.com>
> Cc: outreachy-kernel <outreachy-kernel@googlegroups.com>
> Subject: [PATCH v2 1/3] kernel: pid: Update documentation.
> 
> This patch adds comments to update documentation.
> 
> Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
> ---
> Changes in v2:
> - No change.
> 
>  kernel/pid.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 0143ac0..0b0aad4 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -43,6 +43,7 @@
>  #define pid_hashfn(nr, ns)	\
>  	hash_long((unsigned long)nr + (unsigned long)ns, pidhash_shift)
>  static struct hlist_head *pid_hash;
> +/* Doubly linked hash lists to find PID in given namespace. */
>  static unsigned int pidhash_shift = 4;
>  struct pid init_struct_pid = INIT_STRUCT_PID;
> 
> @@ -53,6 +54,7 @@ int pid_max = PID_MAX_DEFAULT;
>  int pid_max_min = RESERVED_PIDS + 1;
>  int pid_max_max = PID_MAX_LIMIT;
> 
> +/* mk_pid() returns distance between map and offset pidmap. */
>  static inline int mk_pid(struct pid_namespace *pid_ns,
>  		struct pidmap *map, int off)
>  {
> @@ -101,6 +103,7 @@ EXPORT_SYMBOL_GPL(init_pid_ns);
> 
>  static  __cacheline_aligned_in_smp DEFINE_SPINLOCK(pidmap_lock);
> 
> +/* free_pidmap() : It frees process id. */
>  static void free_pidmap(struct upid *upid)
>  {
>  	int nr = upid->nr;
> @@ -150,6 +153,9 @@ static void set_last_pid(struct pid_namespace *pid_ns,
> int base, int pid)
>  	} while ((prev != last_write) && (pid_before(base, last_write, pid)));
>  }
> 
> +/* alloc_pidmap(): Increments last pid by one and returns it if new pid
> + * is valid.
> + */
>  static int alloc_pidmap(struct pid_namespace *pid_ns)
>  {
>  	int i, offset, max_scan, pid, last = pid_ns->last_pid;
> @@ -212,6 +218,9 @@ static int alloc_pidmap(struct pid_namespace *pid_ns)
>  	return -EAGAIN;
>  }
> 
> +/* next_pidmap() is used to find the first pid which is greater than previous
> + * last allocated pid.
> + */
>  int next_pidmap(struct pid_namespace *pid_ns, unsigned int last)
>  {
>  	int offset;
> @@ -233,6 +242,7 @@ int next_pidmap(struct pid_namespace *pid_ns,
> unsigned int last)
>  	return -1;
>  }
> 
> +/* put_pid() checks whether pid can be reallocated. */
>  void put_pid(struct pid *pid)
>  {
>  	struct pid_namespace *ns;
> @@ -249,6 +259,9 @@ void put_pid(struct pid *pid)
>  }
>  EXPORT_SYMBOL_GPL(put_pid);
> 
> +/* delayed_put_pid(): Finds the real location of struct pid in the
> + * memory and checks whether it can be reallocated with put_pid().
> + */
>  static void delayed_put_pid(struct rcu_head *rhp)
>  {
>  	struct pid *pid = container_of(rhp, struct pid, rcu);
> @@ -293,6 +306,9 @@ void free_pid(struct pid *pid)
>  	call_rcu(&pid->rcu, delayed_put_pid);
>  }
> 
> +/* alloc_pid(): Allocates local pid for each multiple namespaces
> + * in which new process created is visible.
> + */
>  struct pid *alloc_pid(struct pid_namespace *ns)
>  {
>  	struct pid *pid;
> --
> 2.9.3

Re: [Outreachy kernel] [PATCH v2 2/3] kernel: pid: Change function return value to bool.

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1947 bytes --]



On Mon, 27 Mar 2017, Varsha Rao wrote:

>
>
> On Monday, March 27, 2017 at 1:35:06 PM UTC+5:30, Julia Lawall wrote:
>
>
>       On Mon, 27 Mar 2017, Varsha Rao wrote:
>
>       >
>       >       On Mon, 27 Mar 2017, Varsha Rao wrote:
>       >
>       >       > Remove unsign cast and add bool as return type for
>       >       pid_before()
>       >       > function.
>       >
>       >       unsign -> unsigned.  But are you sure that the change is
>       >       correct?  For
>       >       example -1 < 1 but (unsigned) -1 > (unsigned) 1.
>       >
>       >
>       >    The change is incorrect. I will make both the differences as absolute
>       >    value, I guess then the change will be correct.
>
>       Not sure.  You would have to provide an argument as to why the change is
>       correct.  Absolute value also requires some computation, while casting
>       requires none.
>
>
> The usage of unsigned is to increase range and consider positive value.
> It would be better to use unsigned int only.
>
> If unsigned int is used, a - base won't exceed MAXUINT value and therefore
> there will be no negative result. As a, b and base are of type int.
> (a - base + MAXUINT) % MAXUINT < (b - base + MAXUINT) % MAXUINT

How do you conclude this?

>
> If abs is considered then the range would decrease.

What is the overall conclusion?

julia

> Thanks,
> Varsha Rao
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/d9905b6f-434e-4943-8c78-8a35bcddfe24%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
>

Re: [Outreachy kernel] [PATCH v2 2/3] kernel: pid: Change function return value to bool.

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1285 bytes --]



On Mon, 27 Mar 2017, Varsha Rao wrote:

>
> The return type can be bool but with the unsigned int casting the
> differences. Then the change would make sense.
>
> +static bool pid_before(int base, int a, int b)
>  {
>         /*
>          * This is the same as saying
> @@ -125,7 +125,7 @@ static int pid_before(int base, int a, int b)
>          * (a - base + MAXUINT) % MAXUINT < (b - base + MAXUINT) % MAXUINT
>          * and that mapping orders 'a' and 'b' with respect to 'base'.
>          */
> -       return (unsigned)(a - base) < (unsigned)(b - base);
> +       return (unsigned int)(a - base) < (unsigned int)(b - base);


This looks safer to me.

julia

>  }
>
>
> Thanks,
> Varsha Rao
>
> --
> You received this message because you are subscribed to the Google Groups "outreachy-kernel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to outreachy-kernel+unsubscribe@googlegroups.com.
> To post to this group, send email to outreachy-kernel@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/outreachy-kernel/476ea4f3-64e1-4c7e-bbcb-36b5091cd8ef%40googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
>