From: Bart Van Assche <Bart.VanAssche@wdc.com>
To: "peterz@infradead.org" <peterz@infradead.org>,
        "axboe@fb.com" <axboe@fb.com>
CC: "parri.andrea@gmail.com" <parri.andrea@gmail.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "tom.leiming@gmail.com" <tom.leiming@gmail.com>,
        "hch@lst.de" <hch@lst.de>,
        "paulmck@linux.vnet.ibm.com" <paulmck@linux.vnet.ibm.com>,
        "will.deacon@arm.com" <will.deacon@arm.com>,
        "boqun.feng@gmail.com" <boqun.feng@gmail.com>,
        "stern@rowland.harvard.edu" <stern@rowland.harvard.edu>
Subject: Re: [PATCH] blk-mq: Start to fix memory ordering...
Thread-Topic: [PATCH] blk-mq: Start to fix memory ordering...
Thread-Index: AQHTJV2SVHzl+/irYU24JEa9ufXxRKKmYigA
Date: Tue, 5 Sep 2017 14:51:25 +0000
Message-ID: <1504623084.4135.27.camel@wdc.com>
References: <20170904090932.ngfefw2clot3sbqi@hirez.programming.kicks-ass.net>
In-Reply-To: <20170904090932.ngfefw2clot3sbqi@hirez.programming.kicks-ass.net>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="utf-8"
Content-ID: <0826C1DE68D43E49B14570E2CD4CF7EE@namprd04.prod.outlook.com>
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Sep 2017 14:51:25.4342
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR0401MB1772
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from base64 to 8bit by nfs id v85EqPse009989
Content-Length: 3226
Lines: 88

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.