Re: [PATCH 2/6] blk-mq: replace timeout synchronization with a RCU and generation based scheme

[Tejun Heo <tj@kernel.org>]

Hello, Peter.

On Tue, Dec 12, 2017 at 11:09:35AM +0100, Peter Zijlstra wrote:
> > +	/*
> > +	 * ->aborted_gstate is used by the timeout to claim a specific
> > +	 * recycle instance of this request.  See blk_mq_timeout_work().
> > +	 */
> > +	struct u64_stats_sync aborted_gstate_sync;
> > +	u64 aborted_gstate;
> 
> So I dislike that u64_stats_sync thingy. Esp when used on a single
> variable like this.

Hmm... I see.

> There are two alternatives, but I don't understand the code well enough
> to judge the trade-offs.
> 
> 1) use gstate_seq for this too; yes it will add some superfluous
>    instructions on 64bit targets, but if timeouts are a slow path
>    this might not matter.

For aborted_gstate, the heavier reader side is the completion hot
path.  That's two rmbs, which in itself isn't too much but is still
difficult to justify.

> 2) use the pattern we use for cfs_rq::min_vruntime; namely:
> 
> 	u64 aborted_gstate
> #ifdef CONFIG_64BIT
> 	u64 aborted_gstate_copy;
> #endif
> 
> 
> static inline void blk_mq_rq_set_abort(struct rq *rq, u64 gstate)
> {
> 	rq->aborted_gstate = gstate;
> #ifdef CONFIG_64BIT
> 	smp_wmb();
> 	rq->aborted_gstate_copy = gstate;
> #endif
> }
> 
> static inline u64 blk_mq_rq_get_abort(struct rq *rq)
> {
> #ifdef CONFIG_64BIT
> 	u64 abort, copy;
> 
> 	do {
> 		copy = rq->aborted_gstate_copy;
> 		smp_rmb();
> 		abort = rq->aborted_gstate;
> 	} while (abort != copy);
> 
> 	return abort;
> #else
> 	return rq->aborted_gstate;
> #endif
> }
> 
>    which is actually _faster_ than the u64_stats_sync stuff (for a
>    single variable).

Hmm... doing the seq reading on the variable content itself, so if we
had something like this as library, I'd be happy to use it but I
really don't want to open-code this.

> But it might not matter; I just dislike that thing, could be me.

I'll leave it as-is for now.  Probably the right thing to do in the
longer term is adding the seq-reading-by-content-thing in the library.

Thanks.

-- 
tejun