Hi,
It was recommended that I use a completion in a driver I am working on.
While figuring out how to use one, I noticed that there was no kernel
doc block comments. I am trying to add them. I would rather not have to
respin the patch for corrections.
--- include/linux/completion.h.orig 2008-08-13 00:56:52.000000000 -0700
+++ include/linux/completion.h 2008-08-19 17:47:49.000000000 -0700
@@ -10,6 +10,14 @@
#include <linux/wait.h>
+/**
+ * struct completion - structure used to maintain state for a "completion"
+ *
+ * This is the opaque structure used to maintain the state for a
"completion".
+ * See also: complete(), wait_for_completion() (and friends _timeout,
+ * _interruptible, _interruptible_timeout, and _killable),
init_completion(),
+ * and macros DECLARE_COMPLETION() and INIT_COMPLETION().
+ */
struct completion {
unsigned int done;
wait_queue_head_t wait;
@@ -21,6 +29,14 @@
#define COMPLETION_INITIALIZER_ONSTACK(work) \
({ init_completion(&work); work; })
+/**
+ * DECLARE_COMPLETION: - declare and initialize a completion structure
+ * @work: identifier for the completion structure
+ *
+ * This macro declares and initializes a completion structure.
Generally used
+ * for static declarations. You should use the _ONSTACK variant for
automatic
+ * variables.
+ */
#define DECLARE_COMPLETION(work) \
struct completion work = COMPLETION_INITIALIZER(work)
@@ -29,6 +45,13 @@
* completions - so we use the _ONSTACK() variant for those that
* are on the kernel stack:
*/
+/**
+ * DECLARE_COMPLETION_ONSTACK: - declare and initialize a completion
structure
+ * @work: identifier for the completion structure
+ *
+ * This macro declares and initializes a completion structure on the kernel
+ * stack.
+ */
#ifdef CONFIG_LOCKDEP
# define DECLARE_COMPLETION_ONSTACK(work) \
struct completion work = COMPLETION_INITIALIZER_ONSTACK(work)
@@ -36,6 +59,13 @@
# define DECLARE_COMPLETION_ONSTACK(work) DECLARE_COMPLETION(work)
#endif
+/**
+ * init_completion: - Initialize a dynamically allocated completion
+ * @x: completion structure that is to be initialized
+ *
+ * This inline function will initialize a dynamically created completion
+ * structure.
+ */
static inline void init_completion(struct completion *x)
{
x->done = 0;
@@ -53,6 +83,13 @@
extern void complete(struct completion *);
extern void complete_all(struct completion *);
+/**
+ * INIT_COMPLETION: - reinitialize a completion structure
+ * @x: completion structure to be reinitialized
+ *
+ * This macro should be used to reinitialize a completion structure so
it can
+ * be reused. This is especially important after complete_all() is used.
+ */
#define INIT_COMPLETION(x) ((x).done = 0)
#endif
--- kernel/sched.c.orig 2008-08-13 02:22:42.000000000 -0700
+++ kernel/sched.c 2008-08-19 17:12:36.000000000 -0700
@@ -4363,6 +4363,15 @@
}
EXPORT_SYMBOL_GPL(__wake_up_sync); /* For internal use only */
+/**
+ * complete: - signals a single thread waiting on this completion
+ * @x: holds the state of this particular completion
+ *
+ * This will wake up a single thread waiting on this completion.
Threads will be
+ * awakened in the same order in which they were queued.
+ *
+ * See also complete_all().
+ */
void complete(struct completion *x)
{
unsigned long flags;
@@ -4374,6 +4383,12 @@
}
EXPORT_SYMBOL(complete);
+/**
+ * complete_all: - signals all threads waiting on this completion
+ * @x: holds the state of this particular completion
+ *
+ * This will wake up all threads waiting on this particular completion
event.
+ */
void complete_all(struct completion *x)
{
unsigned long flags;
@@ -4425,12 +4440,30 @@
return timeout;
}
+/**
+ * wait_for_completion: - waits for completion of a task
+ * @x: holds the state of this particular completion
+ *
+ * This waits to be signaled for completion of a specific task. It is NOT
+ * interruptible and there is no timeout. If it is necessary for the
thread to
+ * wait it will be added to the tail of the wait queue.
+ */
void __sched wait_for_completion(struct completion *x)
{
wait_for_common(x, MAX_SCHEDULE_TIMEOUT, TASK_UNINTERRUPTIBLE);
}
EXPORT_SYMBOL(wait_for_completion);
+/**
+ * wait_for_completion_timeout: - waits for completion of a task
(w/timeout)
+ * @x: holds the state of this particular completion
+ * @timeout: timeout value in jiffies
+ *
+ * This waits for either a completion of a specific task to be signaled
or for a
+ * specified timeout to expire. The timeout is in jiffies. It is not
+ * interruptible. If it is necessary for the thread to wait it will be
+ * added to the tail of the wait queue.
+ */
unsigned long __sched
wait_for_completion_timeout(struct completion *x, unsigned long timeout)
{
@@ -4438,6 +4471,14 @@
}
EXPORT_SYMBOL(wait_for_completion_timeout);
+/**
+ * wait_for_completion_interruptible: - waits for completion of a task
(w/intr)
+ * @x: holds the state of this particular completion
+ *
+ * This waits for completion of a specific task to be signaled. It is
+ * interruptible. If it is necessary for the thread to wait it will be
+ * added to the tail of the wait queue.
+ */
int __sched wait_for_completion_interruptible(struct completion *x)
{
long t = wait_for_common(x, MAX_SCHEDULE_TIMEOUT, TASK_INTERRUPTIBLE);
@@ -4447,6 +4488,16 @@
}
EXPORT_SYMBOL(wait_for_completion_interruptible);
+/**
+ * wait_for_completion_interruptible_timeout: - waits for completion
(w/(to,intr))
+ * @x: holds the state of this particular completion
+ * @timeout: timeout value in jiffies
+ *
+ * This waits for either a completion of a specific task to be signaled
or for a
+ * specified timeout to expire. It is interruptible. The timeout is in
jiffies.
+ * If it is necessary for the thread to wait it will be added to the
tail of
+ * the wait queue.
+ */
unsigned long __sched
wait_for_completion_interruptible_timeout(struct completion *x,
unsigned long timeout)
@@ -4455,6 +4506,14 @@
}
EXPORT_SYMBOL(wait_for_completion_interruptible_timeout);
+/**
+ * wait_for_completion_killable: - waits for completion of a task
(killable)
+ * @x: holds the state of this particular completion
+ *
+ * This waits to be signaled for completion of a specific task. It can be
+ * interrupted by a kill signal. If it is necessary for the thread to
+ * wait it will be added to the tail of the wait queue.
+ */
int __sched wait_for_completion_killable(struct completion *x)
{
long t = wait_for_common(x, MAX_SCHEDULE_TIMEOUT, TASK_KILLABLE);
On Tue, Aug 19, 2008 at 06:30:11PM -0700, Kevin Diggs wrote:
> Hi,
>
> It was recommended that I use a completion in a driver I am working on.
> While figuring out how to use one, I noticed that there was no kernel
> doc block comments. I am trying to add them. I would rather not have to
> respin the patch for corrections.
Rather than documenting exactly how the queuing and wakeup occurs on
all functions, you should document it once. i.e. that completions
currently use FIFO queuing. It is probably best to do this at the
definition of the struct completion.
The reason is that if the implementation changes (e.g. to support
priorities and inheritence) the comments are then incorrect and
then there's lots of comments to remove^Wchange.
Cheers,
Dave.
--
Dave Chinner
[email protected]
* Kevin Diggs <[email protected]> wrote:
> Hi,
>
> It was recommended that I use a completion in a driver I am
> working on. While figuring out how to use one, I noticed that there
> was no kernel doc block comments. I am trying to add them. I would
> rather not have to respin the patch for corrections.
the text looks good to me - but many of the lines are line-wrapped
(longer than 80 chars) - which should be avoided for free-flowing text.
Ingo
> * Kevin Diggs <[email protected]> wrote:
>> It was recommended that I use a completion in a driver I am
>> working on. While figuring out how to use one,
Kevin, as a side note: LDD3 chapter 5 pages 114 ff has some guidance.
http://lwn.net/Kernel/LDD3/
I'm only pointing to it for the unlikely case that you didn't come
across LDD3 yet. The dead tree version is affordable too, and more
comfortable to read than the online version.
--
Stefan Richter
-=====-==--- =--- =-=--
http://arcgraph.de/sr/
Stefan Richter wrote:
>>* Kevin Diggs <[email protected]> wrote:
>>
>>> It was recommended that I use a completion in a driver I am
>>>working on. While figuring out how to use one,
>
>
> Kevin, as a side note: LDD3 chapter 5 pages 114 ff has some guidance.
> http://lwn.net/Kernel/LDD3/
> I'm only pointing to it for the unlikely case that you didn't come
> across LDD3 yet. The dead tree version is affordable too, and more
> comfortable to read than the online version.
Oh! I have that! That is how I figured out that complete() is used to
signal that the wait thingies don't have to wait anymore.
Here is the latest (maybe line wrap is set to 99 so it won't be goofed
anymore?):
--- include/linux/completion.h.orig 2008-08-13 00:56:52.000000000 -0700
+++ include/linux/completion.h 2008-08-20 01:47:21.000000000 -0700
@@ -10,6 +10,18 @@
#include <linux/wait.h>
+/**
+ * struct completion - structure used to maintain state for a "completion"
+ *
+ * This is the opaque structure used to maintain the state for a "completion".
+ * Completions currently use a FIFO to queue threads that have to wait for
+ * the "completion" event.
+ *
+ * See also: complete(), wait_for_completion() (and friends _timeout,
+ * _interruptible, _interruptible_timeout, and _killable), init_completion(),
+ * and macros DECLARE_COMPLETION(), DECLARE_COMPLETION_ONSTACK(), and
+ * INIT_COMPLETION().
+ */
struct completion {
unsigned int done;
wait_queue_head_t wait;
@@ -21,6 +33,14 @@
#define COMPLETION_INITIALIZER_ONSTACK(work) \
({ init_completion(&work); work; })
+/**
+ * DECLARE_COMPLETION: - declare and initialize a completion structure
+ * @work: identifier for the completion structure
+ *
+ * This macro declares and initializes a completion structure. Generally used
+ * for static declarations. You should use the _ONSTACK variant for automatic
+ * variables.
+ */
#define DECLARE_COMPLETION(work) \
struct completion work = COMPLETION_INITIALIZER(work)
@@ -29,6 +49,13 @@
* completions - so we use the _ONSTACK() variant for those that
* are on the kernel stack:
*/
+/**
+ * DECLARE_COMPLETION_ONSTACK: - declare and initialize a completion structure
+ * @work: identifier for the completion structure
+ *
+ * This macro declares and initializes a completion structure on the kernel
+ * stack.
+ */
#ifdef CONFIG_LOCKDEP
# define DECLARE_COMPLETION_ONSTACK(work) \
struct completion work = COMPLETION_INITIALIZER_ONSTACK(work)
@@ -36,6 +63,13 @@
# define DECLARE_COMPLETION_ONSTACK(work) DECLARE_COMPLETION(work)
#endif
+/**
+ * init_completion: - Initialize a dynamically allocated completion
+ * @x: completion structure that is to be initialized
+ *
+ * This inline function will initialize a dynamically created completion
+ * structure.
+ */
static inline void init_completion(struct completion *x)
{
x->done = 0;
@@ -53,6 +87,13 @@
extern void complete(struct completion *);
extern void complete_all(struct completion *);
+/**
+ * INIT_COMPLETION: - reinitialize a completion structure
+ * @x: completion structure to be reinitialized
+ *
+ * This macro should be used to reinitialize a completion structure so it can
+ * be reused. This is especially important after complete_all() is used.
+ */
#define INIT_COMPLETION(x) ((x).done = 0)
#endif
--- kernel/sched.c.orig 2008-08-13 02:22:42.000000000 -0700
+++ kernel/sched.c 2008-08-20 12:36:01.000000000 -0700
@@ -4363,6 +4363,15 @@
}
EXPORT_SYMBOL_GPL(__wake_up_sync); /* For internal use only */
+/**
+ * complete: - signals a single thread waiting on this completion
+ * @x: holds the state of this particular completion
+ *
+ * This will wake up a single thread waiting on this completion. Threads will be
+ * awakened in the same order in which they were queued.
+ *
+ * See also complete_all(), wait_for_completion() and related routines.
+ */
void complete(struct completion *x)
{
unsigned long flags;
@@ -4374,6 +4383,12 @@
}
EXPORT_SYMBOL(complete);
+/**
+ * complete_all: - signals all threads waiting on this completion
+ * @x: holds the state of this particular completion
+ *
+ * This will wake up all threads waiting on this particular completion event.
+ */
void complete_all(struct completion *x)
{
unsigned long flags;
@@ -4425,12 +4440,31 @@
return timeout;
}
+/**
+ * wait_for_completion: - waits for completion of a task
+ * @x: holds the state of this particular completion
+ *
+ * This waits to be signaled for completion of a specific task. It is NOT
+ * interruptible and there is no timeout.
+ *
+ * See also similar routines (i.e. wait_for_completion_timeout()) with timeout
+ * and interrupt capability. Also see complete().
+ */
void __sched wait_for_completion(struct completion *x)
{
wait_for_common(x, MAX_SCHEDULE_TIMEOUT, TASK_UNINTERRUPTIBLE);
}
EXPORT_SYMBOL(wait_for_completion);
+/**
+ * wait_for_completion_timeout: - waits for completion of a task (w/timeout)
+ * @x: holds the state of this particular completion
+ * @timeout: timeout value in jiffies
+ *
+ * This waits for either a completion of a specific task to be signaled or for a
+ * specified timeout to expire. The timeout is in jiffies. It is not
+ * interruptible.
+ */
unsigned long __sched
wait_for_completion_timeout(struct completion *x, unsigned long timeout)
{
@@ -4438,6 +4472,13 @@
}
EXPORT_SYMBOL(wait_for_completion_timeout);
+/**
+ * wait_for_completion_interruptible: - waits for completion of a task (w/intr)
+ * @x: holds the state of this particular completion
+ *
+ * This waits for completion of a specific task to be signaled. It is
+ * interruptible.
+ */
int __sched wait_for_completion_interruptible(struct completion *x)
{
long t = wait_for_common(x, MAX_SCHEDULE_TIMEOUT, TASK_INTERRUPTIBLE);
@@ -4447,6 +4488,14 @@
}
EXPORT_SYMBOL(wait_for_completion_interruptible);
+/**
+ * wait_for_completion_interruptible_timeout: - waits for completion (w/(to,intr))
+ * @x: holds the state of this particular completion
+ * @timeout: timeout value in jiffies
+ *
+ * This waits for either a completion of a specific task to be signaled or for a
+ * specified timeout to expire. It is interruptible. The timeout is in jiffies.
+ */
unsigned long __sched
wait_for_completion_interruptible_timeout(struct completion *x,
unsigned long timeout)
@@ -4455,6 +4504,13 @@
}
EXPORT_SYMBOL(wait_for_completion_interruptible_timeout);
+/**
+ * wait_for_completion_killable: - waits for completion of a task (killable)
+ * @x: holds the state of this particular completion
+ *
+ * This waits to be signaled for completion of a specific task. It can be
+ * interrupted by a kill signal.
+ */
int __sched wait_for_completion_killable(struct completion *x)
{
long t = wait_for_common(x, MAX_SCHEDULE_TIMEOUT, TASK_KILLABLE);
* Kevin Diggs <[email protected]> wrote:
> Here is the latest (maybe line wrap is set to 99 so it won't be goofed
> anymore?):
the patch wont apply as it has whitespace corruption (double spaces).
Please check Documentation/email-clients.txt.
Ingo
Ingo Molnar wrote:
> * Kevin Diggs <[email protected]> wrote:
>
>
>>Here is the latest (maybe line wrap is set to 99 so it won't be goofed
>>anymore?):
>
>
> the patch wont apply as it has whitespace corruption (double spaces).
> Please check Documentation/email-clients.txt.
>
> Ingo
>
>
The double spacing between the diffs? That will prevent it from applying?
I'm still just trying to get the content correct.
Who should this patch be sent to when it is ready? Also, I think I remember
reading in submitting patches that patches should be cced to the general
linux-kernel list. This correct?
kevin
* Kevin Diggs <[email protected]> wrote:
> Ingo Molnar wrote:
>> * Kevin Diggs <[email protected]> wrote:
>>
>>
>>> Here is the latest (maybe line wrap is set to 99 so it won't be
>>> goofed anymore?):
>>
>>
>> the patch wont apply as it has whitespace corruption (double spaces).
>> Please check Documentation/email-clients.txt.
>>
>> Ingo
>>
>>
> The double spacing between the diffs? That will prevent it from
> applying?
No. It's a per line corruption. Instead of:
"+<space>code"
your mail had:
"+<space><space>code"
i usually fix that up for small patch but this wasnt a smaller patch :)
> Who should this patch be sent to when it is ready? Also, I think I
> remember reading in submitting patches that patches should be cced to
> the general linux-kernel list. This correct?
me would be fine, it's scheduler code. I can take MIME attachments too
if that's easier for you.
Ingo