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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 12 Dec 2017 10:02:58 -0800
From:   Tejun Heo <tj@...nel.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     axboe@...nel.dk, linux-kernel@...r.kernel.org, oleg@...hat.com,
        kernel-team@...com, osandov@...com
Subject: Re: [PATCH 2/6] blk-mq: replace timeout synchronization with a RCU
 and generation based scheme

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