On Wed, Jun 13, 2018 at 12:40:29PM +0200, Thomas Hellstrom wrote:
On 06/13/2018 11:50 AM, Peter Zijlstra wrote:There is documentation!? :-) Aaah, you mean that kerneldoc comment with
So what I'm trying to say here is that wake_up_process() ensures that the+It does no such thing. But yes, it does ensure the wakee sees all prior
+ 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 to
stores IFF the wakeup happened.
+ * make sure owner sees it is wounded beforeWhat exactly are you trying to say here ?
+ * 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.
+ */
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.
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.
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.
*/
Right, so that's when the above wakeup isn't the one waking us.This is actually the pairing location of the wake_up_process() comment /- if (likely(!(atomic_long_read(&lock->base.owner) & MUTEX_FLAG_WAITERS)))Explain how we can miss that.
+ 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 pending
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().
Nah, not a NAK, but we should look at whan can be done to improve code.I can't say I'm a fan. I'm already cursing the ww_mutex stuff every timeWell, I can't speak for the current ww implementation except I didn't think
I have to look at it, and you just made it worse spagethi.
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?
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.
Hmm. Yes it indeed looks odd. Seems like the above code should do the trick.
Also, __ww_ctx_stamp_after() is crap; did we want to write:
return (signed long)(a->stamp - b->stamp) > 0;
or something?