2014-06-09 20:28:24

by Thomas Gleixner

Subject: [patch V3 4/7] rtmutex: Siplify remove_waiter()

Exit right away, when the removed waiter was not the top prioriy
waiter on the lock. Get rid of the extra indent level.

Signed-off-by: Thomas Gleixner <[email protected]>
---
kernel/locking/rtmutex.c | 26 ++++++++++----------------
1 file changed, 10 insertions(+), 16 deletions(-)

Index: tip/kernel/locking/rtmutex.c
===================================================================
--- tip.orig/kernel/locking/rtmutex.c
+++ tip/kernel/locking/rtmutex.c
@@ -780,7 +780,7 @@ static void remove_waiter(struct rt_mute
{
int first = (waiter == rt_mutex_top_waiter(lock));
struct task_struct *owner = rt_mutex_owner(lock);
- struct rt_mutex *next_lock = NULL;
+ struct rt_mutex *next_lock;
unsigned long flags;

raw_spin_lock_irqsave(&current->pi_lock, flags);
@@ -788,28 +788,22 @@ static void remove_waiter(struct rt_mute
current->pi_blocked_on = NULL;
raw_spin_unlock_irqrestore(&current->pi_lock, flags);

- if (!owner)
+ if (!owner || !first)
return;

- if (first) {
+ raw_spin_lock_irqsave(&owner->pi_lock, flags);

- raw_spin_lock_irqsave(&owner->pi_lock, flags);
+ rt_mutex_dequeue_pi(owner, waiter);

- rt_mutex_dequeue_pi(owner, waiter);
+ if (rt_mutex_has_waiters(lock))
+ rt_mutex_enqueue_pi(owner, rt_mutex_top_waiter(lock));

- if (rt_mutex_has_waiters(lock)) {
- struct rt_mutex_waiter *next;
+ __rt_mutex_adjust_prio(owner);

- next = rt_mutex_top_waiter(lock);
- rt_mutex_enqueue_pi(owner, next);
- }
- __rt_mutex_adjust_prio(owner);
+ /* Store the lock on which owner is blocked or NULL */
+ next_lock = task_blocked_on_lock(owner);

- /* Store the lock on which owner is blocked or NULL */
- next_lock = task_blocked_on_lock(owner);
-
- raw_spin_unlock_irqrestore(&owner->pi_lock, flags);
- }
+ raw_spin_unlock_irqrestore(&owner->pi_lock, flags);

if (!next_lock)
return;

2014-06-10 00:53:32

by Steven Rostedt

Subject: Re: [patch V3 4/7] rtmutex: Siplify remove_waiter()

On Mon, 09 Jun 2014 20:28:08 -0000
Thomas Gleixner <[email protected]> wrote:

> Exit right away, when the removed waiter was not the top prioriy
> waiter on the lock. Get rid of the extra indent level.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> kernel/locking/rtmutex.c | 26 ++++++++++----------------
> 1 file changed, 10 insertions(+), 16 deletions(-)
>
> Index: tip/kernel/locking/rtmutex.c
> ===================================================================
> --- tip.orig/kernel/locking/rtmutex.c
> +++ tip/kernel/locking/rtmutex.c
> @@ -780,7 +780,7 @@ static void remove_waiter(struct rt_mute
> {
> int first = (waiter == rt_mutex_top_waiter(lock));
> struct task_struct *owner = rt_mutex_owner(lock);
> - struct rt_mutex *next_lock = NULL;
> + struct rt_mutex *next_lock;
> unsigned long flags;
>
> raw_spin_lock_irqsave(&current->pi_lock, flags);
> @@ -788,28 +788,22 @@ static void remove_waiter(struct rt_mute
> current->pi_blocked_on = NULL;
> raw_spin_unlock_irqrestore(&current->pi_lock, flags);
>

Add comment here, something like...

/*
* Only update priority if this task was the highest priority
* task waiting on the lock, and there is an owner to update.
*/

Rest looks good.

Reviewed-by: Steven Rostedt <[email protected]>

-- Steve


> - if (!owner)
> + if (!owner || !first)
> return;
>
> - if (first) {
> + raw_spin_lock_irqsave(&owner->pi_lock, flags);
>
> - raw_spin_lock_irqsave(&owner->pi_lock, flags);
> + rt_mutex_dequeue_pi(owner, waiter);
>
> - rt_mutex_dequeue_pi(owner, waiter);
> + if (rt_mutex_has_waiters(lock))
> + rt_mutex_enqueue_pi(owner, rt_mutex_top_waiter(lock));
>
> - if (rt_mutex_has_waiters(lock)) {
> - struct rt_mutex_waiter *next;
> + __rt_mutex_adjust_prio(owner);
>
> - next = rt_mutex_top_waiter(lock);
> - rt_mutex_enqueue_pi(owner, next);
> - }
> - __rt_mutex_adjust_prio(owner);
> + /* Store the lock on which owner is blocked or NULL */
> + next_lock = task_blocked_on_lock(owner);
>
> - /* Store the lock on which owner is blocked or NULL */
> - next_lock = task_blocked_on_lock(owner);
> -
> - raw_spin_unlock_irqrestore(&owner->pi_lock, flags);
> - }
> + raw_spin_unlock_irqrestore(&owner->pi_lock, flags);
>
> if (!next_lock)
> return;
>

2014-06-10 03:31:02

by Lai Jiangshan

Subject: Re: [patch V3 4/7] rtmutex: Siplify remove_waiter()

On 06/10/2014 08:53 AM, Steven Rostedt wrote:
> On Mon, 09 Jun 2014 20:28:08 -0000
> Thomas Gleixner <[email protected]> wrote:
>
>> Exit right away, when the removed waiter was not the top prioriy
>> waiter on the lock. Get rid of the extra indent level.
>>
>> Signed-off-by: Thomas Gleixner <[email protected]>
>> ---
>> kernel/locking/rtmutex.c | 26 ++++++++++----------------
>> 1 file changed, 10 insertions(+), 16 deletions(-)
>>
>> Index: tip/kernel/locking/rtmutex.c
>> ===================================================================
>> --- tip.orig/kernel/locking/rtmutex.c
>> +++ tip/kernel/locking/rtmutex.c
>> @@ -780,7 +780,7 @@ static void remove_waiter(struct rt_mute
>> {
>> int first = (waiter == rt_mutex_top_waiter(lock));
>> struct task_struct *owner = rt_mutex_owner(lock);
>> - struct rt_mutex *next_lock = NULL;
>> + struct rt_mutex *next_lock;
>> unsigned long flags;
>>
>> raw_spin_lock_irqsave(&current->pi_lock, flags);
>> @@ -788,28 +788,22 @@ static void remove_waiter(struct rt_mute
>> current->pi_blocked_on = NULL;
>> raw_spin_unlock_irqrestore(&current->pi_lock, flags);
>>
>
> Add comment here, something like...
>
> /*
> * Only update priority if this task was the highest priority
> * task waiting on the lock, and there is an owner to update.
> */
>
> Rest looks good.
>
> Reviewed-by: Steven Rostedt <[email protected]>

Reviewed-by: Lai Jiangshan <[email protected]>

Thanks,
Lai

2014-06-10 03:44:16

by Jason Low

Subject: Re: [patch V3 4/7] rtmutex: Siplify remove_waiter()

On Mon, Jun 9, 2014 at 5:53 PM, Steven Rostedt <[email protected]> wrote:
> On Mon, 09 Jun 2014 20:28:08 -0000
> Thomas Gleixner <[email protected]> wrote:
>
>> Exit right away, when the removed waiter was not the top prioriy
>> waiter on the lock. Get rid of the extra indent level.
>>
>> Signed-off-by: Thomas Gleixner <[email protected]>
>> ---
>> kernel/locking/rtmutex.c | 26 ++++++++++----------------
>> 1 file changed, 10 insertions(+), 16 deletions(-)
>>
>> Index: tip/kernel/locking/rtmutex.c
>> ===================================================================
>> --- tip.orig/kernel/locking/rtmutex.c
>> +++ tip/kernel/locking/rtmutex.c
>> @@ -780,7 +780,7 @@ static void remove_waiter(struct rt_mute
>> {
>> int first = (waiter == rt_mutex_top_waiter(lock));
>> struct task_struct *owner = rt_mutex_owner(lock);
>> - struct rt_mutex *next_lock = NULL;
>> + struct rt_mutex *next_lock;
>> unsigned long flags;
>>
>> raw_spin_lock_irqsave(&current->pi_lock, flags);
>> @@ -788,28 +788,22 @@ static void remove_waiter(struct rt_mute
>> current->pi_blocked_on = NULL;
>> raw_spin_unlock_irqrestore(&current->pi_lock, flags);
>>
>
> Add comment here, something like...
>
> /*
> * Only update priority if this task was the highest priority
> * task waiting on the lock, and there is an owner to update.
> */

Would it also make it clearer if we were to change "first" to something
such as "bool is_top_waiter"?

> Rest looks good.
>
> Reviewed-by: Steven Rostedt <[email protected]>

Reviewed-by: Jason Low <[email protected]>

> -- Steve
>
>
>> - if (!owner)
>> + if (!owner || !first)
>> return;
>>
>> - if (first) {
>> + raw_spin_lock_irqsave(&owner->pi_lock, flags);
>>
>> - raw_spin_lock_irqsave(&owner->pi_lock, flags);
>> + rt_mutex_dequeue_pi(owner, waiter);
>>
>> - rt_mutex_dequeue_pi(owner, waiter);
>> + if (rt_mutex_has_waiters(lock))
>> + rt_mutex_enqueue_pi(owner, rt_mutex_top_waiter(lock));
>>
>> - if (rt_mutex_has_waiters(lock)) {
>> - struct rt_mutex_waiter *next;
>> + __rt_mutex_adjust_prio(owner);
>>
>> - next = rt_mutex_top_waiter(lock);
>> - rt_mutex_enqueue_pi(owner, next);
>> - }
>> - __rt_mutex_adjust_prio(owner);
>> + /* Store the lock on which owner is blocked or NULL */
>> + next_lock = task_blocked_on_lock(owner);
>>
>> - /* Store the lock on which owner is blocked or NULL */
>> - next_lock = task_blocked_on_lock(owner);
>> -
>> - raw_spin_unlock_irqrestore(&owner->pi_lock, flags);
>> - }
>> + raw_spin_unlock_irqrestore(&owner->pi_lock, flags);
>>
>> if (!next_lock)
>> return;

2014-06-10 14:12:06

by Brad Mouring

Subject: Re: [patch V3 4/7] rtmutex: Siplify remove_waiter()

On Mon, Jun 09, 2014 at 08:28:08PM -0000, Thomas Gleixner wrote:
> Exit right away, when the removed waiter was not the top prioriy
> waiter on the lock. Get rid of the extra indent level.
>
> Signed-off-by: Thomas Gleixner <[email protected]>
> ---
> kernel/locking/rtmutex.c | 26 ++++++++++----------------
> 1 file changed, 10 insertions(+), 16 deletions(-)
>
> Index: tip/kernel/locking/rtmutex.c
> ===================================================================
> --- tip.orig/kernel/locking/rtmutex.c
> +++ tip/kernel/locking/rtmutex.c
> @@ -780,7 +780,7 @@ static void remove_waiter(struct rt_mute
> {
> int first = (waiter == rt_mutex_top_waiter(lock));

I agree that @first is ambiguous and would prefer something like
@is_top_waiter

Reviewed-by: Brad Mouring <[email protected]>