| Message ID | 20230811213604.548235-1-bvanassche@acm.org |
|---|---|
| Headers | show |
| Series | Improve performance for zoned UFS devices | expand |
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);