linux-kernel - Re: [RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20080529031656.cdc38001.pj@sgi.com>
Date:	Thu, 29 May 2008 03:16:56 -0500
From:	Paul Jackson <pj@....com>
To:	miaox@...fujitsu.com
Cc:	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
	menage@...gle.com
Subject: Re: [RFC] [PATCH 1/2] cpusets: restructure the function
 update_cpumask() and update_nodemask()

Nice work.

As usual, I have a few minor items to note.

 1) No need to initialize 'retval = 0' since retval is set
    unconditionally in the next line of update_tasks_cpumask():

	+static int update_tasks_cpumask(struct cpuset *cs)
	+{
	+	struct cgroup_scanner scan;
	+	struct ptr_heap heap;
	+	int retval = 0;
	+
	+	retval = heap_init(&heap, PAGE_SIZE, GFP_KERNEL, &started_after);

 2) Same thing in update_tasks_nodemask - no need to initialize
    retval = 0.
    
    These initializations use an extra instruction or two, so we
    avoid them, if it is really clear from the code that the
    variable is set before used anyway.
    

 3) The special style for documenting functions, starting with the
    "/**" comment, which is documented in:
        Documentation/kernel-doc-nano-HOWTO.txt
    results in adding that functions documentation to various man pages
    and documents that are generated from these comment blocks.
    Typically, we do not document file static routines this way,
    because such routines cannot be called from outside that file,
    so are of little use to others.  Best not to clutter the more
    widely distributed documentation with details of functions that
    others can't call anyway.

    So I would suggest -removing- the special commenting convention
    for the routines update_tasks_cpumask() and update_tasks_nodemask().
    
    For example, in the update_tasks_nodemask() case, this means
    changing from:

    > /**
    >  * update_tasks_nodemask - Scan tasks in cpuset cs, and update the nodemasks of
    >  * any that need an update.
    >  * @cs: the cpuset in which each task's mems_allowed mask needs to be changed
    >  * @oldmem: old mems_allowed of cpuset cs
    >  *
    >  * Called with cgroup_mutex held
    >  * Return 0 if successful, -errno if not.
    >  */
    > static int update_tasks_nodemask(struct cpuset *cs, const nodemask_t *oldmem)

    to:


    > /*
    >  * update_tasks_nodemask - Scan tasks in cpuset cs, and update the nodemasks of
    >  * any that need an update.
    >  *  cs: the cpuset in which each task's mems_allowed mask needs to be changed
    >  *  oldmem: old mems_allowed of cpuset cs
    >  *
    >  * Called with cgroup_mutex held
    >  * Return 0 if successful, -errno if not.
    >  */
    > static int update_tasks_nodemask(struct cpuset *cs, const nodemask_t *oldmem)


    Similarly for update_tasks_cpumask().

 4) Could you do me a little favor, and include the two minor fixes in
    the following patch in your patch?  These two fixes aren't worth
    making their own separate submission for.  I noticed them when I
    was running the scripts/kernel-doc tool to check the comments for
    my comment (3) above.  You can either just add the minor fixes to
    your patch 1 of 2, or you can make the following a third patch in
    your patch set, under your "Signed-off-by" line.  It does not matter
    at all to me which way you do it.  Take the easy way, which is
    probably just making these three minor changes as part of your
    first patch, just as if they were your code all the time.  Thanks!


====================== Begin Patch ======================
--- 2.6.26-rc2-mm1-pj_efi_patches.orig/kernel/cpuset.c	2008-05-29 00:20:35.000000000 -0700
+++ 2.6.26-rc2-mm1-pj_efi_patches/kernel/cpuset.c	2008-05-29 00:53:42.478128805 -0700
@@ -1938,7 +1938,6 @@ void __init cpuset_init_smp(void)
 }
 
 /**
-
  * cpuset_cpus_allowed - return cpus_allowed mask from a tasks cpuset.
  * @tsk: pointer to task_struct from which to obtain cpuset->cpus_allowed.
  * @pmask: pointer to cpumask_t variable to receive cpus_allowed set.
@@ -1956,10 +1955,10 @@ void cpuset_cpus_allowed(struct task_str
 	mutex_unlock(&callback_mutex);
 }
 
-/**
+/*
  * cpuset_cpus_allowed_locked - return cpus_allowed mask from a tasks cpuset.
  * Must be called with callback_mutex held.
- **/
+ */
 void cpuset_cpus_allowed_locked(struct task_struct *tsk, cpumask_t *pmask)
 {
 	task_lock(tsk);
======================= End Patch =======================


 5) You wrote:
	This patch fixes this bug expect for root cpuset.
    Then you analyze the root cpuset problem that remains.  I will try
    to think more about that perhaps tomorrow; that won't impede progress
    on this current patch set.



These patches look very good to me.  Please add my Acked-by line
in your next and I expect final version:

Acked-by: Paul Jackson <pj@....com>

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@....com> 1.940.382.4214
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/