Re: broken do_each_pid_{thread,task}

[Eric W. Biederman]

Jiri Slaby <jirislaby@xxxxxxxxx> writes:

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

The break statement is also fine because the outer loop is only executed
once so it simply functions as an enclosing block, and the break transfers
control to where it should go.

> After investigating this code usage all around, it's broken on many places
> this or similar way.
>
> 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.

However I also agree with Ken Chen's assessment that we need to loop over
threads and not just the process group leaders in some cases such as
setting the io-priority.

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?

Eric

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