broken do_each_pid_{thread,task}

broken do_each_pid_{thread,task}

[Jiri Slaby]

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.

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?

Re: broken do_each_pid_{thread,task}

[Eric W. Biederman]

Jiri Slaby <jirislaby@gmail.com> 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

Re: broken do_each_pid_{thread,task}

[Oleg Nesterov]

On 12/14, Jiri Slaby wrote:
>
> 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.

Why do you think it is wrong? This break stops the "hlist_for_each"
loop, not the enclosing "do while".

Actually, I don't understand why the compiler complains, and I never
saw a warning myself.

But the check is ugly indeed, that is why the patch was named "uglify ...".

> 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);

Yes, this is obviously not what was intended. But afaics, this is
the only place which should be fixed?

> Any idea how to get rid of this issue?

We had the similar bug for IOPRIO_WHO_USER case, iirc. Probably
we can fix it the same way:

--- a/fs/ioprio.c
+++ b/fs/ioprio.c
@@ -118,8 +118,9 @@ asmlinkage long sys_ioprio_set(int which
 			do_each_pid_thread(pgrp, PIDTYPE_PGID, p) {
 				ret = set_task_ioprio(p, ioprio);
 				if (ret)
-					break;
+					goto end_pgrp;
 			} while_each_pid_thread(pgrp, PIDTYPE_PGID, p);
+end_pgrp:
 			break;
 		case IOPRIO_WHO_USER:
 			if (!who)

Oleg.

Re: broken do_each_pid_{thread,task}

[Oleg Nesterov]

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.

Re: broken do_each_pid_{thread,task}

[Jiri Slaby]

Oleg Nesterov napsal(a):
> On 12/14, Jiri Slaby wrote:
>> 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.
> 
> Why do you think it is wrong? This break stops the "hlist_for_each"
> loop, not the enclosing "do while".

The `continue' matters here (and also in other do_each_pid_task cases).
Sorry for not mentioning it explicitly.

> Actually, I don't understand why the compiler complains, and I never
> saw a warning myself.

Because the `if' is not reachable :). (And it's not compiler which complains
here.)

>> 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);
> 
> Yes, this is obviously not what was intended. But afaics, this is
> the only place which should be fixed?

Actually yes. And add a big warning to the macros or whatever to not get
into it later again.

Re: broken do_each_pid_{thread,task}

[Oleg Nesterov]

On 12/15, Jiri Slaby wrote:
>
> Oleg Nesterov napsal(a):
> > On 12/14, Jiri Slaby wrote:
> >> 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.
> >
> > Why do you think it is wrong? This break stops the "hlist_for_each"
> > loop, not the enclosing "do while".
>
> The `continue' matters here (and also in other do_each_pid_task cases).
> Sorry for not mentioning it explicitly.

Still can't understand... OK, I think we misundersood each other.
Do you agree that the code is technically correct? Or I missed
something?

"continue" looks fine to me too, it is also for the inner loop.

> > Actually, I don't understand why the compiler complains, and I never
> > saw a warning myself.
>
> Because the `if' is not reachable :).

Yes, I see it is not reachable, but I don't understand why this
deserves a warning ;)

Look, "if (PIDTYPE_PGID == PIDTYPE_PID)" is not possible too, should
the compiler (or whatever) complain?
	
> (And it's not compiler which complains
> here.)

Ah, OK, thanks. Just curious, and who does?

> > Yes, this is obviously not what was intended. But afaics, this is
> > the only place which should be fixed?
>
> Actually yes. And add a big warning to the macros or whatever to not get
> into it later again.

Agreed.

Oleg.

Re: broken do_each_pid_{thread,task}

[Jiri Slaby]

Oleg Nesterov napsal(a):
> On 12/15, Jiri Slaby wrote:
>> Oleg Nesterov napsal(a):
>>> On 12/14, Jiri Slaby wrote:
>>>> 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.
>>> Why do you think it is wrong? This break stops the "hlist_for_each"
>>> loop, not the enclosing "do while".
>> The `continue' matters here (and also in other do_each_pid_task cases).
>> Sorry for not mentioning it explicitly.
> 
> Still can't understand... OK, I think we misundersood each other.
> Do you agree that the code is technically correct? Or I missed
> something?
> 
> "continue" looks fine to me too, it is also for the inner loop.

But it doesn't jump to the `if' (this is what I would expect from the
`continue' here), but to the third statement of the `for'.

Maybe better to ask, is the test expected to be fired after *each*
invocation of the body?

>>> Actually, I don't understand why the compiler complains, and I never
>>> saw a warning myself.
>> Because the `if' is not reachable :).
> 
> Yes, I see it is not reachable, but I don't understand why this
> deserves a warning ;)
> 
> Look, "if (PIDTYPE_PGID == PIDTYPE_PID)" is not possible too, should
> the compiler (or whatever) complain?

Correct, in this particular case (and I checked that also other users which
uses `continue' inside the loop don't pass PIDTYPE_PID).

>> (And it's not compiler which complains
>> here.)
> 
> Ah, OK, thanks. Just curious, and who does?

A static analyzer. Stay tuned, we will announce it later, it's in the state
of development :).

Re: broken do_each_pid_{thread,task}

[Oleg Nesterov]

On 12/15, Jiri Slaby wrote:
>
> Oleg Nesterov napsal(a):
> > On 12/15, Jiri Slaby wrote:
> >> Oleg Nesterov napsal(a):
> >>> On 12/14, Jiri Slaby wrote:
> >>>> 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.
> >>> Why do you think it is wrong? This break stops the "hlist_for_each"
> >>> loop, not the enclosing "do while".
> >> The `continue' matters here (and also in other do_each_pid_task cases).
> >> Sorry for not mentioning it explicitly.
> >
> > Still can't understand... OK, I think we misundersood each other.
> > Do you agree that the code is technically correct? Or I missed
> > something?
> >
> > "continue" looks fine to me too, it is also for the inner loop.
>
> But it doesn't jump to the `if' (this is what I would expect from the
> `continue' here), but to the third statement of the `for'.

Yes, but this doesn't matter,

> Maybe better to ask, is the test expected to be fired after *each*
> invocation of the body?

Yes, but we need this only when type == PIDTYPE_PID, so the code
is correct.

> > Look, "if (PIDTYPE_PGID == PIDTYPE_PID)" is not possible too, should
> > the compiler (or whatever) complain?
>
> Correct, in this particular case (and I checked that also other users which
> uses `continue' inside the loop don't pass PIDTYPE_PID).

Yes.

Don't get me wrong, I do agree (let me repeat again) this check is
absolutely ugly and may cause the problems.

As I said, it fixes the minor and only theoretical problem. Perhaps we
can move this check to the code  which does do_each_pid_task(PIDTYPE_PID).
Or better yet, just introduce for_each_pid_task() (see another email).

> >> (And it's not compiler which complains
> >> here.)
> >
> > Ah, OK, thanks. Just curious, and who does?
>
> A static analyzer. Stay tuned, we will announce it later, it's in the state
> of development :).

Great, thanks ;)

Oleg.

Re: broken do_each_pid_{thread,task}

[Oleg Nesterov]

On 12/15, Oleg Nesterov wrote:
>
> On 12/14, Eric W. Biederman wrote:
> >
> > Although seeing the unexpected corner case it gets us into I think it would
> > be good to reconsider this test.

So. I can't decide whether this patch is cleanup or the further
uglification, but if anyone likes it I will be happy to send it.

Then we can kill do_each_pid_task/while_each_pid_task completely.

Oleg.

--- linux-2.6.27/include/linux/pid.h~PID_FOR_EACH	2008-10-10 00:13:53.000000000 +0200
+++ linux-2.6.27/include/linux/pid.h	2008-12-15 17:51:50.000000000 +0100
@@ -144,21 +144,25 @@ static inline pid_t pid_nr(struct pid *p
 pid_t pid_nr_ns(struct pid *pid, struct pid_namespace *ns);
 pid_t pid_vnr(struct pid *pid);
 
+/*
+ * Both old and new leaders may be attached to the same pid in the
+ * middle of de_thread(), that is why we do the special check for
+ * PIDTYPE_PID below.
+ */
+#define pid_for_each_task(pid, type, p)	\
+	for (p = (pid) ? (void*)(pid)->tasks[type].first : NULL;	\
+	     rcu_dereference(p) && ({					\
+		prefetch(((struct hlist_node*)p)->next);		\
+		p = hlist_entry((void*)p, typeof(*p), pids[type].node);	\
+		1; });							\
+	     p = ((type) != PIDTYPE_PID) ?				\
+		(void*)(p)->pids[type].node.next : NULL)
+
 #define do_each_pid_task(pid, type, task)				\
 	do {								\
-		struct hlist_node *pos___;				\
-		if (pid != NULL)					\
-			hlist_for_each_entry_rcu((task), pos___,	\
-				&pid->tasks[type], pids[type].node) {
+		pid_for_each_task(pid, type, task)
 
-			/*
-			 * Both old and new leaders may be attached to
-			 * the same pid in the middle of de_thread().
-			 */
 #define while_each_pid_task(pid, type, task)				\
-				if (type == PIDTYPE_PID)		\
-					break;				\
-			}						\
 	} while (0)
 
 #define do_each_pid_thread(pid, type, task)				\

Re: broken do_each_pid_{thread,task}

[Jiri Slaby]

On 15.12.2008 18:09, Oleg Nesterov wrote:
> On 12/15, Oleg Nesterov wrote:
>> On 12/14, Eric W. Biederman wrote:
>>> Although seeing the unexpected corner case it gets us into I think it would
>>> be good to reconsider this test.
>
> So. I can't decide whether this patch is cleanup or the further
> uglification, but if anyone likes it I will be happy to send it.

FWIW I don't like the patch :), but noone else appears to dislike it. 
Ideas, comments?

Otherwise I'll add at least a big warning about using break/cont 
statements inside the loop.

> Then we can kill do_each_pid_task/while_each_pid_task completely.
>
> Oleg.
>
> --- linux-2.6.27/include/linux/pid.h~PID_FOR_EACH	2008-10-10 00:13:53.000000000 +0200
> +++ linux-2.6.27/include/linux/pid.h	2008-12-15 17:51:50.000000000 +0100
> @@ -144,21 +144,25 @@ static inline pid_t pid_nr(struct pid *p
>   pid_t pid_nr_ns(struct pid *pid, struct pid_namespace *ns);
>   pid_t pid_vnr(struct pid *pid);
>
> +/*
> + * Both old and new leaders may be attached to the same pid in the
> + * middle of de_thread(), that is why we do the special check for
> + * PIDTYPE_PID below.
> + */
> +#define pid_for_each_task(pid, type, p)	\
> +	for (p = (pid) ? (void*)(pid)->tasks[type].first : NULL;	\
> +	     rcu_dereference(p)&&  ({					\
> +		prefetch(((struct hlist_node*)p)->next);		\
> +		p = hlist_entry((void*)p, typeof(*p), pids[type].node);	\
> +		1; });							\
> +	     p = ((type) != PIDTYPE_PID) ?				\
> +		(void*)(p)->pids[type].node.next : NULL)
> +
>   #define do_each_pid_task(pid, type, task)				\
>   	do {								\
> -		struct hlist_node *pos___;				\
> -		if (pid != NULL)					\
> -			hlist_for_each_entry_rcu((task), pos___,	\
> -				&pid->tasks[type], pids[type].node) {
> +		pid_for_each_task(pid, type, task)
>
> -			/*
> -			 * Both old and new leaders may be attached to
> -			 * the same pid in the middle of de_thread().
> -			 */
>   #define while_each_pid_task(pid, type, task)				\
> -				if (type == PIDTYPE_PID)		\
> -					break;				\
> -			}						\
>   	} while (0)
>
>   #define do_each_pid_thread(pid, type, task)				\
>

Re: broken do_each_pid_{thread,task}

[Oleg Nesterov]

On 02/24, Jiri Slaby wrote:
>
> On 15.12.2008 18:09, Oleg Nesterov wrote:
>> On 12/15, Oleg Nesterov wrote:
>>> On 12/14, Eric W. Biederman wrote:
>>>> Although seeing the unexpected corner case it gets us into I think it would
>>>> be good to reconsider this test.
>>
>> So. I can't decide whether this patch is cleanup or the further
>> uglification, but if anyone likes it I will be happy to send it.
>
> FWIW I don't like the patch :)

Well, I agree, it is not very nice ;)

But why do you dislike it? Yes, the implementation of pid_for_each_task()
is not simple. Partly because hlist_for_each_entry_rcu() is ugly and
imho should be fixed (see btw http://marc.info/?t=120879441200004).

But with this patch the callers become simpler, we can just do

	pid_for_each_task(pid, type, task)
		do_something(task);

instead of

	 do_each_pid_task(pid, type, task) {
	 	do_something(task);
	 } while_each_pid_task(pid, type, task);

and we can use break/continue safely.

> Otherwise I'll add at least a big warning about using break/cont
> statements inside the loop.

Agreed, this would be nice.

>> +#define pid_for_each_task(pid, type, p)	\
>> +	for (p = (pid) ? (void*)(pid)->tasks[type].first : NULL;	\
>> +	     rcu_dereference(p)&&  ({					\
>> +		prefetch(((struct hlist_node*)p)->next);		\
>> +		p = hlist_entry((void*)p, typeof(*p), pids[type].node);	\
>> +		1; });							\
>> +	     p = ((type) != PIDTYPE_PID) ?				\
>> +		(void*)(p)->pids[type].node.next : NULL)
>> +

Really, is this too bad?

Oleg.

Re: broken do_each_pid_{thread,task}

[Jiri Slaby]

On 24.2.2009 16:49, Oleg Nesterov wrote:
> But why do you dislike it? Yes, the implementation of pid_for_each_task()
> is not simple. Partly because hlist_for_each_entry_rcu() is ugly and
> imho should be fixed (see btw http://marc.info/?t=120879441200004).
>
> But with this patch the callers become simpler, we can just do
>
> 	pid_for_each_task(pid, type, task)
> 		do_something(task);
>
> instead of
>
> 	 do_each_pid_task(pid, type, task) {
> 	 	do_something(task);
> 	 } while_each_pid_task(pid, type, task);
>
> and we can use break/continue safely.

I like what it does, not much how. Anyway I was thinking about 
hlist_for_each_entry_rcu_param or alike (which would take additional 
parameters for 3 `for' expressions to add to standard hlist for each 
ones), but I think it would be less readable than this:

>>> +#define pid_for_each_task(pid, type, p)	\
>>> +	for (p = (pid) ? (void*)(pid)->tasks[type].first : NULL;	\
>>> +	     rcu_dereference(p)&&   ({					\
>>> +		prefetch(((struct hlist_node*)p)->next);		\
>>> +		p = hlist_entry((void*)p, typeof(*p), pids[type].node);	\
>>> +		1; });							\
>>> +	     p = ((type) != PIDTYPE_PID) ?				\
>>> +		(void*)(p)->pids[type].node.next : NULL)
>>> +
>
> Really, is this too bad?

Well, it still can be worse :).

Ok, could you repost with commit log and proper CCs or merge anywhere to 
pull from?

[RFC, PATCH] introduce pid_for_each_task() to replace do_each_pid_task()

[Oleg Nesterov]

pid_for_each_task() does the same as
do_each_pid_task() + while_each_pid_task(), so instead of

	do_each_pid_task(pid, type, task) {
		do_something(task);
	} while_each_pid_task(pid, type, task);

we can just do

	pid_for_each_task(pid, type, task)
		do_something(task);

This looks better, and we can use break/continue safely.

This patch changes do_each_pid_task() to use pid_for_each_task(), once
we convert all users of do_each_pid_task() we can kill this helper.

However. The implementation of pid_for_each_task() itself is anything
but clean/simple, so this patch asks for the explicit acks/nacks.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>

--- 6.29-rc3/include/linux/pid.h~FOR_EACH_PID	2009-01-12 23:07:48.000000000 +0100
+++ 6.29-rc3/include/linux/pid.h	2009-02-24 22:07:48.000000000 +0100
@@ -162,21 +162,25 @@ static inline pid_t pid_nr(struct pid *p
 pid_t pid_nr_ns(struct pid *pid, struct pid_namespace *ns);
 pid_t pid_vnr(struct pid *pid);
 
+/*
+ * Both old and new leaders may be attached to the same pid in the
+ * middle of de_thread(), that is why we do the special check for
+ * PIDTYPE_PID below.
+ */
+#define pid_for_each_task(pid, type, p)	\
+	for (p = (pid) ? (void*)(pid)->tasks[type].first : NULL;	\
+	     rcu_dereference(p) && ({					\
+		prefetch(((struct hlist_node*)p)->next);		\
+		p = hlist_entry((void*)p, typeof(*p), pids[type].node);	\
+		1; });							\
+	     p = ((type) != PIDTYPE_PID) ?				\
+		(void*)(p)->pids[type].node.next : NULL)
+
 #define do_each_pid_task(pid, type, task)				\
 	do {								\
-		struct hlist_node *pos___;				\
-		if ((pid) != NULL)					\
-			hlist_for_each_entry_rcu((task), pos___,	\
-				&(pid)->tasks[type], pids[type].node) {
+		pid_for_each_task(pid, type, task)
 
-			/*
-			 * Both old and new leaders may be attached to
-			 * the same pid in the middle of de_thread().
-			 */
 #define while_each_pid_task(pid, type, task)				\
-				if (type == PIDTYPE_PID)		\
-					break;				\
-			}						\
 	} while (0)
 
 #define do_each_pid_thread(pid, type, task)				\

Re: broken do_each_pid_{thread,task}

[Jiri Slaby]

On 12/15/2008 12:51 PM, Oleg Nesterov wrote:
> On 12/15, Jiri Slaby wrote:
>>
>> Oleg Nesterov napsal(a):
>>> On 12/15, Jiri Slaby wrote:
>>>> (And it's not compiler which complains
>>>> here.)
>>>
>>> Ah, OK, thanks. Just curious, and who does?
>>
>> A static analyzer. Stay tuned, we will announce it later, it's in the state
>> of development :).
> 
> Great, thanks ;)

It got released a minute ago. If you are interested:
http://lkml.org/lkml/2009/10/12/133