On Wed, Jun 13, 2018 at 12:40:29PM +0200, Thomas Hellstrom wrote:
On 06/13/2018 11:50 AM, Peter Zijlstra wrote:
- lockdep_assert_held(&lock->wait_lock);
- if (owner && hold_ctx && __ww_ctx_stamp_after(hold_ctx, ww_ctx) &&
ww_ctx->acquired > 0) {WRITE_ONCE(hold_ctx->wounded, true);if (owner != current) {/** wake_up_process() inserts a write memory barrier toIt does no such thing. But yes, it does ensure the wakee sees all prior stores IFF the wakeup happened.
* make sure owner sees it is wounded before* TASK_RUNNING in case it's sleeping on another* ww_mutex. Note that owner points to a valid* task_struct as long as we hold the wait_lock.*/What exactly are you trying to say here ?
I'm thinking this is the pairing barrier to the smp_mb() below, with your list_empty() thing? Might make sense to write a single coherent comment and refer to the other location.
So what I'm trying to say here is that wake_up_process() ensures that the owner, if in !TASK_RUNNING, sees the write to hold_ctx->wounded before the transition to TASK_RUNNING. This was how I interpreted "woken up" in the wake up process documentation.
There is documentation!? :-) Aaah, you mean that kerneldoc comment with wake_up_process() ? Yeah, that needs fixing. /me puts on endless todo list.
Anyway, wakeup providing that ordering isn't something that needs a comment of that size; and I think the only comment here is that we care about the ordering and a reference to the site(s) that pairs with it.
Maybe something like:
/* * __ww_mutex_lock_check_stamp() will observe our wounded store. */
- if (likely(!(atomic_long_read(&lock->base.owner) & MUTEX_FLAG_WAITERS)))
- if (likely(list_empty(&lock->base.wait_list))) return; /*
@@ -653,6 +695,17 @@ __ww_mutex_lock_check_stamp(struct mutex *lock, struct mutex_waiter *waiter, struct ww_acquire_ctx *hold_ctx = READ_ONCE(ww->ctx); struct mutex_waiter *cur;
- /*
* If we miss a wounded == true here, we will have a pendingExplain how we can miss that.
This is actually the pairing location of the wake_up_process() comment / code discussed above. Here we should have !TASK_RUNNING, and let's say ctx->wounded is set by another process immediately after we've read it (we "miss" it). At that point there must be a pending wake-up-process() for us and we'll pick up the set value of wounded on the next iteration after returning from schedule().
Right, so that's when the above wakeup isn't the one waking us.
I can't say I'm a fan. I'm already cursing the ww_mutex stuff every time I have to look at it, and you just made it worse spagethi.
Well, I can't speak for the current ww implementation except I didn't think it was too hard to understand for a first time reader.
Admittedly the Wound-Wait path makes it worse since it's a preemptive algorithm and we need to touch other processes a acquire contexts and worry about ordering.
So, assuming your review comments are fixed up, is that a solid NAK or do you have any suggestion that would make you more comfortable with the code? like splitting out ww-stuff to a separate file?
Nah, not a NAK, but we should look at whan can be done to improve code. Maybe add a few more comments that explain why. Part of the problem with ww_mutex is always that I forget exactly how they work and mutex.c doesn't have much useful comments in (most of those are in ww_mutex.h and I always forget to look there).
Also; I'm not at all sure about the exact difference between what we have and what you propose. I did read the documentation part (I really should not have to) but it just doesn't jive.
I suspect you're using preemption entirely different from what we usually call a preemption.
Also, __ww_ctx_stamp_after() is crap; did we want to write:
return (signed long)(a->stamp - b->stamp) > 0;
or something?