Messages in this thread | | | From | Bart Van Assche <> | Subject | Re: [PATCH] blk-mq: Start to fix memory ordering... | Date | Tue, 5 Sep 2017 14:51:25 +0000 |
| |
On Mon, 2017-09-04 at 11:09 +0200, Peter Zijlstra wrote: > /* > * Mark us as started and clear complete. Complete might have been > * set if requeue raced with timeout, which then marked it as > * complete. So be sure to clear complete again when we start > * the request, otherwise we'll ignore the completion event. > + * > + * Ensure that ->deadline is visible before set STARTED, such that
It seems like there is something wrong with the grammar in the above sentence? Did you perhaps mean "before we set STARTED"?
> + * blk_mq_check_expired() is guaranteed to observe our ->deadline > + * when it observes STARTED. > */ > - if (!test_bit(REQ_ATOM_STARTED, &rq->atomic_flags)) > - set_bit(REQ_ATOM_STARTED, &rq->atomic_flags); > - if (test_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags)) > + smp_mb__before_atomic(); > + set_bit(REQ_ATOM_STARTED, &rq->atomic_flags); > + if (test_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags)) { > + /* > + * Coherence order guarantees these consequtive stores to a > + * singe variable propagate in the specified order. Thus the > + * clear_bit() is ordered _after_ the set bit. See > + * blk_mq_check_expired(). > + */ > clear_bit(REQ_ATOM_COMPLETE, &rq->atomic_flags); > + }
Is this new comment really useful? If you want to keep it please spell "consecutive" correctly.
> if (q->dma_drain_size && blk_rq_bytes(rq)) { > /* > @@ -744,11 +751,20 @@ static void blk_mq_check_expired(struct > struct request *rq, void *priv, bool reserved) > { > struct blk_mq_timeout_data *data = priv; > + unsigned long deadline; > > if (!test_bit(REQ_ATOM_STARTED, &rq->atomic_flags)) > return; > > /* > + * Ensures that if we see STARTED we must also see our > + * up-to-date deadline, see blk_mq_start_request(). > + */ > + smp_rmb(); > + > + deadline = READ_ONCE(rq->deaedline);
"deaedline" is a spelling error. Has this patch been tested?
> + /* > * The rq being checked may have been freed and reallocated > * out already here, we avoid this race by checking rq->deadline > * and REQ_ATOM_COMPLETE flag together: > @@ -761,10 +777,20 @@ static void blk_mq_check_expired(struct > * and clearing the flag in blk_mq_start_request(), so > * this rq won't be timed out too. > */ > - if (time_after_eq(jiffies, rq->deadline)) { > - if (!blk_mark_rq_complete(rq)) > + if (time_after_eq(jiffies, deadline)) { > + if (!blk_mark_rq_complete(rq)) { > + /* > + * Relies on the implied MB from test_and_clear() to > + * order the COMPLETE load against the STARTED load. > + * Orders against the coherence order in > + * blk_mq_start_request(). > + * > + * This ensures that if we see !COMPLETE we must see > + * STARTED and ignore this timeout. > + */ > blk_mq_rq_timed_out(rq, reserved); > - } else if (!data->next_set || time_after(data->next, rq->deadline)) { > + } > + } else if (!data->next_set || time_after(data->next, deadline)) { > data->next = rq->deadline; > data->next_set = 1; > }
Apparently a READ_ONCE(rq->deadline) statement has been added but not all rq->deadline reads have been changed into reads of the local variable "deadline"? Was that really your intention?
Bart. | |