On Thu, 15 May 2014 08:47:09 +0200
Ingo Molnar <mi...@kernel.org> wrote:
>
> A couple of suggestions:
>
> 1)
>
> * Thomas Gleixner <t...@linutronix.de> wrote:
>
> > + if (requeue) {
> > + if (waiter == rt_mutex_top_waiter(lock)) {
>
> So we have a 'top_waiter' local variable already at this point, and we
> use it here:
>
> > + /* Boost the owner */
> > + rt_mutex_dequeue_pi(task, top_waiter);
> > + rt_mutex_enqueue_pi(task, waiter);
> > + __rt_mutex_adjust_prio(task);
> > +
> > + } else if (top_waiter == waiter) {
>
> To me it's a bit confusing that we have both the 'top_waiter' local
> variable and also evaluate 'rt_mutex_top_waiter()' directly.
>
> So what happens is that when we do the requeue, the top waiter might
> change. I'd really suggest to signal that via naming - i.e. add
> another local variable (which GCC will optimize out happily), named
> descriptively:
>
> orig_top_waiter = top_waiter;
>
> and use that variable after that point.
I wouldn't use "orig_top_waiter" as all the "orig_*" variables are
passed in via the parameters and do not change.
Maybe: last_top_waiter?
>
> > + /* Deboost the owner */
> > + rt_mutex_dequeue_pi(task, waiter);
> > + waiter = rt_mutex_top_waiter(lock);
> > + rt_mutex_enqueue_pi(task, waiter);
> > + __rt_mutex_adjust_prio(task);
> > + }
> > }
>
> 2)
>
> Also another small flow of control side comment, because this code is
> apparently rather tricky, I'd suggest a bit more explicit structure to
> show the real flow of the logic: for example in the first reading of
> the above block I mistakenly read it as a usual 'if () { } else { }'
> block pattern - which it really isn't.
>
> Something like this would be slightly easier to understand 'at a
> glance', IMHO:
>
> if (requeue) {
> if (waiter == top_waiter) {
> /* Boost the owner */
> rt_mutex_dequeue_pi(task, orig_top_waiter);
> rt_mutex_enqueue_pi(task, waiter);
> __rt_mutex_adjust_prio(task);
>
> } else {
> if (orig_top_waiter == waiter) {
> /* Deboost the owner */
> rt_mutex_dequeue_pi(task, waiter);
> waiter = rt_mutex_top_waiter(lock);
> rt_mutex_enqueue_pi(task, waiter);
> __rt_mutex_adjust_prio(task);
> } else {
> /* The requeueing did not affect us, no need to
> boost or deboost */
> }
> }
> }
>
> Assuming you agree with this structure, it's a bit more verbose, but
> this might be one of the cases where verbosity helps readability.
> (Note that I already propagated the 'orig_top_waiter' name into it.)
Instead of the nesting, what about:
if (requeue) {
if (waiter == top_waiter) {
/* Boost the owner */
rt_mutex_dequeue_pi(task, last_top_waiter);
rt_mutex_enqueue_pi(task, waiter);
__rt_mutex_adjust_prio(task);
} else if (last_top_waiter == waiter) {
/* Deboost the owner */
rt_mutex_dequeue_pi(task, waiter);
waiter = rt_mutex_top_waiter(lock);
rt_mutex_enqueue_pi(task, waiter);
__rt_mutex_adjust_prio(task);
} else {
/*
* The requeueing did not affect us, no need to
* boost or deboost
*/
}
}
That last else should also visually keep one from thinking this is a
simple "if { } else { }" construct.
(Note that I already propagated the 'last_top_waiter' name into it ;-)
>
> 3)
>
> Also note how the code continues:
>
> raw_spin_unlock_irqrestore(&task->pi_lock, flags);
>
> top_waiter = rt_mutex_top_waiter(lock);
> raw_spin_unlock(&lock->wait_lock);
>
> if (!detect_deadlock && waiter != top_waiter)
> goto out_put_task;
>
> goto again;
>
> So we evaluate 'top_waiter' again - maybe we could move that line to
> the two branches that actually have a chance to change the top waiter,
> and not change it in the 'no need to requeue' case.
This is probably a good idea and needs my change I suggested earlier
(which doesn't do the wakeup if requeue == 0).
-- Steve
>
> So ... all in one, what I would suggest is something like the patch
> below, on top of your two patches. Totally untested and such.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
