[PATCH] Cpuset: remove unnecessary NULL check

[PATCH] Cpuset: remove unnecessary NULL check

[Paul Jackson]

From: Paul Jackson <pj@sgi.com>

Remove a no longer needed test for NULL cpuset pointer, with
a little comment explaining why the test isn't needed.

Signed-off-by: Paul Jackson <pj@sgi.com>

---

 kernel/cpuset.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

--- 2.6.16-rc6-mm2.orig/kernel/cpuset.c	2006-03-18 22:15:33.912350380 -0800
+++ 2.6.16-rc6-mm2/kernel/cpuset.c	2006-03-18 22:15:43.681994273 -0800
@@ -2375,12 +2375,12 @@ void __cpuset_memory_pressure_bump(void)
  *  - No need to task_lock(tsk) on this tsk->cpuset reference, as it
  *    doesn't really matter if tsk->cpuset changes after we read it,
  *    and we take manage_mutex, keeping attach_task() from changing it
- *    anyway.
+ *    anyway.  No need to check that tsk->cpuset != NULL, thanks to the
+ *    cpuset_exit() Hack.
  */
 
 static int proc_cpuset_show(struct seq_file *m, void *v)
 {
-	struct cpuset *cs;
 	struct pid *pid;
 	struct task_struct *tsk;
 	char *buf;
@@ -2399,11 +2399,8 @@ static int proc_cpuset_show(struct seq_f
 
 	retval = -EINVAL;
 	mutex_lock(&manage_mutex);
-	cs = tsk->cpuset;
-	if (!cs)
-		goto out_unlock;
 
-	retval = cpuset_path(cs, buf, PAGE_SIZE);
+	retval = cpuset_path(tsk->cpuset, buf, PAGE_SIZE);
 	if (retval < 0)
 		goto out_unlock;
 	seq_puts(m, buf);

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373

Re: [PATCH] Cpuset: remove unnecessary NULL check

[Eric W. Biederman]

Paul Jackson <pj@sgi.com> writes:

> From: Paul Jackson <pj@sgi.com>
>
> Remove a no longer needed test for NULL cpuset pointer, with
> a little comment explaining why the test isn't needed.

Could we make that comment a little more explicit.  Say:

No need to check that tsk->cpuset != NULL, since cpuset_exit() sets
it to top_cpuset instead.

Comments that refer to a nebulous hack in some other function
a little hard to understand when new, and get really confusing
when the other function changes and it isn't clear what aspect
of that other function the comment depended on and if that property
still exists.

>   *  - No need to task_lock(tsk) on this tsk->cpuset reference, as it
>   *    doesn't really matter if tsk->cpuset changes after we read it,
>   *    and we take manage_mutex, keeping attach_task() from changing it
> - *    anyway.
> + *    anyway.  No need to check that tsk->cpuset != NULL, thanks to the
> + *    cpuset_exit() Hack.
>   */


Eric

Re: [PATCH] Cpuset: remove unnecessary NULL check

[Paul Jackson]

Eric wrote:
> Comments that refer to a nebulous hack ...

Well, in this case, it wasn't so nebulous, to me anyway.

The "Hack" referred to has a big fat comment beginning:

 *
 * Hack:
 *
 *    Set the exiting tasks cpuset to the root cpuset (top_cpuset).
 *
 *    Don't leave a task unable to allocate memory, as ...

and is labeled at the scene of the Hack with:

        tsk->cpuset = &top_cpuset;      /* Hack - see comment above */

So "Hack" was intended as a proper noun, not a nebulous generic term.

But, sure, hard to argue with anyone wanting improved comments.

And while I'm at it, I will change the name of this Hack to something
less generic ... say "the_top_cpuset_hack".

Patch coming soon.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: [PATCH] Cpuset: remove unnecessary NULL check

[Eric W. Biederman]

Paul Jackson <pj@sgi.com> writes:

> Eric wrote:
>> Comments that refer to a nebulous hack ...
>
> Well, in this case, it wasn't so nebulous, to me anyway.

Right.  The core problem was there was not enough context to see
what the dependency was from the comment.  

>
> The "Hack" referred to has a big fat comment beginning:
>
>  *
>  * Hack:
>  *
>  *    Set the exiting tasks cpuset to the root cpuset (top_cpuset).
>  *
>  *    Don't leave a task unable to allocate memory, as ...
>
> and is labeled at the scene of the Hack with:
>
>         tsk->cpuset = &top_cpuset;      /* Hack - see comment above */
>
> So "Hack" was intended as a proper noun, not a nebulous generic term.
>
> But, sure, hard to argue with anyone wanting improved comments.
>
> And while I'm at it, I will change the name of this Hack to something
> less generic ... say "the_top_cpuset_hack".
>
> Patch coming soon.

Thanks.

Eric

Re: [PATCH] Cpuset: remove unnecessary NULL check

[Eric W. Biederman]

Paul Jackson <pj@sgi.com> writes:

> Eric wrote:
>> Comments that refer to a nebulous hack ...
>
> Well, in this case, it wasn't so nebulous, to me anyway.

Right.  The core problem was there was not enough context to see
what the dependency was from the comment.  Having to read
another comment to understand the comment you are reading
usually breaks after a while.

Now that we have git the problem is less severe because we
can look in the history and see what the comment meant when
it was written but that is still a hassle.

> Patch coming soon.

Thanks.

Eric

[PATCH] Cpuset: remove unnecessary NULL check

[Paul Jackson]

From: Paul Jackson <pj@sgi.com>

Remove a no longer needed test for NULL cpuset pointer, with
a little comment explaining why the test isn't needed.

Signed-off-by: Paul Jackson <pj@sgi.com>

---

 kernel/cpuset.c |    9 +++------
 1 files changed, 3 insertions(+), 6 deletions(-)

--- 2.6.16-rc5-mm2-pre1.orig/kernel/cpuset.c	2006-03-01 20:25:52.390667963 -0800
+++ 2.6.16-rc5-mm2-pre1/kernel/cpuset.c	2006-03-01 20:30:07.730593536 -0800
@@ -2374,12 +2374,12 @@ void __cpuset_memory_pressure_bump(void)
  *  - No need to task_lock(tsk) on this tsk->cpuset reference, as it
  *    doesn't really matter if tsk->cpuset changes after we read it,
  *    and we take manage_mutex, keeping attach_task() from changing it
- *    anyway.
+ *    anyway.  No need to check that tsk->cpuset != NULL, thanks to the
+ *    cpuset_exit() Hack.
  */
 
 int cpuset_proc_show_path(struct seq_file *m, void *v)
 {
-	struct cpuset *cs;
 	struct task_ref *tref;
 	struct task_struct *tsk;
 	char *buf;
@@ -2398,11 +2398,8 @@ int cpuset_proc_show_path(struct seq_fil
 
 	retval = -EINVAL;
 	mutex_lock(&manage_mutex);
-	cs = tsk->cpuset;
-	if (!cs)
-		goto out_unlock;
 
-	retval = cpuset_path(cs, buf, PAGE_SIZE);
+	retval = cpuset_path(tsk->cpuset, buf, PAGE_SIZE);
 	if (retval < 0)
 		goto out_unlock;
 	seq_puts(m, buf);

-- 
                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <pj@sgi.com> 1.650.933.1373