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
