Re: [PATCH 2/3] block, scsi: Rework runtime power management

On Wed, Jul 18, 2018 at 03:45:15PM +0000, Bart Van Assche wrote:

On Wed, 2018-07-18 at 20:16 +0800, Ming Lei wrote:

quoted

On Wed, Jul 18, 2018 at 7:49 AM, Bart Van Assche [off-list ref] wrote:

quoted

@@ -3801,8 +3778,11 @@ int blk_pre_runtime_suspend(struct request_queue *q)
        if (!q->dev)
                return ret;

+       blk_set_preempt_only(q);
+       blk_freeze_queue_start(q);
+
        spin_lock_irq(q->queue_lock);
-       if (q->nr_pending) {
+       if (!percpu_ref_is_zero(&q->q_usage_counter)) {

This way can't work reliably because the percpu ref isn't in atomic mode
yet after blk_freeze_queue_start() returns, then percpu_ref_is_zero() won't
see accurate value of the counter, finally the device may be put down before
in-flight requests are completed by hardware.

Hello Ming,

The blk_freeze_queue_start() implementation is as follows:

void blk_freeze_queue_start(struct request_queue *q)
{
	int freeze_depth;

	freeze_depth = atomic_inc_return(&q->mq_freeze_depth);
	if (freeze_depth == 1) {
		percpu_ref_kill(&q->q_usage_counter);
		if (q->mq_ops)
			blk_mq_run_hw_queues(q, false);
	}
}

From the documentation header in include/linux/percpu-refcount.h above
percpu_ref_kill():

 * Switches @ref into atomic mode before gathering up the percpu counters
 * and dropping the initial ref.

IMO, the comment isn't accurate enough. Yes, the counter becomes atomic
mode after percpu_ref_kill() returns, but the counter can't be retrieved
accurately before the rcu confirmation is done.

One extra get is done in __percpu_ref_switch_to_atomic(), and the put pair
is run in percpu_ref_call_confirm_rcu(), which is scheduled via call_rcu_sched().

So once blk_freeze_queue_start() returns, percpu_ref_is_zero() won't
return true only until the rcu confirmation is done. That means this
approach may not put device down.

Thanks,
Ming