Messages in this thread | | | Date | Mon, 15 Dec 2008 11:47:16 +0100 | From | Oleg Nesterov <> | Subject | Re: broken do_each_pid_{thread,task} |
| |
On 12/14, Eric W. Biederman wrote: > > Jiri Slaby <jirislaby@gmail.com> writes: > > > > I'm getting > > `if (type == PIDTYPE_PID)' is unreachable > > warning from kernel/exit.c. The preprocessed code looks like: > > do { > > struct hlist_node *pos___; > > if (pgrp != ((void *)0)) > > for (LIST ITERATION) { > > { > > if (!((p->state & 4) != 0)) > > continue; > > retval = 1; > > break; > > } > > if (PIDTYPE_PGID == PIDTYPE_PID) > > break; > > } > > } while (0); > > and it's obviously wrong. > > Actually the test: > > if (PIDTYPE_PGID == PIDTYPE_PID) > > break; > Is technically ok. The compiler should optimize it out instead of warning. > Although seeing the unexpected corner case it gets us into I think it would > be good to reconsider this test.
Agreed. This check uglifies the code to fix the theoretical problem.
But, actually do_each_pid_task() is a bit ugly even without this check. Lets forget about this check for a moment.
Firstly, all hlist_for_each_entry() helpers should be "fixed", we don't need the second argument. For example, hlist_for_each_entry_rcu() could be
#define hlist_for_each_entry_rcu(pos, head, member) \ for (pos = (void*)(head)->first; rcu_dereference(pos) && ({ \ prefetch(((struct hlist_node*)pos)->next); \ pos = hlist_entry((void*)pos, typeof(*pos), member); 1; \ }); pos = (void*)(pos)->member.next)
So we can define
#define for_each_pid_task(pid, type, task) for (task = pid ? (void*)((pid)->tasks + type)->first : NULL; \ rcu_dereference(task) && ({ \ prefetch(((struct hlist_node*)task)->next); \ task = hlist_entry((void*)task, typeof(*task), pids[type].node); 1; \ }); task = (void*)(task)->pids[type].node.next)
Which can be used just
for_each_pid_task(pid, type, task) do_something(task);
Not that I think it is worth to do, though ;)
We can even restore the ugly special case for PIDTYPE_PID:
#define for_each_pid_task(pid, type, task) for (task = pid ? (void*)((pid)->tasks + type)->first : NULL; \ rcu_dereference(task) && ({ \ prefetch(((struct hlist_node*)task)->next); \ task = hlist_entry((void*)task, typeof(*task), pids[type].node); 1; }) \ task = (type != PIDTYPE_PID) ? (void*)(task)->pids[type].node.next : NULL)
> > For do_each_pid_thread(), even this code snippet from fs/ioprio.c is broken > > due to double do {} while expansion: > > do_each_pid_thread(pgrp, PIDTYPE_PGID, p) { > > ret = set_task_ioprio(p, ioprio); > > if (ret) > > break; > > } while_each_pid_thread(pgrp, PIDTYPE_PGID, p); > > > > Any idea how to get rid of this issue? > > The double loop there is certainly an issue. I'm not quite convinced that > the error handling is correct even with the break statement. But the > break statement was written when the code was just a single loop, so the > behavior is definitely not what we intended.
Yes,
> With respect to error handling and IO priorities can we fix the error handling > by doing what we do when we send a signal to a process group? That is note > that there was an error, finish processing all of the other processes and then > return the error?
Personally, I think you are right. But then we should change IOPRIO_WHO_USER accordingly, imho.
Oleg.
| |