On 9/17/18 7:39 PM, jianchao.wang wrote:

On 09/18/2018 04:15 AM, Bart Van Assche wrote:

quoted

Instead of allowing requests that are not power management requests
to enter the queue in runtime suspended status (RPM_SUSPENDED), make
the blk_get_request() caller block. This change fixes a starvation
issue: it is now guaranteed that power management requests will be
executed no matter how many blk_get_request() callers are waiting.
Instead of maintaining the q->nr_pending counter, rely on
q->q_usage_counter.

Looks like we still depend on this nr_pending for blk-legacy.

That's right. I will update the commit message.

blk_mq_queue_tag_busy_iter only accounts the driver tags. This will only work w/o io scheduler

quoted

+
  /**
   * blk_pre_runtime_suspend - Pre runtime suspend check
   * @q: the queue of the device

@@ -68,14 +101,38 @@ int blk_pre_runtime_suspend(struct request_queue *q)
  	if (!q->dev)
  		return ret;
  
+	WARN_ON_ONCE(q->rpm_status != RPM_ACTIVE);
+
+	blk_set_pm_only(q);
+	/*
+	 * This function only gets called if the most recent
+	 * pm_request_resume() call occurred at least autosuspend_delay_ms

            ^^^^^^^^^^^^^^^^^^^
pm_runtime_mark_last_busy ?

Since every pm_request_resume() call from the block layer is followed by 
a pm_runtime_mark_last_busy() call and since the latter is called later 
I think you are right. I will update the comment.

quoted

+	 * ago. Since blk_queue_enter() is called by the request allocation
+	 * code before pm_request_resume(), if no requests have a tag assigned
+	 * it is safe to suspend the device.
+	 */
+	ret = -EBUSY;
+	if (blk_requests_in_flight(q) == 0) {
+		/*
+		 * Call synchronize_rcu() such that later blk_queue_enter()
+		 * calls see the pm-only state. See also
+		 * https://urldefense.proofpoint.com/v2/url?u=http-3A__lwn.net_Articles_573497_&d=DwIDAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=7WdAxUBeiTUTCy8v-7zXyr4qk7sx26ATvfo6QSTvZyQ&m=o3yai95U5Ge5APoSiMJX64Z8wlI7gf3x0mFnfHpuA4E&s=VPnOWAeo4J4VB944La0uBcynCgVHE-qp52b_9pV8NH4&e=.
+		 */
+		synchronize_rcu();
+		if (blk_requests_in_flight(q) == 0)

Seems not safe here.

For blk-mq path:
Someone may have escaped from the preempt checking, missed the blk_pm_request_resume there,
entered into generic_make_request, but have not allocated request and occupied any tag.

There could be a similar scenario for blk-legacy path, the q->nr_pending is increased when
request is queued.

So I guess the q_usage_counter checking is still needed here.

There is only one blk_pm_request_resume() call and that call is inside 
blk_queue_enter() after the pm_only counter has been checked.

For the legacy block layer, nr_pending is increased after the 
blk_queue_enter() call from inside blk_old_get_request() succeeded.

So I don't see how blk_pm_request_resume() or q->nr_pending++ could 
escape from the preempt checking?

Thanks,

Bart.