Re: [patch, minor] workqueue: consistently use 'err' in __create_workqueue_key()

[Dmitry Adamushko]

2008/7/29 Oleg Nesterov <oleg@xxxxxxxxxx>:
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/