workqueue: consistently use 'err' in __create_workqueue_key()

I guess error handling is a bit illogical in __create_workqueue_key()

Quoted 2 lines

I guess error handling is a bit illogical in __create_workqueue_key()

Please see below,

kernel/workqueue.c struct workqueue_struct *__create_workqueue_key(const char *name, · line 844

Quoted 8 lines

              for_each_possible_cpu(cpu) {
                      cwq = init_cpu_workqueue(wq, cpu);

kernel/workqueue.c +3 -1

-                     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.

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.

Oleg.

2008/7/29 Oleg Nesterov <[email protected]>:

Quoted 20 lines · Dmitry Adamushko

Quoted 2 lines

I guess error handling is a bit illogical in __create_workqueue_key()

Please see below,

kernel/workqueue.c struct workqueue_struct *__create_workqueue_key(const char *name, · line 844

Quoted 8 lines

              for_each_possible_cpu(cpu) {
                      cwq = init_cpu_workqueue(wq, cpu);

kernel/workqueue.c +3 -1

-                     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.

Quoted 3 lines

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().

Quoted 5 lines

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.

Quoted 5 lines · Dmitry Adamushko

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.

Yes, we can change __create_workqueue_key() to check err == 0 before list_add(),

Quoted 10 lines · Dmitry Adamushko

Quoted 5 lines

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.

Yes, we can change __create_workqueue_key() to check err == 0 before list_add(),

Well no, we can't do even this.

Then we have another race with cpu-hotplug. Suppose we have CPUs 0, 1, 2. create_workqueue() fails to create cwq->thread for CPU 2 and calls destroy_workqueue(). Before it takes the cpu_add_remove_lock, _cpu_down() removes CPU 1 from cpu_populated_map, but since we didn't add this wq on the global list, cwq[1]->thread remains alive.

destroy_workqueue() takes cpu_add_remove_lock, and calls cleanup_workqueue_thread() for CPUs 0 and 2. cwq[1]->thread is lost.

2008/7/29 Oleg Nesterov <[email protected]>:

Quoted 14 lines · Oleg Nesterov

Quoted 10 lines · Dmitry Adamushko

Quoted 5 lines

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.

Yes, we can change __create_workqueue_key() to check err == 0 before list_add(),

Well no, we can't do even this.

Quoted 8 lines

Then we have another race with cpu-hotplug. Suppose we have CPUs 0, 1, 2. create_workqueue() fails to create cwq->thread for CPU 2 and calls destroy_workqueue(). Before it takes the cpu_add_remove_lock, _cpu_down() removes CPU 1 from cpu_populated_map, but since we didn't add this wq on the global list, cwq[1]->thread remains alive.

destroy_workqueue() takes cpu_add_remove_lock, and calls cleanup_workqueue_thread() for CPUs 0 and 2. cwq[1]->thread is lost.

Yes, I've actually seen this case and that's why I said "the cleanup path in __create_workqueue_key() would need
to be altered" :-) likely, to the extent that it would not be a call to destroy_workqueue() anymore.

either something that only does

for_each_cpu_mask_nr(cpu, *cpu_map)

          cleanup_workqueue_thread(per_cpu_ptr(wq->cpu_wq, cpu));

and from the _same_ 'cpu_add_remove_lock' section which is used to create a wq (so we don't drop a lock);

_or_ do it outside of the locked section _but_ don't rely on for_each_cpu_mask_nr(cpu, *cpu_map)... e.g. just delete all per-cpu wq->cpu_wq structures that have been initialized (that's no matter if their respective cpus are online/offline now).

yes, maybe this cleanup path would not look all that fancy (but I didn't try) but I do think that by not exposing "partially-initialized object to the outside world" (e.g. cpu-hotplug events won't see them) this code would become more straightforward and less prone to possible errors/races.

e.g. all these "create_workqueue_key() may race with cpu-hotplug" would be gone.

Quoted 30 lines

2008/7/29 Oleg Nesterov <[email protected]>:

Quoted 14 lines · Oleg Nesterov

Quoted 10 lines · Dmitry Adamushko

Quoted 5 lines

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.

Yes, we can change __create_workqueue_key() to check err == 0 before list_add(),

Well no, we can't do even this.

Quoted 8 lines

Then we have another race with cpu-hotplug. Suppose we have CPUs 0, 1, 2. create_workqueue() fails to create cwq->thread for CPU 2 and calls destroy_workqueue(). Before it takes the cpu_add_remove_lock, _cpu_down() removes CPU 1 from cpu_populated_map, but since we didn't add this wq on the global list, cwq[1]->thread remains alive.

destroy_workqueue() takes cpu_add_remove_lock, and calls cleanup_workqueue_thread() for CPUs 0 and 2. cwq[1]->thread is lost.

Yes, I've actually seen this case and that's why I said "the cleanup path in __create_workqueue_key() would need
to be altered" :-) likely, to the extent that it would not be a call to destroy_workqueue() anymore.

Quoted 8 lines

either something that only does

for_each_cpu_mask_nr(cpu, *cpu_map)

          cleanup_workqueue_thread(per_cpu_ptr(wq->cpu_wq, cpu));

and from the _same_ 'cpu_add_remove_lock' section which is used to create a wq (so we don't drop a lock);

Why should we duplicate the code?

Quoted 4 lines

_or_ do it outside of the locked section _but_ don't rely on for_each_cpu_mask_nr(cpu, *cpu_map)... e.g. just delete all per-cpu wq->cpu_wq structures that have been initialized (that's no matter if their respective cpus are online/offline now).

Yes. And this means we change the code to handle another special case:
destroy() is called by create(). Why?

Quoted 7 lines

yes, maybe this cleanup path would not look all that fancy (but I didn't try) but I do think that by not exposing "partially-initialized object to the outside world" (e.g. cpu-hotplug events won't see them) this code would become more straightforward and less prone to possible errors/races.

e.g. all these "create_workqueue_key() may race with cpu-hotplug" would be gone.

Once again, from my pov wq is fully initialized. Yes, cwq->thread can be NULL or not, and this doesn't necessary match cpu_online_map. This is normal, for example CPU_POST_DEAD runs when CPU doesn't exists, but cwq[CPU]->thread is alive.

With the current code we just have no special cases.
I do not see
why create_workqueue_key()->destroy_workqueue() should be special.

From my pov, if we fail in create_workqueue_key(), this wq is _not_

From my pov, create_workqueue_key() is a bit illogical.

Oleg.