On 5/23/23 22:55, Adrian Hunter wrote:
On 23/05/23 22:57, Bart Van Assche wrote:
On 5/23/23 12:19, Adrian Hunter wrote:
On 23/05/23 20:10, Bart Van Assche wrote:
The overhead of BLK_MQ_F_BLOCKING is small relative to the
time required to queue a UFS command so I think enabling
BLK_MQ_F_BLOCKING for all UFS host controllers is fine.
Doesn't it also force the queue to be run asynchronously always?
But in any case, it doesn't seem like something to force on
drivers just because it would take a bit more coding to make it
optional.
Making BLK_MQ_F_BLOCKING optional would complicate testing of the
UFS driver. Although it is possible to make BLK_MQ_F_BLOCKING
optional, I'm wondering whether it is worth it? I haven't noticed
any performance difference in my tests with BLK_MQ_F_BLOCKING
enabled compared to BLK_MQ_F_BLOCKING disabled.
It is hard to know the effects, especially wrt to future hardware.
Are you perhaps referring to performance effects? I think the block
layer can be modified to run the queue synchronously in most cases even
if BLK_MQ_F_BLOCKING is set. The patch below works fine but is probably
not acceptable for upstream because it uses in_atomic():
diff --git a/block/blk-mq.c b/block/blk-mq.c
index 551e7760f45e..00fbe0aa56b5 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1471,9 +1471,23 @@ static void blk_mq_requeue_work(struct work_struct *work)
blk_mq_run_hw_queues(q, false);
}
+static bool may_sleep(void)
+{
+#ifdef CONFIG_PREEMPT_COUNT
+ return !in_atomic() && !irqs_disabled();
+#else
+ return false;
+#endif
+}
+
void blk_mq_kick_requeue_list(struct request_queue *q)
{
+ if (may_sleep()) {
+ blk_mq_requeue_work(&q->requeue_work.work);
+ return;
+ }
kblockd_mod_delayed_work_on(WORK_CPU_UNBOUND, &q->requeue_work, 0);
+
}
EXPORT_SYMBOL(blk_mq_kick_requeue_list);
@@ -2228,7 +2242,7 @@ void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async)
if (!need_run)
return;
- if (async || (hctx->flags & BLK_MQ_F_BLOCKING) ||
+ if (async || (hctx->flags & BLK_MQ_F_BLOCKING && !may_sleep()) ||
!cpumask_test_cpu(raw_smp_processor_id(), hctx->cpumask)) {
blk_mq_delay_run_hw_queue(hctx, 0);
return;
What about something like this? [ ... ]
This introduces a redundancy and a potential for a conflict between the
"nonblocking" flag and the UFSHCD_CAP_CLK_GATING flag. It is unfortunate
that hba->caps is set so late otherwise we could use the result of
(hba->caps & UFSHCD_CAP_CLK_GATING) to check whether or not
BLK_MQ_F_BLOCKING is needed.
Additionally, this patch introduces a use-after-free issue since it
causes scsi_host_alloc() to store a pointer to a stack variable (sht)
into a SCSI host structure.
Bart.
