Re: [linux-pm] [PATCH] Workqueue freezer support.

[Patrick Mochel]

On Thu, 21 Jul 2005, Nigel Cunningham wrote:

> This patch implements freezer support for workqueues. The current
> refrigerator implementation makes all workqueues NOFREEZE, regardless of
> whether they need to be or not.

A few comments..

> Signed-off by: Nigel Cunningham <nigel@xxxxxxxxxxxx>
>
>  drivers/acpi/osl.c          |    2 +-
>  drivers/block/ll_rw_blk.c   |    2 +-
>  drivers/char/hvc_console.c  |    2 +-
>  drivers/char/hvcs.c         |    2 +-
>  drivers/input/serio/serio.c |    2 +-
>  drivers/md/dm-crypt.c       |    2 +-
>  drivers/scsi/hosts.c        |    2 +-
>  drivers/usb/net/pegasus.c   |    2 +-

If you want some practice splitting things up, submit the patches above
individually to the maintainers o the relevant code once the patches you
submit below get merged to -mm.

>  include/linux/kthread.h     |   20 ++++++++++++++++++--
>  include/linux/workqueue.h   |    9 ++++++---
>  kernel/kmod.c               |    4 ++++
>  kernel/kthread.c            |   23 ++++++++++++++++++++++-
>  kernel/sched.c              |    4 ++--
>  kernel/softirq.c            |    3 +--
>  kernel/workqueue.c          |   21 ++++++++++++---------
>  15 files changed, 73 insertions(+), 27 deletions(-)


You should make sure that you get an explicit ACK from people (Ingo et al)
about whether this is an acceptable interface.

> --- 400-workthreads.patch-old/include/linux/kthread.h	2004-11-03 21:51:12.000000000 +1100
> +++ 400-workthreads.patch-new/include/linux/kthread.h	2005-07-20 15:11:37.000000000 +1000
> @@ -27,6 +27,14 @@ struct task_struct *kthread_create(int (
>  				   void *data,
>  				   const char namefmt[], ...);
>
> +struct task_struct *_kthread_create(int (*threadfn)(void *data),
> +				   void *data,
> +				   unsigned long freezer_flags,
> +				   const char namefmt[], ...);
> +

This should be __kthread_create(...)

> -#define kthread_run(threadfn, data, namefmt, ...)			   \
> +#define kthread_run(threadfn, data, namefmt, args...)			   \
>  ({									   \
>  	struct task_struct *__k						   \
> -		= kthread_create(threadfn, data, namefmt, ## __VA_ARGS__); \
> +		= kthread_create(threadfn, data, namefmt, ##args);	   \
>  	if (!IS_ERR(__k))						   \
>  		wake_up_process(__k);					   \
>  	__k;								   \
>  })
>
> +#define kthread_nofreeze_run(threadfn, data, namefmt, args...)		   \
> +({									   \
> +	struct task_struct *__k	= kthread_nofreeze_create(threadfn, data,  \
> +			namefmt, ##args);				   \
> +	if (!IS_ERR(__k))						   \
> +		wake_up_process(__k);					   \
> +	__k;								   \
> +})

Do these functions need to be inlined?

> @@ -86,6 +87,10 @@ static int kthread(void *_create)
>  	/* By default we can run anywhere, unlike keventd. */
>  	set_cpus_allowed(current, CPU_MASK_ALL);
>
> +	/* Set our freezer flags */
> +	current->flags &= ~(PF_SYNCTHREAD | PF_NOFREEZE);
> +	current->flags |= (create->freezer_flags & PF_NOFREEZE);
> +

Maybe these should be encapsulated in a helper in include/linux/sched.h
like some other flags manipulations are?

> diff -ruNp 400-workthreads.patch-old/kernel/sched.c 400-workthreads.patch-new/kernel/sched.c
> --- 400-workthreads.patch-old/kernel/sched.c	2005-07-21 04:00:02.000000000 +1000
> +++ 400-workthreads.patch-new/kernel/sched.c	2005-07-21 04:00:19.000000000 +1000
> @@ -4580,10 +4580,10 @@ static int migration_call(struct notifie
>
>  	switch (action) {
>  	case CPU_UP_PREPARE:
> -		p = kthread_create(migration_thread, hcpu, "migration/%d",cpu);
> +		p = kthread_create(migration_thread, hcpu,
> +				"migration/%d",cpu);

This is unnecessary.

Overall, it looks pretty good.

Thanks,



	Pat
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/