Messages in this thread | | | Date | Tue, 29 Jul 2008 13:58:02 +0200 | From | "Dmitry Adamushko" <> | Subject | Re: [patch, minor] workqueue: consistently use 'err' in __create_workqueue_key() |
| |
2008/7/29 Oleg Nesterov <oleg@tv-sign.ru>: > On 07/28, Dmitry Adamushko wrote: >> >> I guess error handling is a bit illogical in __create_workqueue_key() > > Please see below, > >> for_each_possible_cpu(cpu) { >> cwq = init_cpu_workqueue(wq, cpu); >> - if (err || !cpu_online(cpu)) >> + if (!cpu_online(cpu)) >> continue; >> err = create_workqueue_thread(cwq, cpu); >> + if (err) >> + break; > > This was done on purpose. The code above does init_cpu_workqueue(cpu) > for each possible cpu, even if we fail to create cwq->thread for some > cpu. This way destroy_workqueue() (called below) shouldn't worry about > the partially initialized workqueues. > > The patch above should work, but it assumes that destroy_workqueue() > must do nothing with cwq if cwq->thread == NULL, this is not very > robust.
Yes, I saw this test and that's why I decided that destroy_workqueue() is able (designed) to deal with partially-initialized objects.
Note, for the race scenario with cpu-hotplug (which I've overlooked indeed) which you describe below, we also seem to depend on the same "cwq->thread == NULL" test in cleanup_workqueue_thread() as follows:
assume, cpu_down(cpu) -> CPU_POST_DEAD -> cleanup_workqueue_thread() gets called for a partially initialized workqueue for 'cpu' for which create_workqueue_thread() has previously failed in create_worqueue_key().
> > And, more importantly. Let's suppose __create_workqueue_key() does > "break" and drops cpu_add_remove_lock. Then we race with cpu-hotplug > which can hit the uninitialized cwq. This is fixable, but needs other > complication.
And I'd say this behavior (of having a partially-created object visible to the outside world) is not that robust. e.g. the aforementioned race would be eliminated if we place a wq on the global list only when it's been successfully initialized.
For this goal, the cleanup path in __create_workqueue_key() would need to be altered but overall, I think it'd make the code a bit more straightforward.
[ just my 0.02, maybe I'm missing something again ;-) ]
> > Oleg. >
-- Best regards, Dmitry Adamushko
| |