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

[Nigel Cunningham <ncunningham@cyclades.com>]

Hi.

Sorry for the slow response. Busy still.

On Sat, 2005-08-06 at 15:06, Patrick Mochel wrote:
> On Fri, 5 Aug 2005, Nigel Cunningham wrote:
> 
> > Hi.
> >
> > I finally found some time to finish this off. I don't really like the
> > end result - the macros looked clearer to me - but here goes. If it
> > looks okay, I'll seek sign offs from each of the affected driver
> > maintainers and from Ingo. Anyone else?
> 
> What are your feelings about this: http://lwn.net/Articles/145417/ ?

I'm sure it could work, but I do worry a little about the possibilities
for exploits. It seems to me that if someone can get root, they an
insmod a module that could schedule any kind of work via any process.
Tracing that sort of security hole could be intractable. Christoph, is
that something you've considered/have thoughts on? Perhaps I'm just
being paranoid :>

> It seems like a cleaner way to do things. How would these patches change
> if that were merged?

We would still need some way to mark which threads to freeze, so we'd
still need the same sort of thing as far as kthread_run etc goes.

We'd also still mark unfreezable threads with NOFREEZE and not mark
other threads, so no change there.

The first substantial change I can see would be switching from
try_to_freeze to try_todo_list (could this be 'run_todo_list'? Try
implies failure is possible - if it is possible, why do we ignore
whether it fails?).

The second substantial change would be in the area of the refrigeration
code itself, ala Christoph's patch.

One more question regarding Christoph's patch - would it be worth moving
the refrigerator related code from sched.h to a separate file? We
already have so many things that depend on sched.h, and this will add
more. It will also make maintenance less painful because you won't have
to recompile everything that depends on sched.h when all you did was
modify (say) freezing().

> Concerning the patch specifically:
> 
> > diff -ruNp 400-workthreads.patch-old/include/linux/kthread.h 400-workthreads.patch-new/include/linux/kthread.h
> > --- 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-08-03 11:52:01.000000000 +1000
> > @@ -23,10 +23,20 @@
> >   *
> >   * Returns a task_struct or ERR_PTR(-ENOMEM).
> >   */
> > +struct task_struct *__kthread_create(int (*threadfn)(void *data),
> > +				   void *data,
> > +				   unsigned long freezer_flags,
> > +				   const char namefmt[],
> > +				   va_list * args);
> > +
> 
> When comparing this to this:
> 
> > diff -ruNp 400-workthreads.patch-old/include/linux/workqueue.h 400-workthreads.patch-new/include/linux/workqueue.h
> > --- 400-workthreads.patch-old/include/linux/workqueue.h	2005-06-20 11:47:30.000000000 +1000
> > +++ 400-workthreads.patch-new/include/linux/workqueue.h	2005-08-03 11:49:34.000000000 +1000
> > @@ -51,9 +51,12 @@ struct work_struct {
> >  	} while (0)
> >
> >  extern struct workqueue_struct *__create_workqueue(const char *name,
> > -						    int singlethread);
> > -#define create_workqueue(name) __create_workqueue((name), 0)
> > -#define create_singlethread_workqueue(name) __create_workqueue((name), 1)
> > +						    int singlethread,
> > +						    unsigned long freezer_flag);
> > +#define create_workqueue(name) __create_workqueue((name), 0, 0)
> > +#define create_nofreeze_workqueue(name) __create_workqueue((name), 0, PF_NOFREEZE)
> > +#define create_singlethread_workqueue(name) __create_workqueue((name), 1, 0)
> > +#define create_nofreeze_singlethread_workqueue(name) __create_workqueue((name), 1, PF_NOFREEZE)
> 
> And to this:
> 
> 
> >  static struct task_struct *create_workqueue_thread(struct workqueue_struct *wq,
> > -						   int cpu)
> > +						   int cpu,
> > +						   unsigned long freezer_flags)
> >  {
> 
> There is a slight discrepancy in the API changes. Obviously, we don't want
> to change every caller of create_workqueue() to support this. But, perhaps
> you could standardize the handling of the freezer flags (by e.g. creating
> a separate _nofreeze version for each).

Missed that one, sorry.

> Also, functions that take a va_list parameter pass the structure (array)
> rather than the pointer. See the definition of vprintk() in
> include/linux/kernel.h for an example.

Thanks for the pointer.

Nigel

> 
> Thanks,
> 
> 
> 	Pat
> 
> ______________________________________________________________________
> _______________________________________________
> linux-pm mailing list
> linux-pm@lists.osdl.org
> https://lists.osdl.org/mailman/listinfo/linux-pm
-- 
Evolution.
Enumerate the requirements.
Consider the interdependencies.
Calculate the probabilities.