Re: [ANNOUNCE] 3.12.5-rt6

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

* Steven Rostedt | 2013-12-17 06:31:56 [-0500]:

>On Tue, 17 Dec 2013 08:16:31 +0100
>Mike Galbraith <bitbucket@online.de> 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! 

Not exactly sure how I managed this since I had this [0] to test this.
Which I used to come up with the patch. And now I see that the code as
it is here fails the testcase.

[0] http://breakpoint.cc/ptrace-test.c

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

Are you sure? If the state matches we should continue as long as it runs
therefore I would go for !check_task_state(). The problem here was that
I return 0 in both cases.

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

Okay I can do this. But regarding the check_task_state part, I think I
should go with:

--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -1076,9 +1076,7 @@ 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)
+	if (p->state == match_state || p->saved_state == match_state)
 		match = true;
 	raw_spin_unlock_irq(&p->pi_lock);
 
@@ -1129,11 +1127,8 @@ unsigned long wait_task_inactive(struct task_struct *p, long match_state)
 		 * is actually now running somewhere else!
 		 */
 		while (task_running(rq, p)) {
-			if (match_state) {
-				if (!unlikely(check_task_state(p, match_state)))
-					return 0;
+			if (match_state && !check_task_state(p, match_state))
 				return 0;
-			}
 			cpu_relax();
 		}
 

 Any objections?

>
>-- Steve
>

Sebastian