On Tue, 17 Dec 2013 08:16:31 +0100
Mike Galbraith <bitbucket@xxxxxxxxx> wrote:
> Hi Sebastian,
>
> Looks like there's a booboo here:
>
> On Mon, 2013-12-16 at 10:14 +0100, Sebastian Andrzej Siewior wrote:
> "ptrace: fix ptrace vs tasklist_lock race" added..
>
> @@ -1068,8 +1082,11 @@ unsigned long wait_task_inactive(struct
> * is actually now running somewhere else!
> */
> while (task_running(rq, p)) {
> - if (match_state && unlikely(p->state != match_state))
> + if (match_state) {
> + if (!unlikely(check_task_state(p, match_state)))
> + return 0;
> return 0;
> + }
Ouch!
> cpu_relax();
> }
>
> ..which is how it stays with the whole series applied.
>
> The patch contains hunk 2 from
>
> "sched/rt: Fix wait_task_interactive() to test rt_spin_lock state",
>
> which went away in -rt6, so it seems the busted hunk should be as below
> if the two are to be merged.
>
> @@ -1068,8 +1082,10 @@ unsigned long wait_task_inactive(struct
> * is actually now running somewhere else!
> */
> while (task_running(rq, p)) {
> - if (match_state && unlikely(p->state != match_state))
> + if (match_state && unlikely(p->state != match_state)
> + && unlikely(p->saved_state != match_state))
> return 0;
> + }
Yeah, it should just be:
if (match_state && check_task_state(p, match_state))
return 0;
Also, looking at check_task_state():
+static bool check_task_state(struct task_struct *p, long match_state)
+{
+ bool match = false;
+
+ raw_spin_lock_irq(&p->pi_lock);
+ if (p->state == match_state)
+ match = true;
+ else if (p->saved_state == match_state)
+ match = true;
Why the if () else if()? and not just:
if (p->state == match_state || p->save_state == match_state)
match = true;
?
The else if makes me think there's something missing.
-- Steve
+ raw_spin_unlock_irq(&p->pi_lock);
+
+ return match;
+}
> cpu_relax();
> }
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rt-users" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
