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/
[RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
This patch reverts all the recent workqueue hacks added to make it hotplug safe. Signed-off-by : Srivatsa Vaddagiri <[EMAIL PROTECTED]> Signed-off-by : Gautham R Shenoy <[EMAIL PROTECTED]> kernel/workqueue.c | 225 +++-- 1 files changed, 98 insertions(+), 127 deletions(-) Index: hotplug/kernel/workqueue.c === --- hotplug.orig/kernel/workqueue.c +++ hotplug/kernel/workqueue.c @@ -47,7 +47,6 @@ struct cpu_workqueue_struct { struct workqueue_struct *wq; struct task_struct *thread; - int should_stop; int run_depth; /* Detect run_workqueue() recursion depth */ } cacheline_aligned; @@ -65,7 +64,7 @@ struct workqueue_struct { /* All the per-cpu workqueues on the system, for hotplug cpu to add/remove threads to each one as cpus come/go. */ -static DEFINE_MUTEX(workqueue_mutex); +static DEFINE_SPINLOCK(workqueue_lock); static LIST_HEAD(workqueues); static int singlethread_cpu __read_mostly; @@ -344,24 +343,6 @@ static void run_workqueue(struct cpu_wor spin_unlock_irqrestore(>lock, flags); } -/* - * NOTE: the caller must not touch *cwq if this func returns true - */ -static int cwq_should_stop(struct cpu_workqueue_struct *cwq) -{ - int should_stop = cwq->should_stop; - - if (unlikely(should_stop)) { - spin_lock_irq(>lock); - should_stop = cwq->should_stop && list_empty(>worklist); - if (should_stop) - cwq->thread = NULL; - spin_unlock_irq(>lock); - } - - return should_stop; -} - static int worker_thread(void *__cwq) { struct cpu_workqueue_struct *cwq = __cwq; @@ -392,7 +373,7 @@ static int worker_thread(void *__cwq) siginitset(_mask, sigmask(SIGCHLD)); do_sigaction(SIGCHLD, , (struct k_sigaction *)0); - for (;;) { + while (!kthread_should_stop()) { if (cwq->wq->freezeable) { try_to_freeze(); if (cpu_is_offline(bind_cpu)) @@ -400,14 +381,12 @@ static int worker_thread(void *__cwq) } prepare_to_wait(>more_work, , TASK_INTERRUPTIBLE); - if (!cwq->should_stop && list_empty(>worklist)) + if (list_empty(>worklist)) schedule(); finish_wait(>more_work, ); - if (cwq_should_stop(cwq)) - break; - - run_workqueue(cwq); + if (!list_empty(>worklist)) + run_workqueue(cwq); } return 0; @@ -445,9 +424,6 @@ static void insert_wq_barrier(struct cpu insert_work(cwq, >work, tail); } -/* optimization, we could use cpu_possible_map */ -static cpumask_t cpu_populated_map __read_mostly; - static void flush_cpu_workqueue(struct cpu_workqueue_struct *cwq) { if (cwq->thread == current) { @@ -492,7 +468,7 @@ void fastcall flush_workqueue(struct wor else { int cpu; - for_each_cpu_mask(cpu, cpu_populated_map) + for_each_online_cpu(cpu) flush_cpu_workqueue(per_cpu_ptr(wq->cpu_wq, cpu)); } } @@ -552,7 +528,7 @@ void flush_work(struct workqueue_struct else { int cpu; - for_each_cpu_mask(cpu, cpu_populated_map) + for_each_online_cpu(cpu) wait_on_work(per_cpu_ptr(wq->cpu_wq, cpu), work); } } @@ -737,43 +713,25 @@ init_cpu_workqueue(struct workqueue_stru static int create_workqueue_thread(struct cpu_workqueue_struct *cwq, int cpu) { struct task_struct *p; + struct workqueue_struct *wq = cwq->wq; + const char *fmt = is_single_threaded(wq) ? "%s" : "%s/%d"; - spin_lock_irq(>lock); - cwq->should_stop = 0; - p = cwq->thread; - spin_unlock_irq(>lock); - - if (!p) { - struct workqueue_struct *wq = cwq->wq; - const char *fmt = is_single_threaded(wq) ? "%s" : "%s/%d"; - - p = kthread_create(worker_thread, cwq, fmt, wq->name, cpu); - /* -* Nobody can add the work_struct to this cwq, -* if (caller is __create_workqueue) -* nobody should see this wq -* else // caller is CPU_UP_PREPARE -* cpu is not on cpu_online_map -* so we can abort safely. -*/ - if (IS_ERR(p)) - return PTR_ERR(p); - - cwq->thread = p; - if (!is_single_threaded(wq)) - kthread_bind(p, cpu); - /* -* Cancels affinity if the caller is CPU_UP_PREPARE. -* Needs a cleanup, but OK. -*/ -
[RFC PATCH(Experimental) 2/4] Revert changes to workqueue.c
This patch reverts all the recent workqueue hacks added to make it hotplug safe. Signed-off-by : Srivatsa Vaddagiri [EMAIL PROTECTED] Signed-off-by : Gautham R Shenoy [EMAIL PROTECTED] kernel/workqueue.c | 225 +++-- 1 files changed, 98 insertions(+), 127 deletions(-) Index: hotplug/kernel/workqueue.c === --- hotplug.orig/kernel/workqueue.c +++ hotplug/kernel/workqueue.c @@ -47,7 +47,6 @@ struct cpu_workqueue_struct { struct workqueue_struct *wq; struct task_struct *thread; - int should_stop; int run_depth; /* Detect run_workqueue() recursion depth */ } cacheline_aligned; @@ -65,7 +64,7 @@ struct workqueue_struct { /* All the per-cpu workqueues on the system, for hotplug cpu to add/remove threads to each one as cpus come/go. */ -static DEFINE_MUTEX(workqueue_mutex); +static DEFINE_SPINLOCK(workqueue_lock); static LIST_HEAD(workqueues); static int singlethread_cpu __read_mostly; @@ -344,24 +343,6 @@ static void run_workqueue(struct cpu_wor spin_unlock_irqrestore(cwq-lock, flags); } -/* - * NOTE: the caller must not touch *cwq if this func returns true - */ -static int cwq_should_stop(struct cpu_workqueue_struct *cwq) -{ - int should_stop = cwq-should_stop; - - if (unlikely(should_stop)) { - spin_lock_irq(cwq-lock); - should_stop = cwq-should_stop list_empty(cwq-worklist); - if (should_stop) - cwq-thread = NULL; - spin_unlock_irq(cwq-lock); - } - - return should_stop; -} - static int worker_thread(void *__cwq) { struct cpu_workqueue_struct *cwq = __cwq; @@ -392,7 +373,7 @@ static int worker_thread(void *__cwq) siginitset(sa.sa.sa_mask, sigmask(SIGCHLD)); do_sigaction(SIGCHLD, sa, (struct k_sigaction *)0); - for (;;) { + while (!kthread_should_stop()) { if (cwq-wq-freezeable) { try_to_freeze(); if (cpu_is_offline(bind_cpu)) @@ -400,14 +381,12 @@ static int worker_thread(void *__cwq) } prepare_to_wait(cwq-more_work, wait, TASK_INTERRUPTIBLE); - if (!cwq-should_stop list_empty(cwq-worklist)) + if (list_empty(cwq-worklist)) schedule(); finish_wait(cwq-more_work, wait); - if (cwq_should_stop(cwq)) - break; - - run_workqueue(cwq); + if (!list_empty(cwq-worklist)) + run_workqueue(cwq); } return 0; @@ -445,9 +424,6 @@ static void insert_wq_barrier(struct cpu insert_work(cwq, barr-work, tail); } -/* optimization, we could use cpu_possible_map */ -static cpumask_t cpu_populated_map __read_mostly; - static void flush_cpu_workqueue(struct cpu_workqueue_struct *cwq) { if (cwq-thread == current) { @@ -492,7 +468,7 @@ void fastcall flush_workqueue(struct wor else { int cpu; - for_each_cpu_mask(cpu, cpu_populated_map) + for_each_online_cpu(cpu) flush_cpu_workqueue(per_cpu_ptr(wq-cpu_wq, cpu)); } } @@ -552,7 +528,7 @@ void flush_work(struct workqueue_struct else { int cpu; - for_each_cpu_mask(cpu, cpu_populated_map) + for_each_online_cpu(cpu) wait_on_work(per_cpu_ptr(wq-cpu_wq, cpu), work); } } @@ -737,43 +713,25 @@ init_cpu_workqueue(struct workqueue_stru static int create_workqueue_thread(struct cpu_workqueue_struct *cwq, int cpu) { struct task_struct *p; + struct workqueue_struct *wq = cwq-wq; + const char *fmt = is_single_threaded(wq) ? %s : %s/%d; - spin_lock_irq(cwq-lock); - cwq-should_stop = 0; - p = cwq-thread; - spin_unlock_irq(cwq-lock); - - if (!p) { - struct workqueue_struct *wq = cwq-wq; - const char *fmt = is_single_threaded(wq) ? %s : %s/%d; - - p = kthread_create(worker_thread, cwq, fmt, wq-name, cpu); - /* -* Nobody can add the work_struct to this cwq, -* if (caller is __create_workqueue) -* nobody should see this wq -* else // caller is CPU_UP_PREPARE -* cpu is not on cpu_online_map -* so we can abort safely. -*/ - if (IS_ERR(p)) - return PTR_ERR(p); - - cwq-thread = p; - if (!is_single_threaded(wq)) - kthread_bind(p, cpu); - /* -* Cancels affinity if the caller is CPU_UP_PREPARE. -* Needs a cleanup, but OK. -*/ -
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/