I don't yet have a full picture, but let me make a few comments.
On Sat, Dec 09, 2017 at 11:25:21AM -0800, Tejun Heo wrote:
> +static u64 blk_mq_rq_aborted_gstate(struct request *rq)
> +{
> + unsigned int start;
> + u64 aborted_gstate;
> +
> + do {
> + start = u64_stats_fetch_begin(&rq->aborted_gstate_sync);
> + aborted_gstate = rq->aborted_gstate;
> + } while (u64_stats_fetch_retry(&rq->aborted_gstate_sync, start));
> +
> + return aborted_gstate;
> +}
> + /* if in-flight && overdue, mark for abortion */
> + if ((gstate & MQ_RQ_STATE_MASK) == MQ_RQ_IN_FLIGHT &&
> + time_after_eq(jiffies, deadline)) {
> + u64_stats_update_begin(&rq->aborted_gstate_sync);
> + rq->aborted_gstate = gstate;
> + u64_stats_update_end(&rq->aborted_gstate_sync);
> + data->nr_expired++;
> + hctx->nr_expired++;
> } else if (!data->next_set || time_after(data->next, deadline)) {
> + /*
> + * ->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.
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.
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).
But it might not matter; I just dislike that thing, could be me.
