cleanup and remove some extra spinlocks from rtmutex

Another question: why should we take ->pi_lock to modify rt_mutex's ->wait_list?
It looks confusing and unneeded to me, because we already hold ->wait_lock. For example, wakeup_next_waiter() takes current's ->pi_lock before plist_del(), which seems to be completely offtopic, since current->pi_blocked_on has nothing to do with that rt_mutex.

Note also that ->pi_blocked_on is always modified while also holding ->pi_blocked_on->lock->wait_lock, and things like rt_mutex_top_waiter() need ->wait_lock too, so I don't think we need ->pi_lock for ->wait_list.

In other words, could you please explain to me whether the patch below correct or not?

Thanks,

Oleg.

kernel/rtmutex.c renamed from kernel/rtmutex.c~2_rtm +3 -0

 	/* Grab the next task */
 	task = rt_mutex_owner(lock);
 	get_task_struct(task);

@@ -416,15 +416,15 @@ static int task_blocks_on_rt_mutex(struc
 	plist_node_init(&waiter->list_entry, current->prio);
 	plist_node_init(&waiter->pi_list_entry, current->prio);
 
+	current->pi_blocked_on = waiter;
+
+	spin_unlock_irqrestore(&current->pi_lock, flags);

kernel/rtmutex.c renamed from kernel/rtmutex.c~2_rtm +0 -0

 	/*
 	 * Remove it from current->pi_waiters. We do not adjust a
 	 * possible priority boost right now. We execute wakeup in the

Well, we are talking about small optimizations now, moving only a few instructions outside the lock. Except for one of them it is correct, but it is worth risking stability for now?

Quoted 6 lines

kernel/rtmutex.c renamed from kernel/rtmutex.c~2_rtm +0 -0

@@ -236,6 +236,10 @@ static int rt_mutex_adjust_prio_chain(st
 		goto out_unlock_pi;
 	}
 

kernel/rtmutex.c static int rt_mutex_adjust_prio_chain(st · line 242

Quoted 4 lines

kernel/rtmutex.c renamed from kernel/rtmutex.c~2_rtm +4 -0

+	/* Release the task */
+	spin_unlock_irqrestore(&task->pi_lock, flags);
+	put_task_struct(task);
+

So you want the task to go away here and use it below?

Quoted 8 lines

kernel/rtmutex.c renamed from kernel/rtmutex.c~2_rtm +0 -0

 	top_waiter = rt_mutex_top_waiter(lock);
 
 	/* Requeue the waiter */

@@ -243,10 +247,6 @@ static int rt_mutex_adjust_prio_chain(st
 	waiter->list_entry.prio = task->prio;
 	plist_add(&waiter->list_entry, &lock->wait_list);
 

kernel/rtmutex.c static int rt_mutex_adjust_prio_chain(st · line 249

Quoted 4 lines

kernel/rtmutex.c renamed from kernel/rtmutex.c~2_rtm +0 -4

-	/* Release the task */
-	spin_unlock_irqrestore(&task->pi_lock, flags);
-	put_task_struct(task);
-

No! It is used in the line just above, so we better be sure it still exists!

Quoted 4 lines · Esben Nielsen

Well, we are talking about small optimizations now, moving only a few instructions outside the lock. Except for one of them it is correct, but it is worth risking stability for now?

Yes, optimization is small, but I think this cleanups the code, which is (imho) more important. That said, I don't suggest this patch, it was a question. I stiil can't find a time to read the code hard and convince myself I can understand it :)

Also, I think such a change opens the possibility for further cleanups.

Quoted 6 lines

Quoted 6 lines

kernel/rtmutex.c renamed from kernel/rtmutex.c~2_rtm +0 -0

@@ -236,6 +236,10 @@ static int rt_mutex_adjust_prio_chain(st
 		goto out_unlock_pi;
 	}
 

Quoted 6 lines

kernel/rtmutex.c static int rt_mutex_adjust_prio_chain(st · line 242

Quoted 4 lines

kernel/rtmutex.c renamed from kernel/rtmutex.c~2_rtm +4 -0

+	/* Release the task */
+	spin_unlock_irqrestore(&task->pi_lock, flags);
+	put_task_struct(task);
+

So you want the task to go away here and use it below?

task->pi_blocked_on != NULL, we hold task->pi_blocked_on->lock->wait_lock. Can it go away ?

Quoted 9 lines

Quoted 9 lines

kernel/rtmutex.c renamed from kernel/rtmutex.c~2_rtm +0 -0

 	top_waiter = rt_mutex_top_waiter(lock);
 
 	/* Requeue the waiter */

@@ -243,10 +247,6 @@ static int rt_mutex_adjust_prio_chain(st
 	waiter->list_entry.prio = task->prio;
 	plist_add(&waiter->list_entry, &lock->wait_list);
 

Quoted 7 lines

kernel/rtmutex.c static int rt_mutex_adjust_prio_chain(st · line 249

Quoted 4 lines

kernel/rtmutex.c renamed from kernel/rtmutex.c~2_rtm +0 -4

-	/* Release the task */
-	spin_unlock_irqrestore(&task->pi_lock, flags);
-	put_task_struct(task);
-

No! It is used in the line just above, so we better be sure it still exists!

See above. If I am wrong, we can move this line

	waiter->list_entry.prio = task->prio;

up, under ->pi_lock. plist_del() doesn't need a valid ->prio.

Thanks for your answer!

Oleg.

Quoted 3 lines · Oleg Nesterov

task->pi_blocked_on != NULL, we hold task->pi_blocked_on->lock->wait_lock. Can it go away ?

That is correct. But does it make the code more readable?

When you read
the code you shouldn't need to go into that kind of complicated arguments to see the correctness - unless the code can't be written otherwise.

Quoted 7 lines · Esben Nielsen

Quoted 3 lines · Oleg Nesterov

task->pi_blocked_on != NULL, we hold task->pi_blocked_on->lock->wait_lock. Can it go away ?

That is correct. But does it make the code more readable?

Well, in my opinion - yes. But yes, it's only my personal feeling :)

Quoted 3 lines

When you read
the code you shouldn't need to go into that kind of complicated arguments to see the correctness - unless the code can't be written otherwise.

Sure, this needs a comment.

Thanks again,

Oleg.

Quoted 2 lines · Oleg Nesterov

Sure, this needs a comment.

It is even better if you can read the code without a comment. Good code doesn't need comments.