Messages in this thread | | | Date | Thu, 22 Jan 2009 18:47:44 +0100 | Subject | Re: [PATCH 2/3] workqueue: not allow recursion run_workqueue | From | Frédéric Weisbecker <> |
| |
2009/1/22 Oleg Nesterov <oleg@redhat.com>: > On 01/22, Frederic Weisbecker wrote: >> >> On Thu, Jan 22, 2009 at 05:14:24PM +0800, Lai Jiangshan wrote: >> > static int flush_cpu_workqueue(struct cpu_workqueue_struct *cwq) >> > { >> > - int active; >> > + int active = 0; >> > + struct wq_barrier barr; >> > >> > - if (cwq->thread == current) { >> > - /* >> > - * Probably keventd trying to flush its own queue. So simply run >> > - * it by hand rather than deadlocking. >> > - */ >> > - run_workqueue(cwq); >> > - active = 1; >> > - } else { >> > - struct wq_barrier barr; >> > + BUG_ON(cwq->thread == current); >> >> Hi Lai, >> >> BUG_ON seems perhaps a bit too much for such case. The system >> will run in an endless loop because of a mistake that will not have >> necessarily a fatal end. > > Confused. Why do you think the system will run in an endless loop? > cwq-thread will exit.
Because a BUG_ON panics and then spin for ever. Yeah I shoud have said "panic", sorry... It was just to tell that a BUG_ON is the end...
> >> WARN_ON should be enough (plus the warn that lockdep will raise >> too in this case). > > and if cwq-thread proceeds after WARN_ON() it will be "lost" anyway > because it will sleep forever.
You want to say spin forever? Why would it? cwq->lock is unlocked at this time. If we keep the usual path:
if (cwq->thread == current) { run_workqueue(cwq); active = 1; }
it shouldn't hurt.
> Not that I think BUG_ON() is much better, except it is more "loud".
I don't think so IMHO, BUG_ON is for critical issues. Here it is not critical, the workqueue will flush but lockdep will warn because of recursion. That's all.
> > As for the patch itself, I completely agree with Peter. > > Oleg. > >
| |