[PATCH] kernel/async.c:introduce async_schedule*_atomic

[PATCH] kernel/async.c:introduce async_schedule*_atomic

[tom.leiming]

From: Ming Lei <tom.leiming@gmail.com>

The async_schedule* may not be called in atomic contexts if out of
memory or if there's too much work pending already, because the
async function to be called may sleep.

This patch fixes the comment of async_schedule*, and introduces
async_schedules*_atomic to allow them called from atomic contexts
safely.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 include/linux/async.h |    3 ++
 kernel/async.c        |   56 ++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/include/linux/async.h b/include/linux/async.h
index 68a9530..ede9849 100644
--- a/include/linux/async.h
+++ b/include/linux/async.h
@@ -19,6 +19,9 @@ typedef void (async_func_ptr) (void *data, async_cookie_t cookie);
 extern async_cookie_t async_schedule(async_func_ptr *ptr, void *data);
 extern async_cookie_t async_schedule_domain(async_func_ptr *ptr, void *data,
 					    struct list_head *list);
+extern async_cookie_t async_schedule_atomic(async_func_ptr *ptr, void *data);
+extern async_cookie_t async_schedule_domain_atomic(async_func_ptr *ptr, \
+					void *data, struct list_head *list);
 extern void async_synchronize_full(void);
 extern void async_synchronize_full_domain(struct list_head *list);
 extern void async_synchronize_cookie(async_cookie_t cookie);
diff --git a/kernel/async.c b/kernel/async.c
index 968ef94..6bf565b 100644
--- a/kernel/async.c
+++ b/kernel/async.c
@@ -172,12 +172,13 @@ out:
 }
 
 
-static async_cookie_t __async_schedule(async_func_ptr *ptr, void *data, struct list_head *running)
+static async_cookie_t __async_schedule(async_func_ptr *ptr, void *data, \
+				struct list_head *running, int atomic)
 {
 	struct async_entry *entry;
 	unsigned long flags;
 	async_cookie_t newcookie;
-	
+	int  sync_run = 0;
 
 	/* allow irq-off callers */
 	entry = kzalloc(sizeof(struct async_entry), GFP_ATOMIC);
@@ -186,7 +187,9 @@ static async_cookie_t __async_schedule(async_func_ptr *ptr, void *data, struct l
 	 * If we're out of memory or if there's too much work
 	 * pending already, we execute synchronously.
 	 */
-	if (!async_enabled || !entry || atomic_read(&entry_count) > MAX_WORK) {
+	sync_run = !async_enabled || !entry || \
+			atomic_read(&entry_count) > MAX_WORK;
+	if (sync_run && !atomic) {
 		kfree(entry);
 		spin_lock_irqsave(&async_lock, flags);
 		newcookie = next_cookie++;
@@ -195,7 +198,10 @@ static async_cookie_t __async_schedule(async_func_ptr *ptr, void *data, struct l
 		/* low on memory.. run synchronously */
 		ptr(data, newcookie);
 		return newcookie;
+	} else if (sync_run) {
+		return 0;
 	}
+
 	entry->func = ptr;
 	entry->data = data;
 	entry->running = running;
@@ -215,15 +221,31 @@ static async_cookie_t __async_schedule(async_func_ptr *ptr, void *data, struct l
  * @data: data pointer to pass to the function
  *
  * Returns an async_cookie_t that may be used for checkpointing later.
- * Note: This function may be called from atomic or non-atomic contexts.
+ * Note:This function may be called from non-atomic contexts,and not
+ * 	called from atomic contexts with safety. Please use
+ * 	async_schedule_atomic in atomic contexts.
  */
 async_cookie_t async_schedule(async_func_ptr *ptr, void *data)
 {
-	return __async_schedule(ptr, data, &async_running);
+	return __async_schedule(ptr, data, &async_running, 0);
 }
 EXPORT_SYMBOL_GPL(async_schedule);
 
 /**
+ * async_schedule_atomic - schedule a function for asynchronous execution
+ * @ptr: function to execute asynchronously
+ * @data: data pointer to pass to the function
+ *
+ * Returns an async_cookie_t that may be used for checkpointing later.
+ * Note: This function can be called from atomic contexts safely.
+ */
+async_cookie_t async_schedule_atomic(async_func_ptr *ptr, void *data)
+{
+	return __async_schedule(ptr, data, &async_running, 1);
+}
+EXPORT_SYMBOL_GPL(async_schedule_atomic);
+
+/**
  * async_schedule_domain - schedule a function for asynchronous execution within a certain domain
  * @ptr: function to execute asynchronously
  * @data: data pointer to pass to the function
@@ -233,16 +255,36 @@ EXPORT_SYMBOL_GPL(async_schedule);
  * @running may be used in the async_synchronize_*_domain() functions
  * to wait within a certain synchronization domain rather than globally.
  * A synchronization domain is specified via the running queue @running to use.
- * Note: This function may be called from atomic or non-atomic contexts.
+ * Note:This function may be called from non-atomic contexts,and not
+ * 	called from atomic contexts with safety. Please use
+ * 	async_schedule_domain_atomic in atomic contexts.
  */
 async_cookie_t async_schedule_domain(async_func_ptr *ptr, void *data,
 				     struct list_head *running)
 {
-	return __async_schedule(ptr, data, running);
+	return __async_schedule(ptr, data, running, 0);
 }
 EXPORT_SYMBOL_GPL(async_schedule_domain);
 
 /**
+ * async_schedule_domain_atomic - schedule a function for asynchronous execution within a certain domain
+ * @ptr: function to execute asynchronously
+ * @data: data pointer to pass to the function
+ * @running: running list for the domain
+ *
+ * Returns an async_cookie_t that may be used for checkpointing later.
+ * @running may be used in the async_synchronize_*_domain() functions
+ * to wait within a certain synchronization domain rather than globally.
+ * A synchronization domain is specified via the running queue @running to use.
+ * Note: This function can be called from atomic contexts safely.
+ */
+async_cookie_t async_schedule_domain_atomic(async_func_ptr *ptr, void *data,
+				     struct list_head *running)
+{
+	return __async_schedule(ptr, data, running, 1);
+}
+EXPORT_SYMBOL_GPL(async_schedule_domain_atomic);
+/**
  * async_synchronize_full - synchronize all asynchronous function calls
  *
  * This function waits until all asynchronous function calls have been done.
-- 
1.6.0.GIT

Re: [PATCH] kernel/async.c:introduce async_schedule*_atomic

[Frederic Weisbecker]

On Tue, May 12, 2009 at 11:13:42PM +0800, tom.leiming@gmail.com wrote:
> From: Ming Lei <tom.leiming@gmail.com>
> 
> The async_schedule* may not be called in atomic contexts if out of
> memory or if there's too much work pending already, because the
> async function to be called may sleep.
> 
> This patch fixes the comment of async_schedule*, and introduces
> async_schedules*_atomic to allow them called from atomic contexts
> safely.



Aah, great. Such helper could easily replace some workqueues
which receive (in atomic) rare jobs but still need to exist because
they execute jobs which take too much time to be enqueued in kevents.

A good candidate: kpsmoused!


 
> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
> ---
>  include/linux/async.h |    3 ++
>  kernel/async.c        |   56 ++++++++++++++++++++++++++++++++++++++++++------
>  2 files changed, 52 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/async.h b/include/linux/async.h
> index 68a9530..ede9849 100644
> --- a/include/linux/async.h
> +++ b/include/linux/async.h
> @@ -19,6 +19,9 @@ typedef void (async_func_ptr) (void *data, async_cookie_t cookie);
>  extern async_cookie_t async_schedule(async_func_ptr *ptr, void *data);
>  extern async_cookie_t async_schedule_domain(async_func_ptr *ptr, void *data,
>  					    struct list_head *list);
> +extern async_cookie_t async_schedule_atomic(async_func_ptr *ptr, void *data);
> +extern async_cookie_t async_schedule_domain_atomic(async_func_ptr *ptr, \


trailing backslash?


> +					void *data, struct list_head *list);
>  extern void async_synchronize_full(void);
>  extern void async_synchronize_full_domain(struct list_head *list);
>  extern void async_synchronize_cookie(async_cookie_t cookie);
> diff --git a/kernel/async.c b/kernel/async.c
> index 968ef94..6bf565b 100644
> --- a/kernel/async.c
> +++ b/kernel/async.c
> @@ -172,12 +172,13 @@ out:
>  }
>  
>  
> -static async_cookie_t __async_schedule(async_func_ptr *ptr, void *data, struct list_head *running)
> +static async_cookie_t __async_schedule(async_func_ptr *ptr, void *data, \


another one.


> +				struct list_head *running, int atomic)
>  {
>  	struct async_entry *entry;
>  	unsigned long flags;
>  	async_cookie_t newcookie;
> -	
> +	int  sync_run = 0;
>  
>  	/* allow irq-off callers */
>  	entry = kzalloc(sizeof(struct async_entry), GFP_ATOMIC);
> @@ -186,7 +187,9 @@ static async_cookie_t __async_schedule(async_func_ptr *ptr, void *data, struct l
>  	 * If we're out of memory or if there's too much work
>  	 * pending already, we execute synchronously.
>  	 */
> -	if (!async_enabled || !entry || atomic_read(&entry_count) > MAX_WORK) {
> +	sync_run = !async_enabled || !entry || \
> +			atomic_read(&entry_count) > MAX_WORK;
> +	if (sync_run && !atomic) {
>  		kfree(entry);
>  		spin_lock_irqsave(&async_lock, flags);
>  		newcookie = next_cookie++;
> @@ -195,7 +198,10 @@ static async_cookie_t __async_schedule(async_func_ptr *ptr, void *data, struct l
>  		/* low on memory.. run synchronously */
>  		ptr(data, newcookie);
>  		return newcookie;
> +	} else if (sync_run) {
> +		return 0;


Then it's up to the caller to handle the error. I guess it's the best
way to do.
You could put it in a separate list and retry later, but it wouldn't
be a good idea because the work submitter couldn't be sure of the end
result.

So I guess you did a right choice.



>  	}
> +
>  	entry->func = ptr;
>  	entry->data = data;
>  	entry->running = running;
> @@ -215,15 +221,31 @@ static async_cookie_t __async_schedule(async_func_ptr *ptr, void *data, struct l
>   * @data: data pointer to pass to the function
>   *
>   * Returns an async_cookie_t that may be used for checkpointing later.
> - * Note: This function may be called from atomic or non-atomic contexts.
> + * Note:This function may be called from non-atomic contexts,and not
> + * 	called from atomic contexts with safety. Please use
> + * 	async_schedule_atomic in atomic contexts.
>   */
>  async_cookie_t async_schedule(async_func_ptr *ptr, void *data)
>  {
> -	return __async_schedule(ptr, data, &async_running);
> +	return __async_schedule(ptr, data, &async_running, 0);
>  }
>  EXPORT_SYMBOL_GPL(async_schedule);
>  
>  /**
> + * async_schedule_atomic - schedule a function for asynchronous execution
> + * @ptr: function to execute asynchronously
> + * @data: data pointer to pass to the function
> + *
> + * Returns an async_cookie_t that may be used for checkpointing later.
> + * Note: This function can be called from atomic contexts safely.
> + */



Please add a comment to tell that it might fail and return 0
in that case.

May be it would even be worth to detail the error. A cookie
can't be negative, then you can use the common error pattern.

-ENOMEM while running out of memory
-ENOSPC when the threshold number of async work has been overlapped
....

Not sure such precision about the error path is needed though, it's just
an idea.


> +async_cookie_t async_schedule_atomic(async_func_ptr *ptr, void *data)
> +{
> +	return __async_schedule(ptr, data, &async_running, 1);
> +}
> +EXPORT_SYMBOL_GPL(async_schedule_atomic);
> +
> +/**
>   * async_schedule_domain - schedule a function for asynchronous execution within a certain domain
>   * @ptr: function to execute asynchronously
>   * @data: data pointer to pass to the function
> @@ -233,16 +255,36 @@ EXPORT_SYMBOL_GPL(async_schedule);
>   * @running may be used in the async_synchronize_*_domain() functions
>   * to wait within a certain synchronization domain rather than globally.
>   * A synchronization domain is specified via the running queue @running to use.
> - * Note: This function may be called from atomic or non-atomic contexts.
> + * Note:This function may be called from non-atomic contexts,and not
> + * 	called from atomic contexts with safety. Please use
> + * 	async_schedule_domain_atomic in atomic contexts.
>   */
>  async_cookie_t async_schedule_domain(async_func_ptr *ptr, void *data,
>  				     struct list_head *running)
>  {
> -	return __async_schedule(ptr, data, running);
> +	return __async_schedule(ptr, data, running, 0);
>  }
>  EXPORT_SYMBOL_GPL(async_schedule_domain);
>  
>  /**
> + * async_schedule_domain_atomic - schedule a function for asynchronous execution within a certain domain
> + * @ptr: function to execute asynchronously
> + * @data: data pointer to pass to the function
> + * @running: running list for the domain
> + *
> + * Returns an async_cookie_t that may be used for checkpointing later.
> + * @running may be used in the async_synchronize_*_domain() functions
> + * to wait within a certain synchronization domain rather than globally.
> + * A synchronization domain is specified via the running queue @running to use.
> + * Note: This function can be called from atomic contexts safely.
> + */
> +async_cookie_t async_schedule_domain_atomic(async_func_ptr *ptr, void *data,
> +				     struct list_head *running)
> +{
> +	return __async_schedule(ptr, data, running, 1);
> +}
> +EXPORT_SYMBOL_GPL(async_schedule_domain_atomic);
> +/**
>   * async_synchronize_full - synchronize all asynchronous function calls
>   *
>   * This function waits until all asynchronous function calls have been done.
> -- 
> 1.6.0.GIT
> 


Otherwise it looks good, and IMHO it is needed.

Re: [PATCH] kernel/async.c:introduce async_schedule*_atomic

[Américo Wang]

On Tue, May 12, 2009 at 05:44:58PM +0200, Frederic Weisbecker wrote:
>On Tue, May 12, 2009 at 11:13:42PM +0800, tom.leiming@gmail.com wrote:
>> From: Ming Lei <tom.leiming@gmail.com>
>> 
>> The async_schedule* may not be called in atomic contexts if out of
>> memory or if there's too much work pending already, because the
>> async function to be called may sleep.
>> 
>> This patch fixes the comment of async_schedule*, and introduces
>> async_schedules*_atomic to allow them called from atomic contexts
>> safely.
>
>
>
>Aah, great. Such helper could easily replace some workqueues
>which receive (in atomic) rare jobs but still need to exist because
>they execute jobs which take too much time to be enqueued in kevents.
>
>A good candidate: kpsmoused!
>
>
> 
>> Signed-off-by: Ming Lei <tom.leiming@gmail.com>
>> ---
>>  include/linux/async.h |    3 ++
>>  kernel/async.c        |   56 ++++++++++++++++++++++++++++++++++++++++++------
>>  2 files changed, 52 insertions(+), 7 deletions(-)
>> 
>> diff --git a/include/linux/async.h b/include/linux/async.h
>> index 68a9530..ede9849 100644
>> --- a/include/linux/async.h
>> +++ b/include/linux/async.h
>> @@ -19,6 +19,9 @@ typedef void (async_func_ptr) (void *data, async_cookie_t cookie);
>>  extern async_cookie_t async_schedule(async_func_ptr *ptr, void *data);
>>  extern async_cookie_t async_schedule_domain(async_func_ptr *ptr, void *data,
>>  					    struct list_head *list);
>> +extern async_cookie_t async_schedule_atomic(async_func_ptr *ptr, void *data);
>> +extern async_cookie_t async_schedule_domain_atomic(async_func_ptr *ptr, \
>
>
>trailing backslash?
>
>
>> +					void *data, struct list_head *list);
>>  extern void async_synchronize_full(void);
>>  extern void async_synchronize_full_domain(struct list_head *list);
>>  extern void async_synchronize_cookie(async_cookie_t cookie);
>> diff --git a/kernel/async.c b/kernel/async.c
>> index 968ef94..6bf565b 100644
>> --- a/kernel/async.c
>> +++ b/kernel/async.c
>> @@ -172,12 +172,13 @@ out:
>>  }
>>  
>>  
>> -static async_cookie_t __async_schedule(async_func_ptr *ptr, void *data, struct list_head *running)
>> +static async_cookie_t __async_schedule(async_func_ptr *ptr, void *data, \
>
>
>another one.
>
>
>> +				struct list_head *running, int atomic)
>>  {
>>  	struct async_entry *entry;
>>  	unsigned long flags;
>>  	async_cookie_t newcookie;
>> -	
>> +	int  sync_run = 0;
>>  
>>  	/* allow irq-off callers */
>>  	entry = kzalloc(sizeof(struct async_entry), GFP_ATOMIC);
>> @@ -186,7 +187,9 @@ static async_cookie_t __async_schedule(async_func_ptr *ptr, void *data, struct l
>>  	 * If we're out of memory or if there's too much work
>>  	 * pending already, we execute synchronously.
>>  	 */
>> -	if (!async_enabled || !entry || atomic_read(&entry_count) > MAX_WORK) {
>> +	sync_run = !async_enabled || !entry || \
>> +			atomic_read(&entry_count) > MAX_WORK;


And also this one...

Hey, Ming, none of these are macros... You don't need to add backslash to
join two lines, C compilers can recognize.

-- 
Live like a child, think like the god.
 

Re: [PATCH] kernel/async.c:introduce async_schedule*_atomic

[Ming Lei]

2009/5/12 Américo Wang <xiyou.wangcong@gmail.com>:
> On Tue, May 12, 2009 at 05:44:58PM +0200, Frederic Weisbecker wrote:
>>On Tue, May 12, 2009 at 11:13:42PM +0800, tom.leiming@gmail.com wrote:
>>> From: Ming Lei <tom.leiming@gmail.com>
>>>
>>> -    if (!async_enabled || !entry || atomic_read(&entry_count) > MAX_WORK) {
>>> +    sync_run = !async_enabled || !entry || \
>>> +                    atomic_read(&entry_count) > MAX_WORK;
>
>
> And also this one...
>
> Hey, Ming, none of these are macros... You don't need to add backslash to
> join two lines, C compilers can recognize.

OK, I'll fix it in next version.
Thanks!

>
> --
> Live like a child, think like the god.
>
>



-- 
Lei Ming

Re: [PATCH] kernel/async.c:introduce async_schedule*_atomic

[Frederic Weisbecker]

On Tue, May 12, 2009 at 05:44:58PM +0200, Frederic Weisbecker wrote:
> On Tue, May 12, 2009 at 11:13:42PM +0800, tom.leiming@gmail.com wrote:
> > From: Ming Lei <tom.leiming@gmail.com>
> > 
> > The async_schedule* may not be called in atomic contexts if out of
> > memory or if there's too much work pending already, because the
> > async function to be called may sleep.
> > 
> > This patch fixes the comment of async_schedule*, and introduces
> > async_schedules*_atomic to allow them called from atomic contexts
> > safely.



Note that async_schedule_atomic is a confusing name.
At a first glance, it could mean that the scheduled job
will be run atomically.

I would suggest async_schedule_inatomic() so that it follows the common
naming pattern in use in the kernel, eg:

- copy_from_user_inatomic()
- futex_atomic_cmpxchg_inatomic()

and so on.



> >   * Returns an async_cookie_t that may be used for checkpointing later.
> > - * Note: This function may be called from atomic or non-atomic contexts.
> > + * Note:This function may be called from non-atomic contexts,and not
> > + * 	called from atomic contexts with safety. Please use
> > + * 	async_schedule_atomic in atomic contexts.


I suggest to add a comment which explains the reason for which it is unsafe
to call it in atomic context: because the scheduled work might be synchronously
executed.

One could believe this is because async_schedule() internally uses
a function which might sleep whereas the actual problem may come
from the scheduled function.

BTW, now that we have an atomic safe version, may be we could
also adapt the kmalloc GFP flags subsequently?


Frederic.

Re: [PATCH] kernel/async.c:introduce async_schedule*_atomic

[Cornelia Huck]

On Tue, 12 May 2009 18:04:35 +0200,
Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Tue, May 12, 2009 at 05:44:58PM +0200, Frederic Weisbecker wrote:
> > On Tue, May 12, 2009 at 11:13:42PM +0800, tom.leiming@gmail.com wrote:
> > >   * Returns an async_cookie_t that may be used for checkpointing later.
> > > - * Note: This function may be called from atomic or non-atomic contexts.
> > > + * Note:This function may be called from non-atomic contexts,and not
> > > + * 	called from atomic contexts with safety. Please use
> > > + * 	async_schedule_atomic in atomic contexts.
> 
> 
> I suggest to add a comment which explains the reason for which it is unsafe
> to call it in atomic context: because the scheduled work might be synchronously
> executed.
> 
> One could believe this is because async_schedule() internally uses
> a function which might sleep whereas the actual problem may come
> from the scheduled function.

I'm wondering whether this is not mixing two different things up:
- Making async_schedule_* safe from an atomic context.
- Disallowing calling the function synchronously if asynchronous
  scheduling failed.

Perhaps we want async_schedule_nosync() in addition?

> 
> BTW, now that we have an atomic safe version, may be we could
> also adapt the kmalloc GFP flags subsequently?

Yes, that would make sense.

Re: [PATCH] kernel/async.c:introduce async_schedule*_atomic

[Frederic Weisbecker]

On Tue, May 12, 2009 at 06:31:05PM +0200, Cornelia Huck wrote:
> On Tue, 12 May 2009 18:04:35 +0200,
> Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > On Tue, May 12, 2009 at 05:44:58PM +0200, Frederic Weisbecker wrote:
> > > On Tue, May 12, 2009 at 11:13:42PM +0800, tom.leiming@gmail.com wrote:
> > > >   * Returns an async_cookie_t that may be used for checkpointing later.
> > > > - * Note: This function may be called from atomic or non-atomic contexts.
> > > > + * Note:This function may be called from non-atomic contexts,and not
> > > > + * 	called from atomic contexts with safety. Please use
> > > > + * 	async_schedule_atomic in atomic contexts.
> > 
> > 
> > I suggest to add a comment which explains the reason for which it is unsafe
> > to call it in atomic context: because the scheduled work might be synchronously
> > executed.
> > 
> > One could believe this is because async_schedule() internally uses
> > a function which might sleep whereas the actual problem may come
> > from the scheduled function.
> 
> I'm wondering whether this is not mixing two different things up:
> - Making async_schedule_* safe from an atomic context.


It was already safe from atomic context, async_schedule() internals
does not make use of sleeping functions.
So indeed the inatomic suffix is not that much appropriate from
an async internal point in view, it makes sense only from the
caller/callee point of view.

But if we start worrying about the GFP flags used by __async_schedule(),
then yes we are mixing up two things.


> - Disallowing calling the function synchronously if asynchronous
>   scheduling failed.
> 
> Perhaps we want async_schedule_nosync() in addition?



This division would make more sense indeed.

- async_schedule_inatomic() would be nosync() and would use
  GFP_ATOMIC. I guess the case where we want to run
  a job synchronously from atomic in case of async failure is too rare
  (non-existent?).
- async_schedule_nosync() would be only nosync() and would use
  GFP_KERNEL

I'm not sure the second case will ever be used though.

Another alternative would be to define a single async_schedule_nosync()
which also takes a gfp flag.


> > 
> > BTW, now that we have an atomic safe version, may be we could
> > also adapt the kmalloc GFP flags subsequently?
> 
> Yes, that would make sense.

Re: [PATCH] kernel/async.c:introduce async_schedule*_atomic

[Cornelia Huck]

On Tue, 12 May 2009 18:52:29 +0200,
Frederic Weisbecker <fweisbec@gmail.com> wrote:

> This division would make more sense indeed.
> 
> - async_schedule_inatomic() would be nosync() and would use
>   GFP_ATOMIC. I guess the case where we want to run
>   a job synchronously from atomic in case of async failure is too rare
>   (non-existent?).

It would add complexity for those callers providing a function that is
safe to be called in both contexts.

> - async_schedule_nosync() would be only nosync() and would use
>   GFP_KERNEL
> 
> I'm not sure the second case will ever be used though.

It might make sense for the "just fail if we cannot get memory" case.

> 
> Another alternative would be to define a single async_schedule_nosync()
> which also takes a gfp flag.

Wouldn't async_schedule() then need a gfp flag as well?

Re: [PATCH] kernel/async.c:introduce async_schedule*_atomic

[Ming Lei]

2009/5/13 Cornelia Huck <cornelia.huck@de.ibm.com>:
> On Tue, 12 May 2009 18:52:29 +0200,
> Frederic Weisbecker <fweisbec@gmail.com> wrote:
>
>> This division would make more sense indeed.
>>
>> - async_schedule_inatomic() would be nosync() and would use
>>   GFP_ATOMIC. I guess the case where we want to run
>>   a job synchronously from atomic in case of async failure is too rare
>>   (non-existent?).
>
> It would add complexity for those callers providing a function that is
> safe to be called in both contexts.
>
>> - async_schedule_nosync() would be only nosync() and would use
>>   GFP_KERNEL
>>
>> I'm not sure the second case will ever be used though.
>
> It might make sense for the "just fail if we cannot get memory" case.
>
>>
>> Another alternative would be to define a single async_schedule_nosync()
>> which also takes a gfp flag.
>
> Wouldn't async_schedule() then need a gfp flag as well?
>

IMHO, we should call async_schedule*() from non-atomic contexts and
async_schedule_inatomic*() from atomic contexts explicitly, so
async_schedule*()
use GFP_KERNEL and async_schedule_inatomic*() use GFP_ATOMIC
always. This can simplify the problem much more.

Also we still allow async_schedule*()  to run a job synchronously if
out of memory
or other failure. This can keep consistency with before.

Any sugesstions or objections?

-- 
Lei Ming

Re: [PATCH] kernel/async.c:introduce async_schedule*_atomic

[Frederic Weisbecker]

On Wed, May 13, 2009 at 08:28:15AM +0800, Ming Lei wrote:
> 2009/5/13 Cornelia Huck <cornelia.huck@de.ibm.com>:
> > On Tue, 12 May 2009 18:52:29 +0200,
> > Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >
> >> This division would make more sense indeed.
> >>
> >> - async_schedule_inatomic() would be nosync() and would use
> >>   GFP_ATOMIC. I guess the case where we want to run
> >>   a job synchronously from atomic in case of async failure is too rare
> >>   (non-existent?).
> >
> > It would add complexity for those callers providing a function that is
> > safe to be called in both contexts.
> >
> >> - async_schedule_nosync() would be only nosync() and would use
> >>   GFP_KERNEL
> >>
> >> I'm not sure the second case will ever be used though.
> >
> > It might make sense for the "just fail if we cannot get memory" case.
> >
> >>
> >> Another alternative would be to define a single async_schedule_nosync()
> >> which also takes a gfp flag.
> >
> > Wouldn't async_schedule() then need a gfp flag as well?
> >
> 
> IMHO, we should call async_schedule*() from non-atomic contexts and
> async_schedule_inatomic*() from atomic contexts explicitly, so
> async_schedule*()
> use GFP_KERNEL and async_schedule_inatomic*() use GFP_ATOMIC
> always. This can simplify the problem much more.



I think Cornelia is right about the complex case of a job
launched from atomic context that could either be run
synchronously. I have troubles to imagine such case though
but I guess it's possible.

 
> Also we still allow async_schedule*()  to run a job synchronously if
> out of memory
> or other failure. This can keep consistency with before.


Yes, but also most of the current users of async_schedule() could call
it with GFP_KERNEL. For now it's not an issue because it is not widely
used, but who knows how that will evolve...


> Any sugesstions or objections?


I have shared feelings. I don't know if the dual sense of
this new helper deserves enough disambiguation and granularity
to be split up in two parts:

- adding an async_schedule_nosync() helper
- add a new gfpflag_t parameter


Or should we just do:

- adding async_schedule_inatomic() which is a merge of nosync + GFP_ATOMIC
- use GFP_KERNEL in async_schedule()


It depends on the future users. Will someone ever accept to schedule a job
that could end up running synchronously in the worst case?

Frederic.

Re: [PATCH] kernel/async.c:introduce async_schedule*_atomic

[Cornelia Huck]

On Wed, 13 May 2009 03:20:13 +0200,
Frederic Weisbecker <fweisbec@gmail.com> wrote:

> On Wed, May 13, 2009 at 08:28:15AM +0800, Ming Lei wrote:

> > Also we still allow async_schedule*()  to run a job synchronously if
> > out of memory
> > or other failure. This can keep consistency with before.
> 
> 
> Yes, but also most of the current users of async_schedule() could call
> it with GFP_KERNEL. For now it's not an issue because it is not widely
> used, but who knows how that will evolve...

Well, if we want to change the interface, now would be a good time
since there are still few callers.

> 
> 
> > Any sugesstions or objections?
> 
> 
> I have shared feelings. I don't know if the dual sense of
> this new helper deserves enough disambiguation and granularity
> to be split up in two parts:
> 
> - adding an async_schedule_nosync() helper
> - add a new gfpflag_t parameter
> 
> 
> Or should we just do:
> 
> - adding async_schedule_inatomic() which is a merge of nosync + GFP_ATOMIC
> - use GFP_KERNEL in async_schedule()
> 
> 
> It depends on the future users. Will someone ever accept to schedule a job
> that could end up running synchronously in the worst case?

The current callers all look simple enough, it may be that all other
cases besides inatomic+nosync would be overkill (and a too complex api
may lead to confusion).

Do people have candidates for conversion to the async api in mind that
would need one of the complex atomic/sync or non-atomic/non-sync
versions? If no, maybe we should just use the second approach and make
sure that the semantics are well documented.

Re: [PATCH] kernel/async.c:introduce async_schedule*_atomic

[Arjan van de Ven]

On Wed, 13 May 2009 09:47:28 +0200
Cornelia Huck <cornelia.huck@de.ibm.com> wrote:

> On Wed, 13 May 2009 03:20:13 +0200,
> Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > On Wed, May 13, 2009 at 08:28:15AM +0800, Ming Lei wrote:
> 
> > > Also we still allow async_schedule*()  to run a job synchronously
> > > if out of memory
> > > or other failure. This can keep consistency with before.
> > 
> > 
> > Yes, but also most of the current users of async_schedule() could
> > call it with GFP_KERNEL. For now it's not an issue because it is
> > not widely used, but who knows how that will evolve...
> 
> Well, if we want to change the interface, now would be a good time
> since there are still few callers.

I would prefer it that if we make a more complex interface, we keep the
current simple interface as a wrapper, so that the simple case can
remain simple.

-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] kernel/async.c:introduce async_schedule*_atomic

[Cornelia Huck]

On Sun, 17 May 2009 13:59:40 -0700,
Arjan van de Ven <arjan@infradead.org> wrote:

> On Wed, 13 May 2009 09:47:28 +0200
> Cornelia Huck <cornelia.huck@de.ibm.com> wrote:
> 
> > On Wed, 13 May 2009 03:20:13 +0200,
> > Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > 
> > > On Wed, May 13, 2009 at 08:28:15AM +0800, Ming Lei wrote:
> > 
> > > > Also we still allow async_schedule*()  to run a job synchronously
> > > > if out of memory
> > > > or other failure. This can keep consistency with before.
> > > 
> > > 
> > > Yes, but also most of the current users of async_schedule() could
> > > call it with GFP_KERNEL. For now it's not an issue because it is
> > > not widely used, but who knows how that will evolve...
> > 
> > Well, if we want to change the interface, now would be a good time
> > since there are still few callers.
> 
> I would prefer it that if we make a more complex interface, we keep the
> current simple interface as a wrapper, so that the simple case can
> remain simple.

Of course. I was just thinking about changing the semantics of
async_schedule() to doing GFP_KERNEL allocation - now it's still easy
to audit all callers.

Re: [PATCH] kernel/async.c:introduce async_schedule*_atomic

[Ming Lei]

2009/5/13 Cornelia Huck <cornelia.huck@de.ibm.com>:
> On Tue, 12 May 2009 18:52:29 +0200,
> Frederic Weisbecker <fweisbec@gmail.com> wrote:
>
>> This division would make more sense indeed.
>>
>> - async_schedule_inatomic() would be nosync() and would use
>>   GFP_ATOMIC. I guess the case where we want to run
>>   a job synchronously from atomic in case of async failure is too rare
>>   (non-existent?).
>
> It would add complexity for those callers providing a function that is
> safe to be called in both contexts.

So we introduce  async_schedule*_inatomic(), the patch  aims at  making caller
clear that async_schedule*_inatomic() should be used in atomic
contexts instead of
async_schedule*().

>
>> - async_schedule_nosync() would be only nosync() and would use
>>   GFP_KERNEL

I wonder if there is such kind of requirement, can we not introduce it
in the patch?
If someone does need it, we can introduce it later.

>>
>> I'm not sure the second case will ever be used though.
>
> It might make sense for the "just fail if we cannot get memory" case.
>
>>
>> Another alternative would be to define a single async_schedule_nosync()
>> which also takes a gfp flag.
>
> Wouldn't async_schedule() then need a gfp flag as well?
>

IMHO, it is better that async_schedule()  is always called in
non-atomic contexts and
async_schedule*_inatomic() is always called in atomic contexts, so we
can't need a gfp
flag, right?


-- 
Lei Ming

Re: [PATCH] kernel/async.c:introduce async_schedule*_atomic

[Ming Lei]

2009/5/13 Frederic Weisbecker <fweisbec@gmail.com>:
> On Tue, May 12, 2009 at 05:44:58PM +0200, Frederic Weisbecker wrote:

> Note that async_schedule_atomic is a confusing name.
> At a first glance, it could mean that the scheduled job
> will be run atomically.
>
> I would suggest async_schedule_inatomic() so that it follows the common
> naming pattern in use in the kernel, eg:
>
> - copy_from_user_inatomic()
> - futex_atomic_cmpxchg_inatomic()
>
> and so on.

Agree, I'll fix it in next version.

>
>
>
>> >   * Returns an async_cookie_t that may be used for checkpointing later.
>> > - * Note: This function may be called from atomic or non-atomic contexts.
>> > + * Note:This function may be called from non-atomic contexts,and not
>> > + *         called from atomic contexts with safety. Please use
>> > + *         async_schedule_atomic in atomic contexts.
>
>
> I suggest to add a comment which explains the reason for which it is unsafe
> to call it in atomic context: because the scheduled work might be synchronously
> executed.
>
> One could believe this is because async_schedule() internally uses
> a function which might sleep whereas the actual problem may come
> from the scheduled function.

Agree,add it in next version.

>
> BTW, now that we have an atomic safe version, may be we could
> also adapt the kmalloc GFP flags subsequently?

-- 
Lei Ming

Re: [PATCH] kernel/async.c:introduce async_schedule*_atomic

[Arjan van de Ven]

On Tue, 12 May 2009 23:13:42 +0800
tom.leiming@gmail.com wrote:

> From: Ming Lei <tom.leiming@gmail.com>
> 
> The async_schedule* may not be called in atomic contexts if out of
> memory or if there's too much work pending already, because the
> async function to be called may sleep.
> 
> This patch fixes the comment of async_schedule*, and introduces
> async_schedules*_atomic to allow them called from atomic contexts
> safely.

(sorry for the late response; have been away from most of my email for
a few days)

I like the general idea; I was hoping to do it a little bit different
though, API wise.
I don't mind the parameter for "don't do blocking things" (we should
use that to also use GFP_KERNEL/GFP_NOFS or whatever for the
allocation), it makes sense.

What I would like to see is the option to pass in memory that was
externally kmalloc'd. So that you can do

foo = kmalloc(..)
spin_lock(bar)
...
async_schedule_atomic(...);

spin_unlock(bar);
if (not_used_foo)
    kfree(foo);

in cases where you don't want to fail while in the atomic portion,
but can fail better earlier.



-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: [PATCH] kernel/async.c:introduce async_schedule*_atomic

[Ming Lei]

2009/5/18 Arjan van de Ven <arjan@infradead.org>:
> On Tue, 12 May 2009 23:13:42 +0800
> tom.leiming@gmail.com wrote:
>
>> From: Ming Lei <tom.leiming@gmail.com>
>>
>> The async_schedule* may not be called in atomic contexts if out of
>> memory or if there's too much work pending already, because the
>> async function to be called may sleep.
>>
>> This patch fixes the comment of async_schedule*, and introduces
>> async_schedules*_atomic to allow them called from atomic contexts
>> safely.
>
> (sorry for the late response; have been away from most of my email for
> a few days)
>
> I like the general idea; I was hoping to do it a little bit different
> though, API wise.
> I don't mind the parameter for "don't do blocking things" (we should
> use that to also use GFP_KERNEL/GFP_NOFS or whatever for the
> allocation), it makes sense.
>
> What I would like to see is the option to pass in memory that was
> externally kmalloc'd. So that you can do
>
> foo = kmalloc(..)
> spin_lock(bar)
> ...
> async_schedule_atomic(...);
>
> spin_unlock(bar);
> if (not_used_foo)
>    kfree(foo);
>
> in cases where you don't want to fail while in the atomic portion,
> but can fail better earlier.

If we do this way, the type of async_schedule*()  and async_schedule*_inatomic()
is different, and callers will be messed with them, especially for _inatomic().

BTW: I have submited new version of this patches, would you mind
giving a review?

Thanks.

-- 
Lei Ming

Re: [PATCH] kernel/async.c:introduce async_schedule*_atomic

[Arjan van de Ven]

On Mon, 18 May 2009 09:55:14 +0800
Ming Lei <tom.leiming@gmail.com> wrote:

> >
> > in cases where you don't want to fail while in the atomic portion,
> > but can fail better earlier.
> 
> If we do this way, the type of async_schedule*()  and
> async_schedule*_inatomic() is different, and callers will be messed
> with them, especially for _inatomic().

I'm not so much proposing the external "simple API" to have this,
but the primitive should have it, so that we can provide a
__async_schedule_inatomic() which does have the option..


> 
> BTW: I have submited new version of this patches, would you mind
> giving a review?

I gave comments on various threads from my inbox, but I cannot rule out
having missed one... If you think I missed one, could you send me a 
private email with the latest patches you want reviewed (even if it's a
forward from what you sent earlier)?

I'm happy to see these enhancements btw...


-- 
Arjan van de Ven 	Intel Open Source Technology Centre
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org