Re: [RFC 09/16] kgr: mark task_safe in some kthreads

[Tejun Heo <tj@kernel.org>]

Hello, Jiri, Vojtech.

On Wed, May 14, 2014 at 05:15:01PM +0200, Vojtech Pavlik wrote:
> On Wed, May 14, 2014 at 04:59:05PM +0200, Jiri Slaby wrote:
> > I see the worst case scenario. (For curious readers, it is for example
> > this kthread body:
> > while (1) {
> >   some_paired_call(); /* invokes pre-patched code */
> >   if (kthread_should_stop()) { /* kgraft switches to the new code */
> >     its_paired_function(); /* invokes patched code (wrong) */
> >     break;
> >   }
> >   its_paired_function(); /* the same (wrong) */
> > })
> > 
> > What to do with that now? We have come up with a couple possibilities.
> > Would you consider try_to_freeze() a good state-defining function? As it
> > is called when a kthread expects weird things can happen, it should be
> > safe to switch to the patched version in our opinion.
> > 
> > The other possibility is to patch every kthread loop (~300) and insert
> > kgr_task_safe() semi-manually at some proper place.
> > 
> > Or if you have any other suggestions we would appreciate that?
> 
> A heretic idea would be to convert all kernel threads into functions
> that do not sleep and exit after a single iteration and are called from
> a central kthread main loop function. That would get all of

Or converting them to use workqueues instead.  Converting majority of
kthread users to workqueue is probably a good idea regardless of this
because workqueues are far easier to get right and give clear
delineation boundary between execution instances between which it's
safe to freeze and shutdown (and possibly to patch the work function).
Let alone overall lower overhead.  I converted some and was planning
on converting most of them but never got around ot it.

> kthread_should_stop() and try_to_freeze() and kgr_task_safe() nicely
> into one place and at the same time put enough constraint on what the
> thread function can do to prevent it from breaking the assumptions of
> each of these calls. 

Yeah, the exactly same rationales for using workqueue over kthreads.
That said, even with most kthread users converted to workqueue, we'd
probably want something which can really enforce correctness for the
leftovers as long as we continue to expose kthread interface.  Ooh,
there's also kthread_worker thing which puts workqueue-like semantics
on top of kthreads which can be used for whatever which can't be
converted to workqueue due to special worker attributes or whatnot.

So, yeah, I think there are enough tools available to put enough
semantic meanings over how kthreads are used such that things like
freezer or hot-code patching can be implemented in the generic
framework rather than in hundred scattered places but it's likely to
take a substantial amount of work.  The upside is that conversions are
likely beneficial on their own so they can be pushed separately.

Thanks.

-- 
tejun