[RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update_nodemask()

[RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update_nodemask()

[Miao Xie]

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().

Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>

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

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
index 86ea9e3..2c2aafa 100644
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -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

Re: [RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update_nodemask()

[Paul Jackson]

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@sgi.com>

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

Re: [RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update_nodemask()

[Miao Xie]

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

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

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 
 
> 
>  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@sgi.com>
> 

Re: [RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update_nodemask()

[Paul Jackson]

Miao wrote:
> 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

Yes, you are correct.  My mistake.  Sorry.

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

Re: [RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update_nodemask()

[Miao Xie]

on 2008-5-30 9:53 Paul Jackson wrote:
> Miao wrote:
>> 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
> 
> Yes, you are correct.  My mistake.  Sorry.
> 

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

Re: [RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update_nodemask()

[Paul Jackson]

Miao wrote:
> 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 <pj@sgi.com> 1.940.382.4214

Re: [RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update_nodemask()

[Randy Dunlap]

On Thu, 29 May 2008 21:22:11 -0500 Paul Jackson wrote:

> Miao wrote:
> > 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.


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?"

Re: [RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update_nodemask()

[Paul Jackson]

Randy wrote:
> 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.

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 <pj@sgi.com> 1.940.382.4214

Re: [RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update_nodemask()

[Randy Dunlap]

On Thu, 29 May 2008 22:57:08 -0500 Paul Jackson wrote:

> Randy wrote:
> > 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.
> 
> 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,

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

> 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

There are plenty of them.

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

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)

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

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

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

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.

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

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.

> Thank-you.

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?"

Re: [RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update_nodemask()

[Paul Jackson]

Randy wrote:
> The nuance here is the difference in exported/non-exported vs. non-static/static.

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 <pj@sgi.com> 1.940.382.4214

Re: [RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update_nodemask()

[Paul Jackson]

Randy wrote:
> Does that help?  your understanding of kernel-doc or your decision?

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 <pj@sgi.com> 1.940.382.4214

Re: [RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update_nodemask()

[Alan Cox]

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

People wanting to generate things like function lists.

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

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

Re: [RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update_nodemask()

[Paul Jackson]

> And I'll join that by stubbonly NAKing such an approach. 

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 <pj@sgi.com> 1.940.382.4214

Re: [RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update_nodemask()

[Randy Dunlap]

On Fri, 30 May 2008 01:25:37 -0500 Paul Jackson wrote:

> Randy wrote:
> > Does that help?  your understanding of kernel-doc or your decision?
> 
> 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.

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.  :)

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


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

Re: [RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update_nodemask()

[Randy Dunlap]

On Fri, 30 May 2008 01:25:37 -0500 Paul Jackson wrote:

> Randy wrote:
> > Does that help?  your understanding of kernel-doc or your decision?
> 
> 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.

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.

> Enough of this discussion, from me at least.


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

Re: [RFC] [PATCH 1/2] cpusets: restructure the function update_cpumask() and update_nodemask()

[Paul Jackson]

Randy wrote:
> Well, that's some reason for your decision.

yeah ... ;).

> I agree with Alan, of course.

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 <pj@sgi.com> 1.940.382.4214