On 23/05/23 20:10, Bart Van Assche wrote:
> On 5/23/23 09:39, Adrian Hunter wrote:
>> On 18/05/23 01:23, Bart Van Assche wrote:
>>> Prepare for adding code in ufshcd_queuecommand() that may sleep.
>>>
>>> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
>>> ---
>>> drivers/ufs/core/ufshcd.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
>>> index 7ee150d67d49..993034ac1696 100644
>>> --- a/drivers/ufs/core/ufshcd.c
>>> +++ b/drivers/ufs/core/ufshcd.c
>>> @@ -8756,6 +8756,7 @@ static const struct scsi_host_template ufshcd_driver_template = {
>>> .max_host_blocked = 1,
>>> .track_queue_depth = 1,
>>> .skip_settle_delay = 1,
>>> + .queuecommand_may_block = true,
>>
>> Shouldn't this only be for controllers that support
>> clock gating?
>
> Hi Adrian,
>
> 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.
> BLK_MQ_F_BLOCKING causes the block layer to use SRCU
> instead of RCU. The cost of the sleepable RCU primitives is dominated by
> the memory barrier (smp_mb()) in the srcu_read_lock() and srcu_read_unlock()
> calls. From kernel/rcu/srcutree.c:
>
> int __srcu_read_lock(struct srcu_struct *ssp)
> {
> int idx;
>
> idx = READ_ONCE(ssp->srcu_idx) & 0x1;
> this_cpu_inc(ssp->sda->srcu_lock_count[idx].counter);
> smp_mb(); /* B */ /* Avoid leaking the critical section. */
> return idx;
> }
> EXPORT_SYMBOL_GPL(__srcu_read_lock);
>
> void __srcu_read_unlock(struct srcu_struct *ssp, int idx)
> {
> smp_mb(); /* C */ /* Avoid leaking the critical section. */
> this_cpu_inc(ssp->sda->srcu_unlock_count[idx].counter);
> }
> EXPORT_SYMBOL_GPL(__srcu_read_unlock);
>
> The rcu_read_lock() and rcu_read_lock() implementations do not call
> smp_mb() as one can see in include/linux/rcupdate.h:
>
> static inline void __rcu_read_lock(void)
> {
> preempt_disable();
> }
>
> static inline void __rcu_read_unlock(void)
> {
> preempt_enable();
> if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD))
> rcu_read_unlock_strict();
> }
>
> Thanks,
>
> Bart.
>
