diff mbox series

[v8,9/9] scsi: ufs: Inform the block layer about write ordering

Message ID 20230811213604.548235-10-bvanassche@acm.org (mailing list archive)
State New, archived
Headers show
Series Improve performance for zoned UFS devices | expand

Commit Message

Bart Van Assche Aug. 11, 2023, 9:35 p.m. UTC 
From the UFSHCI 4.0 specification, about the legacy (single queue) mode:
"The host controller always process transfer requests in-order according
to the order submitted to the list. In case of multiple commands with
single doorbell register ringing (batch mode), The dispatch order for
these transfer requests by host controller will base on their index in
the List. A transfer request with lower index value will be executed
before a transfer request with higher index value."

From the UFSHCI 4.0 specification, about the MCQ mode:
"Command Submission
1. Host SW writes an Entry to SQ
2. Host SW updates SQ doorbell tail pointer

Command Processing
3. After fetching the Entry, Host Controller updates SQ doorbell head
   pointer
4. Host controller sends COMMAND UPIU to UFS device"

In other words, for both legacy and MCQ mode, UFS controllers are
required to forward commands to the UFS device in the order these
commands have been received from the host.

Notes:
- For legacy mode this is only correct if the host submits one
  command at a time. The UFS driver does this.
- Also in legacy mode, the command order is not preserved if
  auto-hibernation is enabled in the UFS controller. Hence, enable
  zone write locking if auto-hibernation is enabled.

This patch improves performance as follows on my test setup:
- With the mq-deadline scheduler: 2.5x more IOPS for small writes.
- When not using an I/O scheduler compared to using mq-deadline with
  zone locking: 4x more IOPS for small writes.

Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: Can Guo <quic_cang@quicinc.com>
Cc: Avri Altman <avri.altman@wdc.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Damien Le Moal <dlemoal@kernel.org>
Cc: Ming Lei <ming.lei@redhat.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/ufs/core/ufshcd.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

Comments

Bao D. Nguyen Aug. 12, 2023, 5:09 p.m. UTC | #1 
On 8/11/2023 2:35 PM, Bart Van Assche wrote:
>  From the UFSHCI 4.0 specification, about the legacy (single queue) mode:
> "The host controller always process transfer requests in-order according
> to the order submitted to the list. In case of multiple commands with
> single doorbell register ringing (batch mode), The dispatch order for
> these transfer requests by host controller will base on their index in
> the List. A transfer request with lower index value will be executed
> before a transfer request with higher index value."
> 
>  From the UFSHCI 4.0 specification, about the MCQ mode:
> "Command Submission
> 1. Host SW writes an Entry to SQ
> 2. Host SW updates SQ doorbell tail pointer
> 
> Command Processing
> 3. After fetching the Entry, Host Controller updates SQ doorbell head
>     pointer
> 4. Host controller sends COMMAND UPIU to UFS device"
> 
> In other words, for both legacy and MCQ mode, UFS controllers are
> required to forward commands to the UFS device in the order these
> commands have been received from the host.
> 
> Notes:
> - For legacy mode this is only correct if the host submits one
>    command at a time. The UFS driver does this.
> - Also in legacy mode, the command order is not preserved if
>    auto-hibernation is enabled in the UFS controller. Hence, enable
>    zone write locking if auto-hibernation is enabled.
> 
> This patch improves performance as follows on my test setup:
> - With the mq-deadline scheduler: 2.5x more IOPS for small writes.
> - When not using an I/O scheduler compared to using mq-deadline with
>    zone locking: 4x more IOPS for small writes.
> 
> Cc: Martin K. Petersen <martin.petersen@oracle.com>
> Cc: Can Guo <quic_cang@quicinc.com>
> Cc: Avri Altman <avri.altman@wdc.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Cc: Ming Lei <ming.lei@redhat.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>   drivers/ufs/core/ufshcd.c | 35 ++++++++++++++++++++++++++++++++++-
>   1 file changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index ae7b868f9c26..71cee10c75ad 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -4337,23 +4337,48 @@ int ufshcd_uic_hibern8_exit(struct ufs_hba *hba)
>   }
>   EXPORT_SYMBOL_GPL(ufshcd_uic_hibern8_exit);
>   
> +static void ufshcd_update_preserves_write_order(struct ufs_hba *hba,
> +						bool preserves_write_order)
> +{
> +	struct scsi_device *sdev;
> +
> +	shost_for_each_device(sdev, hba->host)
> +		blk_freeze_queue_start(sdev->request_queue);
> +	shost_for_each_device(sdev, hba->host) {
> +		struct request_queue *q = sdev->request_queue;
> +
> +		blk_mq_freeze_queue_wait(q);
> +		q->limits.driver_preserves_write_order = preserves_write_order;
> +		blk_mq_unfreeze_queue(q);
> +	}
> +}
> +
>   void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit)
>   {
>   	unsigned long flags;
> -	bool update = false;
> +	bool prev_state, new_state, update = false;
>   
>   	if (!ufshcd_is_auto_hibern8_supported(hba))
>   		return;
>   
>   	spin_lock_irqsave(hba->host->host_lock, flags);
> +	prev_state = ufshcd_is_auto_hibern8_enabled(hba);
>   	if (hba->ahit != ahit) {
>   		hba->ahit = ahit;
>   		update = true;
>   	}
> +	new_state = ufshcd_is_auto_hibern8_enabled(hba);
>   	spin_unlock_irqrestore(hba->host->host_lock, flags);
>   
>   	if (!update)
>   		return;
> +	if (!is_mcq_enabled(hba) && !prev_state && new_state) {
> +		/*
> +		 * Auto-hibernation will be enabled for legacy UFSHCI mode. Tell
> +		 * the block layer that write requests may be reordered.
> +		 */
> +		ufshcd_update_preserves_write_order(hba, false);
Hi Bart,
I am not reviewing other patches in this series, so I don't know the 
whole context. Here is my comment on this patch alone.

Looks like you rely on ufshcd_auto_hibern8_update() being invoked so 
that you can update the driver_preserves_write_order. However, the 
hba->ahit value can be updated by the vendor's driver, and 
ufshcd_auto_hibern8_enable() can be invoked without 
ufshcd_auto_hibern8_update(). Therefore, you may have a situation where 
the driver_preserves_write_order is true by default, but 
Auto-hibernation is enabled by default.

Thanks,
Bao

> +	}
>   	if (!pm_runtime_suspended(&hba->ufs_device_wlun->sdev_gendev)) {
>   		ufshcd_rpm_get_sync(hba);
>   		ufshcd_hold(hba);
> @@ -4361,6 +4386,13 @@ void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit)
>   		ufshcd_release(hba);
>   		ufshcd_rpm_put_sync(hba);
>   	}
> +	if (!is_mcq_enabled(hba) && prev_state && !new_state) {
> +		/*
> +		 * Auto-hibernation has been disabled. Tell the block layer that
> +		 * the order of write requests is preserved.
> +		 */
> +		ufshcd_update_preserves_write_order(hba, true);
> +	}
>   }
>   EXPORT_SYMBOL_GPL(ufshcd_auto_hibern8_update);
>   
> @@ -5140,6 +5172,7 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
>   
>   	ufshcd_hpb_configure(hba, sdev);
>   
> +	q->limits.driver_preserves_write_order = true;
>   	blk_queue_update_dma_pad(q, PRDT_DATA_BYTE_COUNT_PAD - 1);
>   	if (hba->quirks & UFSHCD_QUIRK_4KB_DMA_ALIGNMENT)
>   		blk_queue_update_dma_alignment(q, SZ_4K - 1);
Bart Van Assche Aug. 14, 2023, 4:23 p.m. UTC | #2 
On 8/12/23 10:09, Bao D. Nguyen wrote:
> I am not reviewing other patches in this series, so I don't know the 
> whole context. Here is my comment on this patch alone.
> 
> Looks like you rely on ufshcd_auto_hibern8_update() being invoked so 
> that you can update the driver_preserves_write_order. However, the 
> hba->ahit value can be updated by the vendor's driver, and 
> ufshcd_auto_hibern8_enable() can be invoked without 
> ufshcd_auto_hibern8_update(). Therefore, you may have a situation where 
> the driver_preserves_write_order is true by default, but 
> Auto-hibernation is enabled by default.

Hi Bao,

Other than setting a default value for auto-hibernation, vendor drivers
must not modify the auto-hibernation settings.

ufshcd_auto_hibern8_enable() calls from outside 
ufshcd_auto_hibern8_update() are used to reapply auto-hibernation
settings and not to modify auto-hibernation settings.

So I think that integrating the following change on top of this patch is
sufficient:

@@ -5172,7 +5172,9 @@ static int ufshcd_slave_configure(struct 
scsi_device *sdev)

  	ufshcd_hpb_configure(hba, sdev);

-	q->limits.driver_preserves_write_order = true;
+	q->limits.driver_preserves_write_order =
+		!ufshcd_is_auto_hibern8_supported(hba) ||
+		FIELD_GET(UFSHCI_AHIBERN8_TIMER_MASK, hba->ahit) == 0;
  	blk_queue_update_dma_pad(q, PRDT_DATA_BYTE_COUNT_PAD - 1);
  	if (hba->quirks & UFSHCD_QUIRK_4KB_DMA_ALIGNMENT)
  		blk_queue_update_dma_alignment(q, SZ_4K - 1);

Thanks,

Bart.
Bao D. Nguyen Aug. 15, 2023, 3:20 a.m. UTC | #3 
On 8/14/2023 9:23 AM, Bart Van Assche wrote:
> On 8/12/23 10:09, Bao D. Nguyen wrote:
>> I am not reviewing other patches in this series, so I don't know the 
>> whole context. Here is my comment on this patch alone.
>>
>> Looks like you rely on ufshcd_auto_hibern8_update() being invoked so 
>> that you can update the driver_preserves_write_order. However, the 
>> hba->ahit value can be updated by the vendor's driver, and 
>> ufshcd_auto_hibern8_enable() can be invoked without 
>> ufshcd_auto_hibern8_update(). Therefore, you may have a situation 
>> where the driver_preserves_write_order is true by default, but 
>> Auto-hibernation is enabled by default.
> 
> Hi Bao,
> 
> Other than setting a default value for auto-hibernation, vendor drivers
> must not modify the auto-hibernation settings.
IMO, it is not a good solution to prohibit changing auto-hibernation 
settings during runtime. I can think of a few situations where changing 
this parameter may help the system such as:
- Reduce heat. The device may be hot, so hibernate often helps.
- Battery is low, so hibernate quicker to save battery.
- Allows the vendor to make decision whether to trade performance for 
power, or vice versa. Sometimes, the system can afford trading 
performance for power saving.

How about this?
- Make ufshcd_auto_hibern8_enable() a static function. It should not be 
called from the vendor drivers.
- Mandate that vendor drivers must only update auto-hibernation settings 
via the ufshcd_auto_hibern8_update() api. This function is already 
exported, so only need to update the function comment to make it clear 
(updating the hba->ahit alone may result in abnormal behavior).

Thanks,
Bao


> 
> ufshcd_auto_hibern8_enable() calls from outside 
> ufshcd_auto_hibern8_update() are used to reapply auto-hibernation
> settings and not to modify auto-hibernation settings.
> 
> So I think that integrating the following change on top of this patch is
> sufficient:
> 
> @@ -5172,7 +5172,9 @@ static int ufshcd_slave_configure(struct 
> scsi_device *sdev)
> 
>       ufshcd_hpb_configure(hba, sdev);
> 
> -    q->limits.driver_preserves_write_order = true;
> +    q->limits.driver_preserves_write_order =
> +        !ufshcd_is_auto_hibern8_supported(hba) ||
> +        FIELD_GET(UFSHCI_AHIBERN8_TIMER_MASK, hba->ahit) == 0;
Yes, this should help.

>       blk_queue_update_dma_pad(q, PRDT_DATA_BYTE_COUNT_PAD - 1);
>       if (hba->quirks & UFSHCD_QUIRK_4KB_DMA_ALIGNMENT)
>           blk_queue_update_dma_alignment(q, SZ_4K - 1);
> 
> Thanks,
> 
> Bart.
>
Bart Van Assche Aug. 15, 2023, 3:41 p.m. UTC | #4 
On 8/14/23 20:20, Bao D. Nguyen wrote:
> How about this?
> - Make ufshcd_auto_hibern8_enable() a static function. It should not >   be called from the vendor drivers.
> - Mandate that vendor drivers must only update auto-hibernation >   settings via the ufshcd_auto_hibern8_update() api. This function 
is>   already exported, so only need to update the function comment to> 
  make it clear (updating the hba->ahit alone may result in abnormal
>   behavior).

That sounds good to me. I will look into implementing the above proposal.

Thanks,

Bart.
diff mbox series

Patch

diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index ae7b868f9c26..71cee10c75ad 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -4337,23 +4337,48 @@  int ufshcd_uic_hibern8_exit(struct ufs_hba *hba)
 }
 EXPORT_SYMBOL_GPL(ufshcd_uic_hibern8_exit);
 
+static void ufshcd_update_preserves_write_order(struct ufs_hba *hba,
+						bool preserves_write_order)
+{
+	struct scsi_device *sdev;
+
+	shost_for_each_device(sdev, hba->host)
+		blk_freeze_queue_start(sdev->request_queue);
+	shost_for_each_device(sdev, hba->host) {
+		struct request_queue *q = sdev->request_queue;
+
+		blk_mq_freeze_queue_wait(q);
+		q->limits.driver_preserves_write_order = preserves_write_order;
+		blk_mq_unfreeze_queue(q);
+	}
+}
+
 void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit)
 {
 	unsigned long flags;
-	bool update = false;
+	bool prev_state, new_state, update = false;
 
 	if (!ufshcd_is_auto_hibern8_supported(hba))
 		return;
 
 	spin_lock_irqsave(hba->host->host_lock, flags);
+	prev_state = ufshcd_is_auto_hibern8_enabled(hba);
 	if (hba->ahit != ahit) {
 		hba->ahit = ahit;
 		update = true;
 	}
+	new_state = ufshcd_is_auto_hibern8_enabled(hba);
 	spin_unlock_irqrestore(hba->host->host_lock, flags);
 
 	if (!update)
 		return;
+	if (!is_mcq_enabled(hba) && !prev_state && new_state) {
+		/*
+		 * Auto-hibernation will be enabled for legacy UFSHCI mode. Tell
+		 * the block layer that write requests may be reordered.
+		 */
+		ufshcd_update_preserves_write_order(hba, false);
+	}
 	if (!pm_runtime_suspended(&hba->ufs_device_wlun->sdev_gendev)) {
 		ufshcd_rpm_get_sync(hba);
 		ufshcd_hold(hba);
@@ -4361,6 +4386,13 @@  void ufshcd_auto_hibern8_update(struct ufs_hba *hba, u32 ahit)
 		ufshcd_release(hba);
 		ufshcd_rpm_put_sync(hba);
 	}
+	if (!is_mcq_enabled(hba) && prev_state && !new_state) {
+		/*
+		 * Auto-hibernation has been disabled. Tell the block layer that
+		 * the order of write requests is preserved.
+		 */
+		ufshcd_update_preserves_write_order(hba, true);
+	}
 }
 EXPORT_SYMBOL_GPL(ufshcd_auto_hibern8_update);
 
@@ -5140,6 +5172,7 @@  static int ufshcd_slave_configure(struct scsi_device *sdev)
 
 	ufshcd_hpb_configure(hba, sdev);
 
+	q->limits.driver_preserves_write_order = true;
 	blk_queue_update_dma_pad(q, PRDT_DATA_BYTE_COUNT_PAD - 1);
 	if (hba->quirks & UFSHCD_QUIRK_4KB_DMA_ALIGNMENT)
 		blk_queue_update_dma_alignment(q, SZ_4K - 1);