Re: [PATCH V8 07/11] blk-mq: stop to handle IO and drain IO before hctx becomes inactive

["Paul E. McKenney" <paulmck@kernel.org>]

On Sat, Apr 25, 2020 at 05:48:32PM +0200, Christoph Hellwig wrote:
> FYI, here is what I think we should be doing (but the memory model
> experts please correct me):

I would be happy to, but could you please tell me what to apply this
against?  I made several wrong guesses, and am not familiar enough with
this code to evaluate this patch in isolation.

							Thanx, Paul

>  - just drop the direct_issue flag and check for the CPU, which is
>    cheap enough
>  - replace the raw_smp_processor_id with a get_cpu to make sure we
>    don't hit the tiny migration windows
>  - a bunch of random cleanups to make the code easier to read, mostly
>    by being more self-documenting or improving the comments.
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index bfa4020256ae9..da749865f6eed 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1049,28 +1049,16 @@ static bool __blk_mq_get_driver_tag(struct request *rq)
>  		atomic_inc(&data.hctx->nr_active);
>  	}
>  	data.hctx->tags->rqs[rq->tag] = rq;
> -	return true;
> -}
> -
> -static bool blk_mq_get_driver_tag(struct request *rq, bool direct_issue)
> -{
> -	if (rq->tag != -1)
> -		return true;
>  
> -	if (!__blk_mq_get_driver_tag(rq))
> -		return false;
>  	/*
> -	 * Add one memory barrier in case that direct issue IO process is
> -	 * migrated to other CPU which may not belong to this hctx, so we can
> -	 * order driver tag assignment and checking BLK_MQ_S_INACTIVE.
> -	 * Otherwise, barrier() is enough given both setting BLK_MQ_S_INACTIVE
> -	 * and driver tag assignment are run on the same CPU in case that
> -	 * BLK_MQ_S_INACTIVE is set.
> +	 * Ensure updates to rq->tag and tags->rqs[] are seen by
> +	 * blk_mq_tags_inflight_rqs.  This pairs with the smp_mb__after_atomic
> +	 * in blk_mq_hctx_notify_offline.  This only matters in case a process
> +	 * gets migrated to another CPU that is not mapped to this hctx.
>  	 */
> -	if (unlikely(direct_issue && rq->mq_ctx->cpu != raw_smp_processor_id()))
> +	if (rq->mq_ctx->cpu != get_cpu())
>  		smp_mb();
> -	else
> -		barrier();
> +	put_cpu();
>  
>  	if (unlikely(test_bit(BLK_MQ_S_INACTIVE, &rq->mq_hctx->state))) {
>  		blk_mq_put_driver_tag(rq);
> @@ -1079,6 +1067,13 @@ static bool blk_mq_get_driver_tag(struct request *rq, bool direct_issue)
>  	return true;
>  }
>  
> +static bool blk_mq_get_driver_tag(struct request *rq)
> +{
> +	if (rq->tag != -1)
> +		return true;
> +	return __blk_mq_get_driver_tag(rq);
> +}
> +
>  static int blk_mq_dispatch_wake(wait_queue_entry_t *wait, unsigned mode,
>  				int flags, void *key)
>  {
> @@ -1125,7 +1120,7 @@ static bool blk_mq_mark_tag_wait(struct blk_mq_hw_ctx *hctx,
>  		 * Don't clear RESTART here, someone else could have set it.
>  		 * At most this will cost an extra queue run.
>  		 */
> -		return blk_mq_get_driver_tag(rq, false);
> +		return blk_mq_get_driver_tag(rq);
>  	}
>  
>  	wait = &hctx->dispatch_wait;
> @@ -1151,7 +1146,7 @@ static bool blk_mq_mark_tag_wait(struct blk_mq_hw_ctx *hctx,
>  	 * allocation failure and adding the hardware queue to the wait
>  	 * queue.
>  	 */
> -	ret = blk_mq_get_driver_tag(rq, false);
> +	ret = blk_mq_get_driver_tag(rq);
>  	if (!ret) {
>  		spin_unlock(&hctx->dispatch_wait_lock);
>  		spin_unlock_irq(&wq->lock);
> @@ -1252,7 +1247,7 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
>  			break;
>  		}
>  
> -		if (!blk_mq_get_driver_tag(rq, false)) {
> +		if (!blk_mq_get_driver_tag(rq)) {
>  			/*
>  			 * The initial allocation attempt failed, so we need to
>  			 * rerun the hardware queue when a tag is freed. The
> @@ -1284,7 +1279,7 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list,
>  			bd.last = true;
>  		else {
>  			nxt = list_first_entry(list, struct request, queuelist);
> -			bd.last = !blk_mq_get_driver_tag(nxt, false);
> +			bd.last = !blk_mq_get_driver_tag(nxt);
>  		}
>  
>  		ret = q->mq_ops->queue_rq(hctx, &bd);
> @@ -1886,7 +1881,7 @@ static blk_status_t __blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
>  	if (!blk_mq_get_dispatch_budget(hctx))
>  		goto insert;
>  
> -	if (!blk_mq_get_driver_tag(rq, true)) {
> +	if (!blk_mq_get_driver_tag(rq)) {
>  		blk_mq_put_dispatch_budget(hctx);
>  		goto insert;
>  	}
> @@ -2327,23 +2322,24 @@ static bool blk_mq_inflight_rq(struct request *rq, void *data,
>  static unsigned blk_mq_tags_inflight_rqs(struct blk_mq_hw_ctx *hctx)
>  {
>  	struct count_inflight_data count_data = {
> -		.count	= 0,
>  		.hctx	= hctx,
>  	};
>  
>  	blk_mq_all_tag_busy_iter(hctx->tags, blk_mq_count_inflight_rq,
>  			blk_mq_inflight_rq, &count_data);
> -
>  	return count_data.count;
>  }
>  
> -static void blk_mq_hctx_drain_inflight_rqs(struct blk_mq_hw_ctx *hctx)
> +static inline bool blk_mq_last_cpu_in_hctx(unsigned int cpu,
> +		struct blk_mq_hw_ctx *hctx)
>  {
> -	while (1) {
> -		if (!blk_mq_tags_inflight_rqs(hctx))
> -			break;
> -		msleep(5);
> -	}
> +	if (!cpumask_test_cpu(cpu, hctx->cpumask))
> +		return false;
> +	if (cpumask_next_and(-1, hctx->cpumask, cpu_online_mask) != cpu)
> +		return false;
> +	if (cpumask_next_and(cpu, hctx->cpumask, cpu_online_mask) < nr_cpu_ids)
> +		return false;
> +	return true;
>  }
>  
>  static int blk_mq_hctx_notify_offline(unsigned int cpu, struct hlist_node *node)
> @@ -2351,25 +2347,19 @@ static int blk_mq_hctx_notify_offline(unsigned int cpu, struct hlist_node *node)
>  	struct blk_mq_hw_ctx *hctx = hlist_entry_safe(node,
>  			struct blk_mq_hw_ctx, cpuhp_online);
>  
> -	if (!cpumask_test_cpu(cpu, hctx->cpumask))
> -		return 0;
> -
> -	if ((cpumask_next_and(-1, hctx->cpumask, cpu_online_mask) != cpu) ||
> -	    (cpumask_next_and(cpu, hctx->cpumask, cpu_online_mask) < nr_cpu_ids))
> +	if (!blk_mq_last_cpu_in_hctx(cpu, hctx))
>  		return 0;
>  
>  	/*
> -	 * The current CPU is the last one in this hctx, S_INACTIVE
> -	 * can be observed in dispatch path without any barrier needed,
> -	 * cause both are run on one same CPU.
> +	 * Order setting BLK_MQ_S_INACTIVE versus checking rq->tag and rqs[tag],
> +	 * in blk_mq_tags_inflight_rqs.  It pairs with the smp_mb() in
> +	 * blk_mq_get_driver_tag.
>  	 */
>  	set_bit(BLK_MQ_S_INACTIVE, &hctx->state);
> -	/*
> -	 * Order setting BLK_MQ_S_INACTIVE and checking rq->tag & rqs[tag],
> -	 * and its pair is the smp_mb() in blk_mq_get_driver_tag
> -	 */
>  	smp_mb__after_atomic();
> -	blk_mq_hctx_drain_inflight_rqs(hctx);
> +
> +	while (blk_mq_tags_inflight_rqs(hctx))
> +		msleep(5);
>  	return 0;
>  }
>