Re: [PATCH 7/8] Clean up workqueue.c with respect to the freezer based cpu-hotplug

[Oleg Nesterov <oleg@tv-sign.ru>]

On 04/03, Srivatsa Vaddagiri wrote:
>
> On Tue, Apr 03, 2007 at 03:47:29PM +0400, Oleg Nesterov wrote:
> > 
> > 		for (;;) {
> > 			try_to_freeze();
> > 
> > 			prepare_to_wait(&cwq->more_work, &wait, TASK_INTERRUPTIBLE);
> > 			if (!kthread_should_stop() && list_empty(&cwq->worklist))
> > 				schedule();
> > 			finish_wait(&cwq->more_work, &wait);
> > 
> > 			if (kthread_should_stop(cwq))
> > 				break;
> > 
> > 			run_workqueue(cwq);
> > 		}
> 
> cleanup_workqueue_thread (in Gautham's patches) does this:
> 
> 	thaw_process()
> 	kthread_stop()

Yes, thanks.

> We could do what you are suggesting if the thaw_process() part was
> integrated into kthread_stop() code [basically thaw_process after
> setting the kthread_stop_info.k flag].

I think it would be nice to do. I believe we can cleanup ksoftirqd()
and migration_thread() as well (kill wait_to_die: loop). Probably it
is better to introduce a new helper for that, kthread_thaw_stop() or
something.

> > >  void fastcall flush_workqueue(struct workqueue_struct *wq)
> > >  {
> > > -	const cpumask_t *cpu_map = wq_cpu_map(wq);
> > >  	int cpu;
> > >
> > >  	might_sleep();
> > > -	for_each_cpu_mask(cpu, *cpu_map)
> > > +	for_each_online_cpu(cpu)
> > >  		flush_cpu_workqueue(per_cpu_ptr(wq->cpu_wq, cpu));
> > >  }
> > 
> > Hm... I can't understand this change. I believe it is wrong.
> 
> Why?

What if is_single_threaded(wq) == true? In that case we should call
flush_cpu_workqueue(cpu) only if cpu == singlethread_cpu, otherwise
this is unneeded and wrong, because per_cpu_ptr(wq->cpu_wq, cpu) was
not initialized.

> > > +		kthread_stop(cwq->thread);
> > >  		cwq->thread = NULL;
> > > -		spin_unlock_wait(&cwq->lock);
> > >  	}
> > >  }
> > 
> > Deadlockable. Suppose that the freezing is in progress, cwq->thread is not
> > frozen yet. cleanup_workqueue_thread() calls thaw_process(cwq->thread),
> > then cwq->thread() goes to refrigerator, kthread_stop() blocks forever.
> 
> Good catch! Can cleanup_workqueue_thread take some mutex to serialize
> with freezer here (say freezer_mutex)?
> 
> Or better, since this seems to be a general problem for anyone who wants to do a
> kthread_stop, how abt modifying kthread_stop like below:
> 
> kthread_stop(p)
> {
> 	int old_exempt_flags;
> 
> 	task_lock(p);
> 	old_exempt_flags = p->flags;
> 	p->flags |= PFE_ALL;	/* Exempt 'p' from being frozen? */

I agree, we should mark this thread as non-freezable, but we can't modify
p->flags, this is racy. "current" owns its ->flags and it is not atomic.
Note that thaw_process() checks frozen(p) when it clears PF_FROZEN.

Actually, we should do this before destroy_workqueue() calls flush_workqueue().
Otherwise flush_cpu_workqueue() can hang forever in a similar manner.

Needs more thinking, I guess.

> > This means that the work_struct on single_threaded wq can't use any of
> > 
> > 	__create_workqueue()
> > 	destroy_workqueue()
> > 	flush_workqueue()
> > 	cancel_work_sync()
> 
> The workqueue_mutex() should serialize these with workqueue_cpu_callback() to 
> an extent, but  ..

No, no, workqueue_mutex can't help. Just for example: CPU_UP_PREPARE completes
and drops workqueue_mutex. __create_workqueue(wq) doesn't see the new cpu, it
is not on cpu_online_map, so it doesn't create cwq->thread. CPU_ONLINE oopses.

> Yes I agree, we should target freezing everybody here. It feels much
> safer that way!

Good!

Oleg.