lkml.org 
[lkml]   [2007]   [Apr]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 7/8] Clean up workqueue.c with respect to the freezer based cpu-hotplug
    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.

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

    \
     
     \ /
      Last update: 2007-04-03 18:03    [W:0.037 / U:91.204 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site