Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Wed, Feb 21, 2007 at 05:30:10PM +0300, Oleg Nesterov wrote: > Agreed. Note that we don't need the new "del_work". It is always safe to > use cancel_work_sync() if we know that the workqueue is frozen, it won't > block. We can also do > > if (!cancel_delayed_work()) > cancel_work_sync(); > > but it is ok to do cancel_work_sync() unconditionally. Argh ..I should keep referring to recent sources. I didnt see cancel_work_sync() in my sources (2.6.20-rc4) and hence invented that del_work()! Anyway thanx for pointing out. This change will probably let us do CPU_DOWN_PREPARE after freeze_processes(). However I will keep my fingers crossed on whether it is really a good idea to send CPU_DOWN/UP_PREPARE after freeze_processes() until we get more review/testing results. -- Regards, vatsa - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Wed, Feb 21, 2007 at 05:30:10PM +0300, Oleg Nesterov wrote: > On 02/21, Srivatsa Vaddagiri wrote: > > > > On Tue, Feb 20, 2007 at 11:09:36PM +0300, Oleg Nesterov wrote: > > > > Which caller are you referring to here? Maybe we can decide on the > > > > option after we see the users of flush_workqueue() in DOWN_PREPARE. > > > > > > mm/slab.c:cpuup_callback() > > > > The cancel_rearming_delayed_work, if used as it is in cpuup_callback, > > will require that we send DOWN_PREPARE before freeze_processes(). > > > > But ..I am wondering if we can avoid doing cancel_rearming_delayed_work > > (and thus flush_workqueue) in CPU_DOWN_PREPARE of slab.c. Basically, > > > > mm/slab.c: > > > > CPU_DOWN_PREPARE: /* All processes frozen now */ > > cancel_delayed_work(_cpu(reap_work, cpu).timer); > > del_work(_cpu(reap_work, cpu).work); > > break; > > > > > > At the point of CPU_DOWN_PREPARE, keventd should be frozen and hence > > del_work() is a matter of just deleting the work from cwq->worklist. > > Agreed. Note that we don't need the new "del_work". It is always safe to > use cancel_work_sync() if we know that the workqueue is frozen, it won't > block. We can also do > > if (!cancel_delayed_work()) > cancel_work_sync(); > > but it is ok to do cancel_work_sync() unconditionally. True. But this might be a one off solution for slab. However, if someone in the future might require to do a flush_workqueue from CPU_DOWN_PREPARE context, we would need to find a new workaround. So, I'll try running CPU_DOWN_PREPARE and CPU_UP_PREPARE from a non frozen context to check if there are any potential problems. Hopfully there shouldn't be (m)any! > > Oleg. > thanks and regards gautham. -- Gautham R Shenoy Linux Technology Center IBM India. "Freedom comes with a price tag of responsibility, which is still a bargain, because Freedom is priceless!" - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On 02/21, Srivatsa Vaddagiri wrote: > > On Tue, Feb 20, 2007 at 11:09:36PM +0300, Oleg Nesterov wrote: > > > Which caller are you referring to here? Maybe we can decide on the > > > option after we see the users of flush_workqueue() in DOWN_PREPARE. > > > > mm/slab.c:cpuup_callback() > > The cancel_rearming_delayed_work, if used as it is in cpuup_callback, > will require that we send DOWN_PREPARE before freeze_processes(). > > But ..I am wondering if we can avoid doing cancel_rearming_delayed_work > (and thus flush_workqueue) in CPU_DOWN_PREPARE of slab.c. Basically, > > mm/slab.c: > > CPU_DOWN_PREPARE: /* All processes frozen now */ > cancel_delayed_work(_cpu(reap_work, cpu).timer); > del_work(_cpu(reap_work, cpu).work); > break; > > > At the point of CPU_DOWN_PREPARE, keventd should be frozen and hence > del_work() is a matter of just deleting the work from cwq->worklist. Agreed. Note that we don't need the new "del_work". It is always safe to use cancel_work_sync() if we know that the workqueue is frozen, it won't block. We can also do if (!cancel_delayed_work()) cancel_work_sync(); but it is ok to do cancel_work_sync() unconditionally. Oleg. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On 02/21, Srivatsa Vaddagiri wrote: On Tue, Feb 20, 2007 at 11:09:36PM +0300, Oleg Nesterov wrote: Which caller are you referring to here? Maybe we can decide on the option after we see the users of flush_workqueue() in DOWN_PREPARE. mm/slab.c:cpuup_callback() The cancel_rearming_delayed_work, if used as it is in cpuup_callback, will require that we send DOWN_PREPARE before freeze_processes(). But ..I am wondering if we can avoid doing cancel_rearming_delayed_work (and thus flush_workqueue) in CPU_DOWN_PREPARE of slab.c. Basically, mm/slab.c: CPU_DOWN_PREPARE: /* All processes frozen now */ cancel_delayed_work(per_cpu(reap_work, cpu).timer); del_work(per_cpu(reap_work, cpu).work); break; At the point of CPU_DOWN_PREPARE, keventd should be frozen and hence del_work() is a matter of just deleting the work from cwq-worklist. Agreed. Note that we don't need the new del_work. It is always safe to use cancel_work_sync() if we know that the workqueue is frozen, it won't block. We can also do if (!cancel_delayed_work()) cancel_work_sync(); but it is ok to do cancel_work_sync() unconditionally. Oleg. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Wed, Feb 21, 2007 at 05:30:10PM +0300, Oleg Nesterov wrote: On 02/21, Srivatsa Vaddagiri wrote: On Tue, Feb 20, 2007 at 11:09:36PM +0300, Oleg Nesterov wrote: Which caller are you referring to here? Maybe we can decide on the option after we see the users of flush_workqueue() in DOWN_PREPARE. mm/slab.c:cpuup_callback() The cancel_rearming_delayed_work, if used as it is in cpuup_callback, will require that we send DOWN_PREPARE before freeze_processes(). But ..I am wondering if we can avoid doing cancel_rearming_delayed_work (and thus flush_workqueue) in CPU_DOWN_PREPARE of slab.c. Basically, mm/slab.c: CPU_DOWN_PREPARE: /* All processes frozen now */ cancel_delayed_work(per_cpu(reap_work, cpu).timer); del_work(per_cpu(reap_work, cpu).work); break; At the point of CPU_DOWN_PREPARE, keventd should be frozen and hence del_work() is a matter of just deleting the work from cwq-worklist. Agreed. Note that we don't need the new del_work. It is always safe to use cancel_work_sync() if we know that the workqueue is frozen, it won't block. We can also do if (!cancel_delayed_work()) cancel_work_sync(); but it is ok to do cancel_work_sync() unconditionally. True. But this might be a one off solution for slab. However, if someone in the future might require to do a flush_workqueue from CPU_DOWN_PREPARE context, we would need to find a new workaround. So, I'll try running CPU_DOWN_PREPARE and CPU_UP_PREPARE from a non frozen context to check if there are any potential problems. Hopfully there shouldn't be (m)any! Oleg. thanks and regards gautham. -- Gautham R Shenoy Linux Technology Center IBM India. Freedom comes with a price tag of responsibility, which is still a bargain, because Freedom is priceless! - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Wed, Feb 21, 2007 at 05:30:10PM +0300, Oleg Nesterov wrote: Agreed. Note that we don't need the new del_work. It is always safe to use cancel_work_sync() if we know that the workqueue is frozen, it won't block. We can also do if (!cancel_delayed_work()) cancel_work_sync(); but it is ok to do cancel_work_sync() unconditionally. Argh ..I should keep referring to recent sources. I didnt see cancel_work_sync() in my sources (2.6.20-rc4) and hence invented that del_work()! Anyway thanx for pointing out. This change will probably let us do CPU_DOWN_PREPARE after freeze_processes(). However I will keep my fingers crossed on whether it is really a good idea to send CPU_DOWN/UP_PREPARE after freeze_processes() until we get more review/testing results. -- Regards, vatsa - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Tue, Feb 20, 2007 at 11:09:36PM +0300, Oleg Nesterov wrote: > > Which caller are you referring to here? Maybe we can decide on the > > option after we see the users of flush_workqueue() in DOWN_PREPARE. > > mm/slab.c:cpuup_callback() The cancel_rearming_delayed_work, if used as it is in cpuup_callback, will require that we send DOWN_PREPARE before freeze_processes(). But ..I am wondering if we can avoid doing cancel_rearming_delayed_work (and thus flush_workqueue) in CPU_DOWN_PREPARE of slab.c. Basically, mm/slab.c: CPU_DOWN_PREPARE: /* All processes frozen now */ cancel_delayed_work(_cpu(reap_work, cpu).timer); del_work(_cpu(reap_work, cpu).work); break; At the point of CPU_DOWN_PREPARE, keventd should be frozen and hence del_work() is a matter of just deleting the work from cwq->worklist. -- Regards, vatsa - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On 02/20, Srivatsa Vaddagiri wrote: > > On Sun, Feb 18, 2007 at 12:59:28AM +0300, Oleg Nesterov wrote: > > Before you begin. You are doing CPU_DOWN_PREPARE after freeze_processes(). > > Not good. This makes impossible to do flush_workueue() at CPU_DOWN_PREPARE > > stage, we have callers. > > We have few solutions to deal with this: > > a. Mark such workqueues not freezable for hotplug > b. If above is not possible, don't call flush_workqueue in DOWN_PREPARE > c. If above is not possible, send DOWN_PREPARE before freeze_processes() > > I would prefer a solution in the above order listed. > > Which caller are you referring to here? Maybe we can decide on the > option after we see the users of flush_workqueue() in DOWN_PREPARE. mm/slab.c:cpuup_callback() > > I'm afraid it won't be so easy to solve all locking/racing problems. Will > > wait for the patch :) > > I dont see problems for workqueue.c even if we follow option c. Do you > see any? I don't right now... Oh, I already forgot everything :) I'll try to recall when I see the patch. Oleg. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Sun, Feb 18, 2007 at 12:59:28AM +0300, Oleg Nesterov wrote: > Before you begin. You are doing CPU_DOWN_PREPARE after freeze_processes(). > Not good. This makes impossible to do flush_workueue() at CPU_DOWN_PREPARE > stage, we have callers. We have few solutions to deal with this: a. Mark such workqueues not freezable for hotplug b. If above is not possible, don't call flush_workqueue in DOWN_PREPARE c. If above is not possible, send DOWN_PREPARE before freeze_processes() I would prefer a solution in the above order listed. Which caller are you referring to here? Maybe we can decide on the option after we see the users of flush_workqueue() in DOWN_PREPARE. > I'm afraid it won't be so easy to solve all locking/racing problems. Will > wait for the patch :) I dont see problems for workqueue.c even if we follow option c. Do you see any? -- Regards, vatsa - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Sun, Feb 18, 2007 at 12:59:28AM +0300, Oleg Nesterov wrote: Before you begin. You are doing CPU_DOWN_PREPARE after freeze_processes(). Not good. This makes impossible to do flush_workueue() at CPU_DOWN_PREPARE stage, we have callers. We have few solutions to deal with this: a. Mark such workqueues not freezable for hotplug b. If above is not possible, don't call flush_workqueue in DOWN_PREPARE c. If above is not possible, send DOWN_PREPARE before freeze_processes() I would prefer a solution in the above order listed. Which caller are you referring to here? Maybe we can decide on the option after we see the users of flush_workqueue() in DOWN_PREPARE. I'm afraid it won't be so easy to solve all locking/racing problems. Will wait for the patch :) I dont see problems for workqueue.c even if we follow option c. Do you see any? -- Regards, vatsa - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On 02/20, Srivatsa Vaddagiri wrote: On Sun, Feb 18, 2007 at 12:59:28AM +0300, Oleg Nesterov wrote: Before you begin. You are doing CPU_DOWN_PREPARE after freeze_processes(). Not good. This makes impossible to do flush_workueue() at CPU_DOWN_PREPARE stage, we have callers. We have few solutions to deal with this: a. Mark such workqueues not freezable for hotplug b. If above is not possible, don't call flush_workqueue in DOWN_PREPARE c. If above is not possible, send DOWN_PREPARE before freeze_processes() I would prefer a solution in the above order listed. Which caller are you referring to here? Maybe we can decide on the option after we see the users of flush_workqueue() in DOWN_PREPARE. mm/slab.c:cpuup_callback() I'm afraid it won't be so easy to solve all locking/racing problems. Will wait for the patch :) I dont see problems for workqueue.c even if we follow option c. Do you see any? I don't right now... Oh, I already forgot everything :) I'll try to recall when I see the patch. Oleg. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Tue, Feb 20, 2007 at 11:09:36PM +0300, Oleg Nesterov wrote: Which caller are you referring to here? Maybe we can decide on the option after we see the users of flush_workqueue() in DOWN_PREPARE. mm/slab.c:cpuup_callback() The cancel_rearming_delayed_work, if used as it is in cpuup_callback, will require that we send DOWN_PREPARE before freeze_processes(). But ..I am wondering if we can avoid doing cancel_rearming_delayed_work (and thus flush_workqueue) in CPU_DOWN_PREPARE of slab.c. Basically, mm/slab.c: CPU_DOWN_PREPARE: /* All processes frozen now */ cancel_delayed_work(per_cpu(reap_work, cpu).timer); del_work(per_cpu(reap_work, cpu).work); break; At the point of CPU_DOWN_PREPARE, keventd should be frozen and hence del_work() is a matter of just deleting the work from cwq-worklist. -- Regards, vatsa - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On 02/17, Srivatsa Vaddagiri wrote: > > Yeah, thats what I thought. We will try to split it to the extent > possible in the next iteration. Before you begin. You are doing CPU_DOWN_PREPARE after freeze_processes(). Not good. This makes impossible to do flush_workueue() at CPU_DOWN_PREPARE stage, we have callers. I'm afraid it won't be so easy to solve all locking/racing problems. Will wait for the patch :) Oleg. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On 02/17, Srivatsa Vaddagiri wrote: Yeah, thats what I thought. We will try to split it to the extent possible in the next iteration. Before you begin. You are doing CPU_DOWN_PREPARE after freeze_processes(). Not good. This makes impossible to do flush_workueue() at CPU_DOWN_PREPARE stage, we have callers. I'm afraid it won't be so easy to solve all locking/racing problems. Will wait for the patch :) Oleg. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Sat, Feb 17, 2007 at 02:59:39AM +0300, Oleg Nesterov wrote: > In that case (all processes are frozen when workqueue_cpu_callback() > calls cleanup_workqueue_thread()) I agree, it is better to just use > kthread_stop/kthread_should_stop. Great, thanks! > This also means that probably it won't be convenient to do this change > "by small steps" as I suggested before. Yeah, thats what I thought. We will try to split it to the extent possible in the next iteration. Thanks for your feedback/review! -- Regards, vatsa - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On 02/16, Srivatsa Vaddagiri wrote: > > Note with the change proposed in refrigerator, we can avoid > CPU_DEAD_KILL_THREADS and do all cleanup in CPU_DEAD itself. In that case (all processes are frozen when workqueue_cpu_callback() calls cleanup_workqueue_thread()) I agree, it is better to just use kthread_stop/kthread_should_stop. This also means that probably it won't be convenient to do this change "by small steps" as I suggested before. Oleg. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On 02/16, Srivatsa Vaddagiri wrote:
>
> 2.6.20-mm1 (cwq->should_stop)
> =
>
> static void cleanup_workqueue_thread(struct cpu_workqueue_struct *cwq, int
> cpu)
> {
> struct wq_barrier barr;
> int alive = 0;
>
> spin_lock_irq(>lock);
> if (cwq->thread != NULL) {
> insert_wq_barrier(cwq, , 1);
> cwq->should_stop = 1;
> alive = 1;
> }
> spin_unlock_irq(>lock);
>
> if (alive) {
> wait_for_completion();
>
> while (unlikely(cwq->thread != NULL))
> cpu_relax();
> /*
> * Wait until cwq->thread unlocks cwq->lock,
> * it won't touch *cwq after that.
> */
> smp_rmb();
> spin_unlock_wait(>lock);
> }
> }
>
> Patch (based on kthread_should_stop)
>
>
> static void cleanup_workqueue_thread(struct workqueue_struct *wq, int cpu)
> {
> struct cpu_workqueue_struct *cwq = per_cpu_ptr(wq->cpu_wq, cpu);
>
> if (cwq->thread != NULL) {
> kthread_stop(cwq->thread);
> cwq->thread = NULL;
> }
> }
>
> > No more changes are required, cwq_should_stop() just works
> > because it is more flexible than kthread_should_stop().
>
> What is more flexible abt cwq_should_stop()?
- it doesn't use a global semaphore
- it works with or without freezer
- it works with or without take_over_work()
- it doesn't require that we have no pending works when
cleanup_workqueue_thread() is called.
- worker_thread() doesn't need to have 2 different conditions
to exit in 2 different ways.
- it allows us to do further improvements (don't take workqueue
mutex for the whole cpu-hotplug event), but this needs more work
and probably is not valid any longer if we use freezer.
Ok. This is a matter of taste. I will not argue if you send a patch
to convert the code to use kthread_stop() again (if it is correct :),
but let it be a separate change, please.
Oleg.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Fri, Feb 16, 2007 at 06:33:21PM +0300, Oleg Nesterov wrote:
> I take my words back. It is not "ugly" any longer because with this change
> we don't do kthread_stop()->wakeup_process() while cwq->thread may sleep in
> work->func(). Still I don't see (ok, I am biased and probably wrong, please
> correct me) why kthread_stop+wait_to_die is better than cwq_should_stop(),
> see below.
I just like using existing code (kthread_stop) as much as possible and not add
new code (->should_stop). Also the 'while (cwq->thread != NULL)' loop in
cleanup_workqueue_thread is not neat IMHO, compared to kthread_stop+wait_to_die.
Pls compare cleanup_workqueue_thread() in 2.6.20-mm1 and what is
proposed in the patch :-
2.6.20-mm1 (cwq->should_stop)
=
static void cleanup_workqueue_thread(struct cpu_workqueue_struct *cwq, int cpu)
{
struct wq_barrier barr;
int alive = 0;
spin_lock_irq(>lock);
if (cwq->thread != NULL) {
insert_wq_barrier(cwq, , 1);
cwq->should_stop = 1;
alive = 1;
}
spin_unlock_irq(>lock);
if (alive) {
wait_for_completion();
while (unlikely(cwq->thread != NULL))
cpu_relax();
/*
* Wait until cwq->thread unlocks cwq->lock,
* it won't touch *cwq after that.
*/
smp_rmb();
spin_unlock_wait(>lock);
}
}
Patch (based on kthread_should_stop)
static void cleanup_workqueue_thread(struct workqueue_struct *wq, int cpu)
{
struct cpu_workqueue_struct *cwq = per_cpu_ptr(wq->cpu_wq, cpu);
if (cwq->thread != NULL) {
kthread_stop(cwq->thread);
cwq->thread = NULL;
}
}
The version using kthread_should_stop() is more simple IMO.
> > I feel it is nice if the cleanup is synchronous i.e when cpu_down() is
> > complete, all the dead cpu's worker threads would have terminated.
> > Otherwise we expose races between CPU_UP_PREPARE/kthread_create and the
> > (old) thread exiting.
>
> Please look at 2.6.20-mm1, cleanup is synchronous. Probably we misunderstood
> each other looking at different code.
Ok ..I hadnt looked at 2.6.20-mm1 (it wasnt out when we posted the
patch). Neverthless I think most of our intended changes would apply for
2.6.20-mm1 also. We will post a new version (breaking down workqueue changes
as you want) against 2.6.20-mm1.
> > How abt retaining the break above but setting cwq->thread = NULL in
> > create_workqueue_thread in failure case?
>
> Perhaps do it, but why? The failure should be rare, and it is a bit
> dangerous to have workqueue_struct which was not properly initialized.
> Suppose we change CPU_UP_PREPARE so it is called before freeze_processes()
> stage, then we have a problem.
Ok ..no problem. Will not add the 'break' there!
> Srivatsa, don't get we wrong. I can't judge about using freezer for cpu
> hotplug,
> but yes, we can improve workqueue.c in this case! But this changes should be
> small and understandable. When cpu hotplug is converted, we don't need _any_
> changes in workqueue.c, it should work (except
> s/CPU_DEAD/CPU_DEAD_KILL_THREADS
> if you insist).
Note with the change proposed in refrigerator, we can avoid
CPU_DEAD_KILL_THREADS and do all cleanup in CPU_DEAD itself.
> No more changes are required, cwq_should_stop() just works
> because it is more flexible than kthread_should_stop().
What is more flexible abt cwq_should_stop()?
--
Regards,
vatsa
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On 02/16, Srivatsa Vaddagiri wrote: > > On Wed, Feb 14, 2007 at 11:09:04PM +0300, Oleg Nesterov wrote: > > What else you don't like? Why do you want to remove cwq_should_stop() and > > restore an ugly (ugly for workqueue.c) kthread_stop/kthread_should_stop() ? > > What is ugly abt kthread_stop in workqueue.c? I take my words back. It is not "ugly" any longer because with this change we don't do kthread_stop()->wakeup_process() while cwq->thread may sleep in work->func(). Still I don't see (ok, I am biased and probably wrong, please correct me) why kthread_stop+wait_to_die is better than cwq_should_stop(), see below. > I feel it is nice if the cleanup is synchronous i.e when cpu_down() is > complete, all the dead cpu's worker threads would have terminated. > Otherwise we expose races between CPU_UP_PREPARE/kthread_create and the > (old) thread exiting. Please look at 2.6.20-mm1, cleanup is synchronous. Probably we misunderstood each other looking at different code. > > > + if (err) > > > + break; > > > > No, we can't break. We are going to execute destroy_workqueue(), it will > > iterate over all cwqs. > > and try to kthread_stop() uninitialized cwq->thread? > > How abt retaining the break above but setting cwq->thread = NULL in > create_workqueue_thread in failure case? Perhaps do it, but why? The failure should be rare, and it is a bit dangerous to have workqueue_struct which was not properly initialized. Suppose we change CPU_UP_PREPARE so it is called before freeze_processes() stage, then we have a problem. > > > +static void take_over_work(struct workqueue_struct *wq, unsigned int cpu) > > > > I think this is unneeded complication, but ok, should work. > > This is required if we want to stop per-cpu threads synchronously. See above. Srivatsa, don't get we wrong. I can't judge about using freezer for cpu hotplug, but yes, we can improve workqueue.c in this case! But this changes should be small and understandable. When cpu hotplug is converted, we don't need _any_ changes in workqueue.c, it should work (except s/CPU_DEAD/CPU_DEAD_KILL_THREADS if you insist). Then, [PATCH] make all multithread workqueus freezable [PATCH] remove cpu_populated_map just remove, very easy. Good change! [PATCH] restore take_over_works() This is not strictly needed! But ok, this can speedup cpu_down, and now we can't have a race with flush_worqueue (freezer). Just do case CPU_XXX: // all tasks are frozen + take_over_work(wq, hotcpu); No more changes are required, cwq_should_stop() just works because it is more flexible than kthread_should_stop(). [PATCH] don't take workqueue_mutex in ... [PATCH] ... Probably I missed something, and we should fix/improve/drop cwq_should_stop(), but please do this in a separate patch, with a proper changelog which explains why we are doing so. Currently I believe that workqueue.c will need very minimal and simple changes if we use freezer. Oleg. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On 02/16, Srivatsa Vaddagiri wrote: On Wed, Feb 14, 2007 at 11:09:04PM +0300, Oleg Nesterov wrote: What else you don't like? Why do you want to remove cwq_should_stop() and restore an ugly (ugly for workqueue.c) kthread_stop/kthread_should_stop() ? What is ugly abt kthread_stop in workqueue.c? I take my words back. It is not ugly any longer because with this change we don't do kthread_stop()-wakeup_process() while cwq-thread may sleep in work-func(). Still I don't see (ok, I am biased and probably wrong, please correct me) why kthread_stop+wait_to_die is better than cwq_should_stop(), see below. I feel it is nice if the cleanup is synchronous i.e when cpu_down() is complete, all the dead cpu's worker threads would have terminated. Otherwise we expose races between CPU_UP_PREPARE/kthread_create and the (old) thread exiting. Please look at 2.6.20-mm1, cleanup is synchronous. Probably we misunderstood each other looking at different code. + if (err) + break; No, we can't break. We are going to execute destroy_workqueue(), it will iterate over all cwqs. and try to kthread_stop() uninitialized cwq-thread? How abt retaining the break above but setting cwq-thread = NULL in create_workqueue_thread in failure case? Perhaps do it, but why? The failure should be rare, and it is a bit dangerous to have workqueue_struct which was not properly initialized. Suppose we change CPU_UP_PREPARE so it is called before freeze_processes() stage, then we have a problem. +static void take_over_work(struct workqueue_struct *wq, unsigned int cpu) I think this is unneeded complication, but ok, should work. This is required if we want to stop per-cpu threads synchronously. See above. Srivatsa, don't get we wrong. I can't judge about using freezer for cpu hotplug, but yes, we can improve workqueue.c in this case! But this changes should be small and understandable. When cpu hotplug is converted, we don't need _any_ changes in workqueue.c, it should work (except s/CPU_DEAD/CPU_DEAD_KILL_THREADS if you insist). Then, [PATCH] make all multithread workqueus freezable [PATCH] remove cpu_populated_map just remove, very easy. Good change! [PATCH] restore take_over_works() This is not strictly needed! But ok, this can speedup cpu_down, and now we can't have a race with flush_worqueue (freezer). Just do case CPU_XXX: // all tasks are frozen + take_over_work(wq, hotcpu); No more changes are required, cwq_should_stop() just works because it is more flexible than kthread_should_stop(). [PATCH] don't take workqueue_mutex in ... [PATCH] ... Probably I missed something, and we should fix/improve/drop cwq_should_stop(), but please do this in a separate patch, with a proper changelog which explains why we are doing so. Currently I believe that workqueue.c will need very minimal and simple changes if we use freezer. Oleg. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Fri, Feb 16, 2007 at 06:33:21PM +0300, Oleg Nesterov wrote:
I take my words back. It is not ugly any longer because with this change
we don't do kthread_stop()-wakeup_process() while cwq-thread may sleep in
work-func(). Still I don't see (ok, I am biased and probably wrong, please
correct me) why kthread_stop+wait_to_die is better than cwq_should_stop(),
see below.
I just like using existing code (kthread_stop) as much as possible and not add
new code (-should_stop). Also the 'while (cwq-thread != NULL)' loop in
cleanup_workqueue_thread is not neat IMHO, compared to kthread_stop+wait_to_die.
Pls compare cleanup_workqueue_thread() in 2.6.20-mm1 and what is
proposed in the patch :-
2.6.20-mm1 (cwq-should_stop)
=
static void cleanup_workqueue_thread(struct cpu_workqueue_struct *cwq, int cpu)
{
struct wq_barrier barr;
int alive = 0;
spin_lock_irq(cwq-lock);
if (cwq-thread != NULL) {
insert_wq_barrier(cwq, barr, 1);
cwq-should_stop = 1;
alive = 1;
}
spin_unlock_irq(cwq-lock);
if (alive) {
wait_for_completion(barr.done);
while (unlikely(cwq-thread != NULL))
cpu_relax();
/*
* Wait until cwq-thread unlocks cwq-lock,
* it won't touch *cwq after that.
*/
smp_rmb();
spin_unlock_wait(cwq-lock);
}
}
Patch (based on kthread_should_stop)
static void cleanup_workqueue_thread(struct workqueue_struct *wq, int cpu)
{
struct cpu_workqueue_struct *cwq = per_cpu_ptr(wq-cpu_wq, cpu);
if (cwq-thread != NULL) {
kthread_stop(cwq-thread);
cwq-thread = NULL;
}
}
The version using kthread_should_stop() is more simple IMO.
I feel it is nice if the cleanup is synchronous i.e when cpu_down() is
complete, all the dead cpu's worker threads would have terminated.
Otherwise we expose races between CPU_UP_PREPARE/kthread_create and the
(old) thread exiting.
Please look at 2.6.20-mm1, cleanup is synchronous. Probably we misunderstood
each other looking at different code.
Ok ..I hadnt looked at 2.6.20-mm1 (it wasnt out when we posted the
patch). Neverthless I think most of our intended changes would apply for
2.6.20-mm1 also. We will post a new version (breaking down workqueue changes
as you want) against 2.6.20-mm1.
How abt retaining the break above but setting cwq-thread = NULL in
create_workqueue_thread in failure case?
Perhaps do it, but why? The failure should be rare, and it is a bit
dangerous to have workqueue_struct which was not properly initialized.
Suppose we change CPU_UP_PREPARE so it is called before freeze_processes()
stage, then we have a problem.
Ok ..no problem. Will not add the 'break' there!
Srivatsa, don't get we wrong. I can't judge about using freezer for cpu
hotplug,
but yes, we can improve workqueue.c in this case! But this changes should be
small and understandable. When cpu hotplug is converted, we don't need _any_
changes in workqueue.c, it should work (except
s/CPU_DEAD/CPU_DEAD_KILL_THREADS
if you insist).
Note with the change proposed in refrigerator, we can avoid
CPU_DEAD_KILL_THREADS and do all cleanup in CPU_DEAD itself.
No more changes are required, cwq_should_stop() just works
because it is more flexible than kthread_should_stop().
What is more flexible abt cwq_should_stop()?
--
Regards,
vatsa
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On 02/16, Srivatsa Vaddagiri wrote:
2.6.20-mm1 (cwq-should_stop)
=
static void cleanup_workqueue_thread(struct cpu_workqueue_struct *cwq, int
cpu)
{
struct wq_barrier barr;
int alive = 0;
spin_lock_irq(cwq-lock);
if (cwq-thread != NULL) {
insert_wq_barrier(cwq, barr, 1);
cwq-should_stop = 1;
alive = 1;
}
spin_unlock_irq(cwq-lock);
if (alive) {
wait_for_completion(barr.done);
while (unlikely(cwq-thread != NULL))
cpu_relax();
/*
* Wait until cwq-thread unlocks cwq-lock,
* it won't touch *cwq after that.
*/
smp_rmb();
spin_unlock_wait(cwq-lock);
}
}
Patch (based on kthread_should_stop)
static void cleanup_workqueue_thread(struct workqueue_struct *wq, int cpu)
{
struct cpu_workqueue_struct *cwq = per_cpu_ptr(wq-cpu_wq, cpu);
if (cwq-thread != NULL) {
kthread_stop(cwq-thread);
cwq-thread = NULL;
}
}
No more changes are required, cwq_should_stop() just works
because it is more flexible than kthread_should_stop().
What is more flexible abt cwq_should_stop()?
- it doesn't use a global semaphore
- it works with or without freezer
- it works with or without take_over_work()
- it doesn't require that we have no pending works when
cleanup_workqueue_thread() is called.
- worker_thread() doesn't need to have 2 different conditions
to exit in 2 different ways.
- it allows us to do further improvements (don't take workqueue
mutex for the whole cpu-hotplug event), but this needs more work
and probably is not valid any longer if we use freezer.
Ok. This is a matter of taste. I will not argue if you send a patch
to convert the code to use kthread_stop() again (if it is correct :),
but let it be a separate change, please.
Oleg.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On 02/16, Srivatsa Vaddagiri wrote: Note with the change proposed in refrigerator, we can avoid CPU_DEAD_KILL_THREADS and do all cleanup in CPU_DEAD itself. In that case (all processes are frozen when workqueue_cpu_callback() calls cleanup_workqueue_thread()) I agree, it is better to just use kthread_stop/kthread_should_stop. This also means that probably it won't be convenient to do this change by small steps as I suggested before. Oleg. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Sat, Feb 17, 2007 at 02:59:39AM +0300, Oleg Nesterov wrote: In that case (all processes are frozen when workqueue_cpu_callback() calls cleanup_workqueue_thread()) I agree, it is better to just use kthread_stop/kthread_should_stop. Great, thanks! This also means that probably it won't be convenient to do this change by small steps as I suggested before. Yeah, thats what I thought. We will try to split it to the extent possible in the next iteration. Thanks for your feedback/review! -- Regards, vatsa - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Wed, Feb 14, 2007 at 11:09:04PM +0300, Oleg Nesterov wrote:
> What else you don't like? Why do you want to remove cwq_should_stop() and
> restore an ugly (ugly for workqueue.c) kthread_stop/kthread_should_stop() ?
What is ugly abt kthread_stop in workqueue.c?
I feel it is nice if the cleanup is synchronous i.e when cpu_down() is
complete, all the dead cpu's worker threads would have terminated.
Otherwise we expose races between CPU_UP_PREPARE/kthread_create and the
(old) thread exiting.
> We can restore take_over_works(), although I don't see why this is needed.
> But cwq_should_stop() will just work regardless, why do you want to add
> this "wait_to_die" ... well, hack :)
wait_to_die is not a new "hack"! Its already used in several other
places ..
> > -static DEFINE_MUTEX(workqueue_mutex);
> > +static DEFINE_SPINLOCK(workqueue_lock);
>
> No. We can't do this. see below.
Ok ..
> > struct workqueue_struct *__create_workqueue(const char *name,
> > int singlethread, int freezeable)
> > {
> > @@ -798,17 +756,20 @@ struct workqueue_struct *__create_workqu
> > INIT_LIST_HEAD(>list);
> > cwq = init_cpu_workqueue(wq, singlethread_cpu);
> > err = create_workqueue_thread(cwq, singlethread_cpu);
> > + if (!err)
> > + wake_up_process(cwq->thread);
> > } else {
> > - mutex_lock(_mutex);
> > + spin_lock(_lock);
> > list_add(>list, );
> > -
> > - for_each_possible_cpu(cpu) {
> > + spin_unlock(_lock);
> > + for_each_online_cpu(cpu) {
> > cwq = init_cpu_workqueue(wq, cpu);
> > - if (err || !(cpu_online(cpu) || cpu == embryonic_cpu))
> > - continue;
> > err = create_workqueue_thread(cwq, cpu);
> > + if (err)
> > + break;
>
> No, we can't break. We are going to execute destroy_workqueue(), it will
> iterate over all cwqs.
and try to kthread_stop() uninitialized cwq->thread?
How abt retaining the break above but setting cwq->thread = NULL in
create_workqueue_thread in failure case?
> > +static void take_over_work(struct workqueue_struct *wq, unsigned int cpu)
> > +{
> > +}
>
> I think this is unneeded complication, but ok, should work.
This is required if we want to stop per-cpu threads synchronously.
> > + case CPU_DEAD:
> > + list_for_each_entry(wq, , list)
> > + take_over_work(wq, hotcpu);
> > + break;
> > +
> > + case CPU_DEAD_KILL_THREADS:
> > + list_for_each_entry(wq, , list)
> > + cleanup_workqueue_thread(wq, hotcpu);
> > }
>
> Both CPU_UP_CANCELED and CPU_DEAD_KILL_THREADS runs after thaw_processes(),
> this means that workqueue_cpu_callback() is racy wrt create/destroy workqueue,
> we should take the mutex, and it can't be spinlock_t.
Ok yes ..thanks for pointing out!
--
Regards,
vatsa
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Wed, Feb 14, 2007 at 11:09:04PM +0300, Oleg Nesterov wrote:
What else you don't like? Why do you want to remove cwq_should_stop() and
restore an ugly (ugly for workqueue.c) kthread_stop/kthread_should_stop() ?
What is ugly abt kthread_stop in workqueue.c?
I feel it is nice if the cleanup is synchronous i.e when cpu_down() is
complete, all the dead cpu's worker threads would have terminated.
Otherwise we expose races between CPU_UP_PREPARE/kthread_create and the
(old) thread exiting.
We can restore take_over_works(), although I don't see why this is needed.
But cwq_should_stop() will just work regardless, why do you want to add
this wait_to_die ... well, hack :)
wait_to_die is not a new hack! Its already used in several other
places ..
-static DEFINE_MUTEX(workqueue_mutex);
+static DEFINE_SPINLOCK(workqueue_lock);
No. We can't do this. see below.
Ok ..
struct workqueue_struct *__create_workqueue(const char *name,
int singlethread, int freezeable)
{
@@ -798,17 +756,20 @@ struct workqueue_struct *__create_workqu
INIT_LIST_HEAD(wq-list);
cwq = init_cpu_workqueue(wq, singlethread_cpu);
err = create_workqueue_thread(cwq, singlethread_cpu);
+ if (!err)
+ wake_up_process(cwq-thread);
} else {
- mutex_lock(workqueue_mutex);
+ spin_lock(workqueue_lock);
list_add(wq-list, workqueues);
-
- for_each_possible_cpu(cpu) {
+ spin_unlock(workqueue_lock);
+ for_each_online_cpu(cpu) {
cwq = init_cpu_workqueue(wq, cpu);
- if (err || !(cpu_online(cpu) || cpu == embryonic_cpu))
- continue;
err = create_workqueue_thread(cwq, cpu);
+ if (err)
+ break;
No, we can't break. We are going to execute destroy_workqueue(), it will
iterate over all cwqs.
and try to kthread_stop() uninitialized cwq-thread?
How abt retaining the break above but setting cwq-thread = NULL in
create_workqueue_thread in failure case?
+static void take_over_work(struct workqueue_struct *wq, unsigned int cpu)
+{
snip
+}
I think this is unneeded complication, but ok, should work.
This is required if we want to stop per-cpu threads synchronously.
+ case CPU_DEAD:
+ list_for_each_entry(wq, workqueues, list)
+ take_over_work(wq, hotcpu);
+ break;
+
+ case CPU_DEAD_KILL_THREADS:
+ list_for_each_entry(wq, workqueues, list)
+ cleanup_workqueue_thread(wq, hotcpu);
}
Both CPU_UP_CANCELED and CPU_DEAD_KILL_THREADS runs after thaw_processes(),
this means that workqueue_cpu_callback() is racy wrt create/destroy workqueue,
we should take the mutex, and it can't be spinlock_t.
Ok yes ..thanks for pointing out!
--
Regards,
vatsa
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On 02/14, Srivatsa Vaddagiri wrote:
>
> On Wed, Feb 14, 2007 at 08:13:05PM +0530, Gautham R Shenoy wrote:
> > + switch (action) {
> > + case CPU_UP_PREPARE:
> > + /* Create a new workqueue thread for it. */
> > + list_for_each_entry(wq, , list) {
>
> Its probably safe to take the workqueue (spin) lock here (and other
> notifiers as well), before traversing the list.
We can't fork() under spin lock.
>
> > + cwq = per_cpu_ptr(wq->cpu_wq, hotcpu);
> > + if (create_workqueue_thread(cwq, hotcpu)) {
> > + printk("workqueue for %i failed\n", hotcpu);
> > + return NOTIFY_BAD;
> > + }
> > + }
> > + break;
Oleg.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On 02/14, Gautham R Shenoy wrote:
>
> This patch reverts all the recent workqueue hacks added to make it
> hotplug safe.
In my opinion these hacks are cleanups :)
Ok. If we use freezer then yes, we can remove cpu_populated_map and just
use for_each_online_cpu(). This is easy and good.
What else you don't like? Why do you want to remove cwq_should_stop() and
restore an ugly (ugly for workqueue.c) kthread_stop/kthread_should_stop() ?
We can restore take_over_works(), although I don't see why this is needed.
But cwq_should_stop() will just work regardless, why do you want to add
this "wait_to_die" ... well, hack :)
> -static DEFINE_MUTEX(workqueue_mutex);
> +static DEFINE_SPINLOCK(workqueue_lock);
No. We can't do this. see below.
> struct workqueue_struct *__create_workqueue(const char *name,
> int singlethread, int freezeable)
> {
> @@ -798,17 +756,20 @@ struct workqueue_struct *__create_workqu
> INIT_LIST_HEAD(>list);
> cwq = init_cpu_workqueue(wq, singlethread_cpu);
> err = create_workqueue_thread(cwq, singlethread_cpu);
> + if (!err)
> + wake_up_process(cwq->thread);
> } else {
> - mutex_lock(_mutex);
> + spin_lock(_lock);
> list_add(>list, );
> -
> - for_each_possible_cpu(cpu) {
> + spin_unlock(_lock);
> + for_each_online_cpu(cpu) {
> cwq = init_cpu_workqueue(wq, cpu);
> - if (err || !(cpu_online(cpu) || cpu == embryonic_cpu))
> - continue;
> err = create_workqueue_thread(cwq, cpu);
> + if (err)
> + break;
No, we can't break. We are going to execute destroy_workqueue(), it will
iterate over all cwqs.
> +static void take_over_work(struct workqueue_struct *wq, unsigned int cpu)
> +{
> + struct cpu_workqueue_struct *cwq = per_cpu_ptr(wq->cpu_wq, cpu);
> + struct list_head list;
> + struct work_struct *work;
> +
> + spin_lock_irq(>lock);
> + list_replace_init(>worklist, );
> +
> + while (!list_empty()) {
> + work = list_entry(list.next,struct work_struct,entry);
> + list_del(>entry);
> + __queue_work(per_cpu_ptr(wq->cpu_wq, smp_processor_id()), work);
> + }
> +
> + spin_unlock_irq(>lock);
> +}
I think this is unneeded complication, but ok, should work.
> static int __devinit workqueue_cpu_callback(struct notifier_block *nfb,
> unsigned long action,
> void *hcpu)
> + case CPU_UP_CANCELED:
> + list_for_each_entry(wq, , list) {
> + if (!per_cpu_ptr(wq->cpu_wq, hotcpu)->thread)
> + continue;
> + /* Unbind so it can run. */
> + kthread_bind(per_cpu_ptr(wq->cpu_wq, hotcpu)->thread,
> + any_online_cpu(cpu_online_map));
> + cleanup_workqueue_thread(wq, hotcpu);
> }
> + break;
> +
> + case CPU_DEAD:
> + list_for_each_entry(wq, , list)
> + take_over_work(wq, hotcpu);
> + break;
> +
> + case CPU_DEAD_KILL_THREADS:
> + list_for_each_entry(wq, , list)
> + cleanup_workqueue_thread(wq, hotcpu);
> }
Both CPU_UP_CANCELED and CPU_DEAD_KILL_THREADS runs after thaw_processes(),
this means that workqueue_cpu_callback() is racy wrt create/destroy workqueue,
we should take the mutex, and it can't be spinlock_t.
Oleg.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Wed, Feb 14, 2007 at 08:13:05PM +0530, Gautham R Shenoy wrote:
> + switch (action) {
> + case CPU_UP_PREPARE:
> + /* Create a new workqueue thread for it. */
> + list_for_each_entry(wq, , list) {
Its probably safe to take the workqueue (spin) lock here (and other
notifiers as well), before traversing the list.
> + cwq = per_cpu_ptr(wq->cpu_wq, hotcpu);
> + if (create_workqueue_thread(cwq, hotcpu)) {
> + printk("workqueue for %i failed\n", hotcpu);
> + return NOTIFY_BAD;
> + }
> + }
> + break;
--
Regards,
vatsa
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Wed, Feb 14, 2007 at 08:13:05PM +0530, Gautham R Shenoy wrote: > This patch reverts all the recent workqueue hacks added to make it > hotplug safe. Oleg, This patch probably needs review for any races we may have missed to account for. Also we have considered only workqueue.c present in 2.6.20-rc6-mm3, which means some recent patches in mm tree which are yet to be published arent accounted for. Also I expect almost all worker threads to be frozen "for hotplug". The only exception I have found so far is kthread workqueue, which needs to remain unfrozen "for cpu hotplug" because kthread_create (in CPU_UP_PREPARE and stop_machine) relies on its services (while everyone else is frozen). -- Regards, vatsa - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Wed, Feb 14, 2007 at 08:13:05PM +0530, Gautham R Shenoy wrote: This patch reverts all the recent workqueue hacks added to make it hotplug safe. Oleg, This patch probably needs review for any races we may have missed to account for. Also we have considered only workqueue.c present in 2.6.20-rc6-mm3, which means some recent patches in mm tree which are yet to be published arent accounted for. Also I expect almost all worker threads to be frozen for hotplug. The only exception I have found so far is kthread workqueue, which needs to remain unfrozen for cpu hotplug because kthread_create (in CPU_UP_PREPARE and stop_machine) relies on its services (while everyone else is frozen). -- Regards, vatsa - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On Wed, Feb 14, 2007 at 08:13:05PM +0530, Gautham R Shenoy wrote:
+ switch (action) {
+ case CPU_UP_PREPARE:
+ /* Create a new workqueue thread for it. */
+ list_for_each_entry(wq, workqueues, list) {
Its probably safe to take the workqueue (spin) lock here (and other
notifiers as well), before traversing the list.
+ cwq = per_cpu_ptr(wq-cpu_wq, hotcpu);
+ if (create_workqueue_thread(cwq, hotcpu)) {
+ printk(workqueue for %i failed\n, hotcpu);
+ return NOTIFY_BAD;
+ }
+ }
+ break;
--
Regards,
vatsa
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On 02/14, Gautham R Shenoy wrote:
This patch reverts all the recent workqueue hacks added to make it
hotplug safe.
In my opinion these hacks are cleanups :)
Ok. If we use freezer then yes, we can remove cpu_populated_map and just
use for_each_online_cpu(). This is easy and good.
What else you don't like? Why do you want to remove cwq_should_stop() and
restore an ugly (ugly for workqueue.c) kthread_stop/kthread_should_stop() ?
We can restore take_over_works(), although I don't see why this is needed.
But cwq_should_stop() will just work regardless, why do you want to add
this wait_to_die ... well, hack :)
-static DEFINE_MUTEX(workqueue_mutex);
+static DEFINE_SPINLOCK(workqueue_lock);
No. We can't do this. see below.
struct workqueue_struct *__create_workqueue(const char *name,
int singlethread, int freezeable)
{
@@ -798,17 +756,20 @@ struct workqueue_struct *__create_workqu
INIT_LIST_HEAD(wq-list);
cwq = init_cpu_workqueue(wq, singlethread_cpu);
err = create_workqueue_thread(cwq, singlethread_cpu);
+ if (!err)
+ wake_up_process(cwq-thread);
} else {
- mutex_lock(workqueue_mutex);
+ spin_lock(workqueue_lock);
list_add(wq-list, workqueues);
-
- for_each_possible_cpu(cpu) {
+ spin_unlock(workqueue_lock);
+ for_each_online_cpu(cpu) {
cwq = init_cpu_workqueue(wq, cpu);
- if (err || !(cpu_online(cpu) || cpu == embryonic_cpu))
- continue;
err = create_workqueue_thread(cwq, cpu);
+ if (err)
+ break;
No, we can't break. We are going to execute destroy_workqueue(), it will
iterate over all cwqs.
+static void take_over_work(struct workqueue_struct *wq, unsigned int cpu)
+{
+ struct cpu_workqueue_struct *cwq = per_cpu_ptr(wq-cpu_wq, cpu);
+ struct list_head list;
+ struct work_struct *work;
+
+ spin_lock_irq(cwq-lock);
+ list_replace_init(cwq-worklist, list);
+
+ while (!list_empty(list)) {
+ work = list_entry(list.next,struct work_struct,entry);
+ list_del(work-entry);
+ __queue_work(per_cpu_ptr(wq-cpu_wq, smp_processor_id()), work);
+ }
+
+ spin_unlock_irq(cwq-lock);
+}
I think this is unneeded complication, but ok, should work.
static int __devinit workqueue_cpu_callback(struct notifier_block *nfb,
unsigned long action,
void *hcpu)
+ case CPU_UP_CANCELED:
+ list_for_each_entry(wq, workqueues, list) {
+ if (!per_cpu_ptr(wq-cpu_wq, hotcpu)-thread)
+ continue;
+ /* Unbind so it can run. */
+ kthread_bind(per_cpu_ptr(wq-cpu_wq, hotcpu)-thread,
+ any_online_cpu(cpu_online_map));
+ cleanup_workqueue_thread(wq, hotcpu);
}
+ break;
+
+ case CPU_DEAD:
+ list_for_each_entry(wq, workqueues, list)
+ take_over_work(wq, hotcpu);
+ break;
+
+ case CPU_DEAD_KILL_THREADS:
+ list_for_each_entry(wq, workqueues, list)
+ cleanup_workqueue_thread(wq, hotcpu);
}
Both CPU_UP_CANCELED and CPU_DEAD_KILL_THREADS runs after thaw_processes(),
this means that workqueue_cpu_callback() is racy wrt create/destroy workqueue,
we should take the mutex, and it can't be spinlock_t.
Oleg.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
On 02/14, Srivatsa Vaddagiri wrote:
On Wed, Feb 14, 2007 at 08:13:05PM +0530, Gautham R Shenoy wrote:
+ switch (action) {
+ case CPU_UP_PREPARE:
+ /* Create a new workqueue thread for it. */
+ list_for_each_entry(wq, workqueues, list) {
Its probably safe to take the workqueue (spin) lock here (and other
notifiers as well), before traversing the list.
We can't fork() under spin lock.
+ cwq = per_cpu_ptr(wq-cpu_wq, hotcpu);
+ if (create_workqueue_thread(cwq, hotcpu)) {
+ printk(workqueue for %i failed\n, hotcpu);
+ return NOTIFY_BAD;
+ }
+ }
+ break;
Oleg.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
