cpusets: restructure the function update_cpumask() and update_nodemask()

extract two functions from update_cpumask() and update_nodemask().They will be used later for updating tasks' cpus_allowed and mems_allowed after CPU/NODE offline/online.

NOTE: In original code of update_cpumask(), the variable heap's memory space was allocated before changing the cpuset's cpus_allowed. Now we do it in the function update_tasks_cpumask after changing the cpuset's cpus_allowed. Similar to update_nodemask().

---
 kernel/cpuset.c |  180 +++++++++++++++++++++++++++++++++---------------------
 1 files changed, 110 insertions(+), 70 deletions(-)

index 86ea9e3..2c2aafa 100644
@@ -766,6 +766,38 @@ static void cpuset_change_cpumask(struct task_struct *tsk,
 }
 
 /**
+ * update_tasks_cpumask - Scan tasks in cpuset cs, and update the cpumasks of
+ * any that need an update.
+ * @cs: the cpuset in which each task's cpus_allowed mask needs to be changed
+ *
+ * Called with cgroup_mutex held
+ *
+ * The cgroup_scan_tasks() function will scan all the tasks in a cgroup,
+ * calling callback functions for each.
+ *
+ * Return 0 if successful, -errno if not.
+ */
+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);
+	if (retval)
+		return retval;
+
+	scan.cg = cs->css.cgroup;
+	scan.test_task = cpuset_test_cpumask;
+	scan.process_task = cpuset_change_cpumask;
+	scan.heap = &heap;
+	retval = cgroup_scan_tasks(&scan);
+
+	heap_free(&heap);
+	return retval;
+}
+
+/**
  * update_cpumask - update the cpus_allowed mask of a cpuset and all tasks in it
  * @cs: the cpuset to consider
  * @buf: buffer of cpu numbers written to this cpuset

@@ -773,8 +805,6 @@ static void cpuset_change_cpumask(struct task_struct *tsk,
 static int update_cpumask(struct cpuset *cs, char *buf)
 {
 	struct cpuset trialcs;
-	struct cgroup_scanner scan;
-	struct ptr_heap heap;
 	int retval;
 	int is_load_balanced;
 

@@ -807,10 +837,6 @@ static int update_cpumask(struct cpuset *cs, char *buf)
 	if (cpus_equal(cs->cpus_allowed, trialcs.cpus_allowed))
 		return 0;
 
-	retval = heap_init(&heap, PAGE_SIZE, GFP_KERNEL, &started_after);
-	if (retval)
-		return retval;
-
 	is_load_balanced = is_sched_load_balance(&trialcs);
 
 	mutex_lock(&callback_mutex);

@@ -821,12 +847,9 @@ static int update_cpumask(struct cpuset *cs, char *buf)
 	 * Scan tasks in the cpuset, and update the cpumasks of any
 	 * that need an update.
 	 */
-	scan.cg = cs->css.cgroup;
-	scan.test_task = cpuset_test_cpumask;
-	scan.process_task = cpuset_change_cpumask;
-	scan.heap = &heap;
-	cgroup_scan_tasks(&scan);
-	heap_free(&heap);
+	retval = update_tasks_cpumask(cs);
+	if (retval < 0)
+		return retval;
 
 	if (is_load_balanced)
 		rebuild_sched_domains();

@@ -882,72 +905,26 @@ static void cpuset_migrate_mm(struct mm_struct *mm, const nodemask_t *from,
 	mutex_unlock(&callback_mutex);
 }
 
-/*
- * Handle user request to change the 'mems' memory placement
- * of a cpuset.  Needs to validate the request, update the
- * cpusets mems_allowed and mems_generation, and for each
- * task in the cpuset, rebind any vma mempolicies and if
- * the cpuset is marked 'memory_migrate', migrate the tasks
- * pages to the new memory.
- *
- * Call with cgroup_mutex held.  May take callback_mutex during call.
- * Will take tasklist_lock, scan tasklist for tasks in cpuset cs,
- * lock each such tasks mm->mmap_sem, scan its vma's and rebind
- * their mempolicies to the cpusets new mems_allowed.
- */
-
 static void *cpuset_being_rebound;
 
-static int update_nodemask(struct cpuset *cs, char *buf)
+/**
+ * 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)
 {
-	struct cpuset trialcs;
-	nodemask_t oldmem;
 	struct task_struct *p;
 	struct mm_struct **mmarray;
 	int i, n, ntasks;
 	int migrate;
 	int fudge;
-	int retval;
 	struct cgroup_iter it;
-
-	/*
-	 * top_cpuset.mems_allowed tracks node_stats[N_HIGH_MEMORY];
-	 * it's read-only
-	 */
-	if (cs == &top_cpuset)
-		return -EACCES;
-
-	trialcs = *cs;
-
-	/*
-	 * An empty mems_allowed is ok iff there are no tasks in the cpuset.
-	 * Since nodelist_parse() fails on an empty mask, we special case
-	 * that parsing.  The validate_change() call ensures that cpusets
-	 * with tasks have memory.
-	 */
-	buf = strstrip(buf);
-	if (!*buf) {
-		nodes_clear(trialcs.mems_allowed);
-	} else {
-		retval = nodelist_parse(buf, trialcs.mems_allowed);
-		if (retval < 0)
-			goto done;
-	}
-	nodes_and(trialcs.mems_allowed, trialcs.mems_allowed,
-						node_states[N_HIGH_MEMORY]);
-	oldmem = cs->mems_allowed;
-	if (nodes_equal(oldmem, trialcs.mems_allowed)) {
-		retval = 0;		/* Too easy - nothing to do */
-		goto done;
-	}
-	retval = validate_change(cs, &trialcs);
-	if (retval < 0)
-		goto done;
-
-	mutex_lock(&callback_mutex);
-	cs->mems_allowed = trialcs.mems_allowed;
-	cs->mems_generation = cpuset_mems_generation++;
-	mutex_unlock(&callback_mutex);
+	int retval = 0;
 
 	cpuset_being_rebound = cs;		/* causes mpol_dup() rebind */
 

@@ -1014,7 +991,7 @@ static int update_nodemask(struct cpuset *cs, char *buf)
 
 		mpol_rebind_mm(mm, &cs->mems_allowed);
 		if (migrate)
-			cpuset_migrate_mm(mm, &oldmem, &cs->mems_allowed);
+			cpuset_migrate_mm(mm, oldmem, &cs->mems_allowed);
 		mmput(mm);
 	}
 

@@ -1026,6 +1003,69 @@ done:
 	return retval;
 }
 
+/*
+ * Handle user request to change the 'mems' memory placement
+ * of a cpuset.  Needs to validate the request, update the
+ * cpusets mems_allowed and mems_generation, and for each
+ * task in the cpuset, rebind any vma mempolicies and if
+ * the cpuset is marked 'memory_migrate', migrate the tasks
+ * pages to the new memory.
+ *
+ * Call with cgroup_mutex held.  May take callback_mutex during call.
+ * Will take tasklist_lock, scan tasklist for tasks in cpuset cs,
+ * lock each such tasks mm->mmap_sem, scan its vma's and rebind
+ * their mempolicies to the cpusets new mems_allowed.
+ */
+static int update_nodemask(struct cpuset *cs, char *buf)
+{
+	struct cpuset trialcs;
+	nodemask_t oldmem;
+	int retval;
+
+	/*
+	 * top_cpuset.mems_allowed tracks node_stats[N_HIGH_MEMORY];
+	 * it's read-only
+	 */
+	if (cs == &top_cpuset)
+		return -EACCES;
+
+	trialcs = *cs;
+
+	/*
+	 * An empty mems_allowed is ok iff there are no tasks in the cpuset.
+	 * Since nodelist_parse() fails on an empty mask, we special case
+	 * that parsing.  The validate_change() call ensures that cpusets
+	 * with tasks have memory.
+	 */
+	buf = strstrip(buf);
+	if (!*buf) {
+		nodes_clear(trialcs.mems_allowed);
+	} else {
+		retval = nodelist_parse(buf, trialcs.mems_allowed);
+		if (retval < 0)
+			goto done;
+	}
+	nodes_and(trialcs.mems_allowed, trialcs.mems_allowed,
+						node_states[N_HIGH_MEMORY]);
+	oldmem = cs->mems_allowed;
+	if (nodes_equal(oldmem, trialcs.mems_allowed)) {
+		retval = 0;		/* Too easy - nothing to do */
+		goto done;
+	}
+	retval = validate_change(cs, &trialcs);
+	if (retval < 0)
+		goto done;
+
+	mutex_lock(&callback_mutex);
+	cs->mems_allowed = trialcs.mems_allowed;
+	cs->mems_generation = cpuset_mems_generation++;
+	mutex_unlock(&callback_mutex);
+
+	retval = update_tasks_nodemask(cs, &oldmem);
+done:
+	return retval;
+}
+
 int current_cpuset_is_being_rebound(void)
 {
 	return task_cs(current) == cpuset_being_rebound;

1.5.4.rc3

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

@@ -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:

                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.940.382.4214

on 2008-5-29 16:16 Paul Jackson wrote:
[snip]

I think that it is unnecessary to change cpuset_cpus_allowed_locked()'s comment because it isn't a static function, it is a extern function and it is called by move_task_off_dead_cpu() in kernel/sched.c

Yes, you are correct. My mistake. Sorry.

                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.940.382.4214

I check kernel/cpuset.c and find many static functions with "/**" comment. So I want to remove the special commenting convention for them.

Right you are. Offhand, in kernel/cpuset.c, I see:

static int cpuset_test_cpumask(struct task_struct *tsk,
static void cpuset_change_cpumask(struct task_struct *tsk,
static int update_cpumask(struct cpuset *cs, char *buf)
static void cpuset_do_move_task(struct task_struct *tsk,
static void move_member_tasks_to_cpuset(struct cpuset *from, struct cpuset *to)

all having "/**" header comments. I would be glad to Ack a patch from you to fix such comments. Thank-you.

                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.940.382.4214

Uh.. We strongly want non-static functions to be documented via kernel-doc. For static functions, it's up to the maintainer/developer whether to do that or not. But if the functions already have kernel-doc, there's no strong reason to remove it. And these look good currently, so I see no good reason to change them.

---
~Randy
"He closes his eyes and drops the goggles. You can't get hurt by looking at a bitmap. Or can you?"

Ok - thank-you for the explanation.

I had this vague recollection that kernel-doc was just for non-static functions, so when I saw Miao put kernel-doc comments on a static routine, I looked around to see if that was normal.

I didn't see mention of static functions in kernel-doc-nano-HOWTO.txt, and in a brief survey, didn't happen to see any static functions with kernel-doc comments. However my survey was so brief I even missed five such functions in the file I maintain, kernel/cpuset.c. So from that I came to the (apparently flawed) conclusion that it didn't make sense to kernel-doc static functions, and so advised Miao.

I don't quite see what purpose kernel-doc would have for static functions, and am still concerned that such would (mildly) clutter the presentation of the externally visible cpuset functions in which those who just use cpusets would be interested.

It remains, of course, important to have good documentation of functions, static or not, but I would expect developers working inside the kernel/cpuset.c file to look for documentation of functions static to that file within the code and comments of that file, not in kernel-doc.

However I am just the cpuset maintainer, not a kernel-doc expert.

I remain inclined toward removing the kernel-doc markings on static functions in kernel/cpuset.c, but if you wish, Randy, to make a renewed appeal for me not to do so, I will happily honor that appeal.

Life is good either way when we are discussing how to annotate documentation, rather than lamenting its absence.

If you, Randy, felt inclined to explain the value of kernel-doc comments on file static functions in kernel-doc-nano-HOWTO.txt, that might be valuable for the next person who travels this path.

Thank-you.

                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.940.382.4214

There's no restriction to static or non-static.

There are plenty of them.

Well, they won't clutter the generated kernel-doc docbook output unless you/we/someone requests that both non-static and static functions be listed in it. (more on that below)

kernel-doc is in comments. Just specially formatted ones.

a. I looked at those 5 functions. They should be documented, whether it's done with kernel-doc or some other way. I don't see why there would be a problem with using kernel-doc instead of something else.

b. Yes, any documentation along these lines is better than none. We agree.

It might need a little tweaking. Basically what is already there says:

"""
!E<filename> is replaced by the documentation, in <filename>, for functions that are exported using EXPORT_SYMBOL: the function list is collected from files listed in Documentation/DocBook/Makefile.

!I<filename> is replaced by the documentation for functions that are _not_ exported using EXPORT_SYMBOL.
"""

so if a docbook (like one generated from Documentation/DocBook/kernel-api.tmpl) uses both of these:
!Ekernel/cpuset.c
!Ikernel/cpuset.c

then that docbook will contain both exported functions and non-exported ones. To produce a docbook with only exported symbols, use only !Ekernel/cpuset.c

The nuance here is the difference in exported/non-exported vs. non-static/static.

Same to you, sir.

Does that help? your understanding of kernel-doc or your decision?

---
~Randy
"He closes his eyes and drops the goggles. You can't get hurt by looking at a bitmap. Or can you?"

Ok ... hmmm ... how to say this...

The documentation in Documentation/kernel-doc-nano-HOWTO.txt of kernel-doc is mostly focused on the details of the apparatus that converts the "/**" comments into various documenation formats, and has only somewhat buried and non-inviting documentation for kernel hackers wanting the "Kernel-Doc for Dummies" summary of how and when and where to create such "/**" comments.

For example, the distinction you note between:
!E<filename> external (EXPORT_SYMBOL), and
!I<filename> internal (not exported)
is buried in the "How to make new SGML template files" section, which I would hope that I never had to read.

However ... in accordance with the usual rule that he who complains gets to fix it, I find this kernel-doc documentation to be PERFECT! ;).

                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.940.382.4214

Well, I get the difference between E (exported) and I (non-exported) now. And I see that one could prepare documents using SGML templates that contained one, or the other of these, for any kernel source files of interest.

I'm stuck on the next step of this decision.

Usually, when I am preparing documens, I know what document I am preparing and have an idea who is in its audience.

I have never seen or heard of a document using the "/**" kernel-doc entries of kernel/cpuset.c, and I have no idea who actually has (in the past or present, not just hypothetically) read such or why.

So I'm kinda shootin in the dark here.

So, mostly just to be consistent with my previous call, because I enjoy being a stubborn retard, I continue to prefer that file static routines in kernel/cpuset.c not have "/**" kernel-doc markers on their comments, and I would still welcome a patch from Miao removing the ones already there.

Enough of this discussion, from me at least.

                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.940.382.4214

People wanting to generate things like function lists.

And I'll join that by stubbonly NAKing such an approach. We want consistent clear formats for documentation extraction. Simply getting people to use the kerneldoc format materially improves the documentation quality, so please don't set bad examples ;)

Alan

That's good - thanks, Alan.

I'll ack Alan's nak of my ack ;).

                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.940.382.4214

You are certainly welcome to add kernel/cpuset.c to kernel-api.tmpl or any other appropriate docbook file. I have been known to do things like that as well. :)

---
~Randy
"He closes his eyes and drops the goggles. You can't get hurt by looking at a bitmap. Or can you?"

Well, that's some reason for your decision. At least you aren't just being arbitrary. I agree with Alan, of course. I don't think that you have stated a Good reason for removing the kernel-doc on the static functions, but it is your call AFAICT.

---
~Randy
"He closes his eyes and drops the goggles. You can't get hurt by looking at a bitmap. Or can you?"

yeah ... ;).

So be it ... more kernel-doc comments are good,
static or not.

                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <[email protected]> 1.940.382.4214