| Message ID | 20211130233324.1402448-17-bvanassche@acm.org |
|---|---|
| State | New |
| Headers | show |
| Series | UFS patches for kernel v5.17 | expand |
On 11/30/2021 3:33 PM, Bart Van Assche wrote: > Remove the clock scaling lock from ufshcd_queuecommand() since it is a > performance bottleneck. Instead, use synchronize_rcu_expedited() to wait > for ongoing ufshcd_queuecommand() calls. > > Cc: Asutosh Das (asd) <asutoshd@codeaurora.org> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/ufs/ufshcd.c | 12 +++++++----- > drivers/scsi/ufs/ufshcd.h | 1 + > 2 files changed, 8 insertions(+), 5 deletions(-) > Hi Bart, Say an IO (req1) has crossed the scsi_host_queue_ready() check but hasn't yet reached ufshcd_queuecommand() and DBR is 0. ufshcd_clock_scaling_prepare() is invoked and completes and scaling proceeds to change the clocks and gear. I wonder if the IO (req1) would be issued while scaling is in progress. If so, do you think a check should be added in ufshcd_queuecommand() to see if scaling is in progress or if host is blocked? > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index c4cf5c4b4893..3e4c62c6f9d2 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -1196,6 +1196,13 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba) > /* let's not get into low power until clock scaling is completed */ > ufshcd_hold(hba, false); > > + /* > + * Wait for ongoing ufshcd_queuecommand() calls. Calling > + * synchronize_rcu_expedited() instead of synchronize_rcu() reduces the > + * waiting time from milliseconds to microseconds. > + */ > + synchronize_rcu_expedited(); > + > out: > return ret; > } > @@ -2681,9 +2688,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) > > WARN_ONCE(tag < 0, "Invalid tag %d\n", tag); > > - if (!down_read_trylock(&hba->clk_scaling_lock)) > - return SCSI_MLQUEUE_HOST_BUSY; > - > /* > * Allows the UFS error handler to wait for prior ufshcd_queuecommand() > * calls. > @@ -2772,8 +2776,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) > out: > rcu_read_unlock(); > > - up_read(&hba->clk_scaling_lock); > - > if (ufs_trigger_eh()) { > unsigned long flags; > > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index c3c2792f309f..411c6015bbfe 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -779,6 +779,7 @@ struct ufs_hba_monitor { > * @clk_list_head: UFS host controller clocks list node head > * @pwr_info: holds current power mode > * @max_pwr_info: keeps the device max valid pwm > + * @clk_scaling_lock: used to serialize device commands and clock scaling > * @desc_size: descriptor sizes reported by device > * @urgent_bkops_lvl: keeps track of urgent bkops level for device > * @is_urgent_bkops_lvl_checked: keeps track if the urgent bkops level for >
On 12/1/21 3:33 PM, Asutosh Das (asd) wrote: > Hi Bart, > Say an IO (req1) has crossed the scsi_host_queue_ready() check but hasn't yet reached ufshcd_queuecommand() and DBR is 0. > ufshcd_clock_scaling_prepare() is invoked and completes and scaling proceeds to change the clocks and gear. > I wonder if the IO (req1) would be issued while scaling is in progress. > If so, do you think a check should be added in ufshcd_queuecommand() to see if scaling is in progress or if host is blocked? How about using the patch below instead of patch 16/17 from this patch series? The patch below should not trigger the race condition mentioned above. Thanks, Bart. Subject: [PATCH] scsi: ufs: Optimize the command queueing code Remove the clock scaling lock from ufshcd_queuecommand() since it is a performance bottleneck. Instead, wait until all budget_maps have cleared to wait for ongoing ufshcd_queuecommand() calls. Cc: Asutosh Das (asd) <asutoshd@codeaurora.org> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/ufs/ufshcd.c | 33 +++++++++++++++++++++++---------- drivers/scsi/ufs/ufshcd.h | 1 + 2 files changed, 24 insertions(+), 10 deletions(-) diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index c4cf5c4b4893..be679f2a97da 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -1070,13 +1070,31 @@ static bool ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba, return false; } +/* + * Determine the number of pending commands by counting the bits in the SCSI + * device budget maps. This approach has been selected because a bit is set in + * the budget map before scsi_host_queue_ready() checks the host_self_blocked + * flag. The host_self_blocked flag can be modified by calling + * scsi_block_requests() or scsi_unblock_requests(). + */ +static u32 ufshcd_pending_cmds(struct ufs_hba *hba) +{ + struct scsi_device *sdev; + u32 pending; + + shost_for_each_device(sdev, hba->host) + pending += sbitmap_weight(&sdev->budget_map); + + return pending; +} + static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba, u64 wait_timeout_us) { unsigned long flags; int ret = 0; u32 tm_doorbell; - u32 tr_doorbell; + u32 tr_pending; bool timeout = false, do_last_check = false; ktime_t start; @@ -1094,8 +1112,8 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba, } tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL); - tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL); - if (!tm_doorbell && !tr_doorbell) { + tr_pending = ufshcd_pending_cmds(hba); + if (!tm_doorbell && !tr_pending) { timeout = false; break; } else if (do_last_check) { @@ -1115,12 +1133,12 @@ static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba, do_last_check = true; } spin_lock_irqsave(hba->host->host_lock, flags); - } while (tm_doorbell || tr_doorbell); + } while (tm_doorbell || tr_pending); if (timeout) { dev_err(hba->dev, "%s: timedout waiting for doorbell to clear (tm=0x%x, tr=0x%x)\n", - __func__, tm_doorbell, tr_doorbell); + __func__, tm_doorbell, tr_pending); ret = -EBUSY; } out: @@ -2681,9 +2699,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) WARN_ONCE(tag < 0, "Invalid tag %d\n", tag); - if (!down_read_trylock(&hba->clk_scaling_lock)) - return SCSI_MLQUEUE_HOST_BUSY; - /* * Allows the UFS error handler to wait for prior ufshcd_queuecommand() * calls. @@ -2772,8 +2787,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) out: rcu_read_unlock(); - up_read(&hba->clk_scaling_lock); - if (ufs_trigger_eh()) { unsigned long flags; diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index c3c2792f309f..411c6015bbfe 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -779,6 +779,7 @@ struct ufs_hba_monitor { * @clk_list_head: UFS host controller clocks list node head * @pwr_info: holds current power mode * @max_pwr_info: keeps the device max valid pwm + * @clk_scaling_lock: used to serialize device commands and clock scaling * @desc_size: descriptor sizes reported by device * @urgent_bkops_lvl: keeps track of urgent bkops level for device * @is_urgent_bkops_lvl_checked: keeps track if the urgent bkops level for
On 12/2/21 10:13 AM, Bart Van Assche wrote: > +static u32 ufshcd_pending_cmds(struct ufs_hba *hba) > +{ > + struct scsi_device *sdev; > + u32 pending; ^^^^^^^^^^^ (replying to my own email) The above should be changed into "u32 pending = 0;" Thanks, Bart.
On 12/2/2021 10:13 AM, Bart Van Assche wrote: > On 12/1/21 3:33 PM, Asutosh Das (asd) wrote: >> Hi Bart, >> Say an IO (req1) has crossed the scsi_host_queue_ready() check but >> hasn't yet reached ufshcd_queuecommand() and DBR is 0. >> ufshcd_clock_scaling_prepare() is invoked and completes and scaling >> proceeds to change the clocks and gear. >> I wonder if the IO (req1) would be issued while scaling is in progress. >> If so, do you think a check should be added in ufshcd_queuecommand() >> to see if scaling is in progress or if host is blocked? > > How about using the patch below instead of patch 16/17 from this patch > series? The patch below should not trigger the race condition mentioned > above. > > Thanks, > > Bart. > Hi Bart, This looks like it'd work. -asd > > Subject: [PATCH] scsi: ufs: Optimize the command queueing code > > Remove the clock scaling lock from ufshcd_queuecommand() since it is a > performance bottleneck. Instead, wait until all budget_maps have cleared > to wait for ongoing ufshcd_queuecommand() calls. > > Cc: Asutosh Das (asd) <asutoshd@codeaurora.org> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/ufs/ufshcd.c | 33 +++++++++++++++++++++++---------- > drivers/scsi/ufs/ufshcd.h | 1 + > 2 files changed, 24 insertions(+), 10 deletions(-) > > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c > index c4cf5c4b4893..be679f2a97da 100644 > --- a/drivers/scsi/ufs/ufshcd.c > +++ b/drivers/scsi/ufs/ufshcd.c > @@ -1070,13 +1070,31 @@ static bool > ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba, > return false; > } > > +/* > + * Determine the number of pending commands by counting the bits in the > SCSI > + * device budget maps. This approach has been selected because a bit is > set in > + * the budget map before scsi_host_queue_ready() checks the > host_self_blocked > + * flag. The host_self_blocked flag can be modified by calling > + * scsi_block_requests() or scsi_unblock_requests(). > + */ > +static u32 ufshcd_pending_cmds(struct ufs_hba *hba) > +{ > + struct scsi_device *sdev; > + u32 pending; > + > + shost_for_each_device(sdev, hba->host) > + pending += sbitmap_weight(&sdev->budget_map); > + > + return pending; > +} > + > static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba, > u64 wait_timeout_us) > { > unsigned long flags; > int ret = 0; > u32 tm_doorbell; > - u32 tr_doorbell; > + u32 tr_pending; > bool timeout = false, do_last_check = false; > ktime_t start; > > @@ -1094,8 +1112,8 @@ static int ufshcd_wait_for_doorbell_clr(struct > ufs_hba *hba, > } > > tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL); > - tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL); > - if (!tm_doorbell && !tr_doorbell) { > + tr_pending = ufshcd_pending_cmds(hba); > + if (!tm_doorbell && !tr_pending) { > timeout = false; > break; > } else if (do_last_check) { > @@ -1115,12 +1133,12 @@ static int ufshcd_wait_for_doorbell_clr(struct > ufs_hba *hba, > do_last_check = true; > } > spin_lock_irqsave(hba->host->host_lock, flags); > - } while (tm_doorbell || tr_doorbell); > + } while (tm_doorbell || tr_pending); > > if (timeout) { > dev_err(hba->dev, > "%s: timedout waiting for doorbell to clear (tm=0x%x, > tr=0x%x)\n", > - __func__, tm_doorbell, tr_doorbell); > + __func__, tm_doorbell, tr_pending); > ret = -EBUSY; > } > out: > @@ -2681,9 +2699,6 @@ static int ufshcd_queuecommand(struct Scsi_Host > *host, struct scsi_cmnd *cmd) > > WARN_ONCE(tag < 0, "Invalid tag %d\n", tag); > > - if (!down_read_trylock(&hba->clk_scaling_lock)) > - return SCSI_MLQUEUE_HOST_BUSY; > - > /* > * Allows the UFS error handler to wait for prior > ufshcd_queuecommand() > * calls. > @@ -2772,8 +2787,6 @@ static int ufshcd_queuecommand(struct Scsi_Host > *host, struct scsi_cmnd *cmd) > out: > rcu_read_unlock(); > > - up_read(&hba->clk_scaling_lock); > - > if (ufs_trigger_eh()) { > unsigned long flags; > > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h > index c3c2792f309f..411c6015bbfe 100644 > --- a/drivers/scsi/ufs/ufshcd.h > +++ b/drivers/scsi/ufs/ufshcd.h > @@ -779,6 +779,7 @@ struct ufs_hba_monitor { > * @clk_list_head: UFS host controller clocks list node head > * @pwr_info: holds current power mode > * @max_pwr_info: keeps the device max valid pwm > + * @clk_scaling_lock: used to serialize device commands and clock scaling > * @desc_size: descriptor sizes reported by device > * @urgent_bkops_lvl: keeps track of urgent bkops level for device > * @is_urgent_bkops_lvl_checked: keeps track if the urgent bkops level > for
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index c4cf5c4b4893..3e4c62c6f9d2 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -1196,6 +1196,13 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba) /* let's not get into low power until clock scaling is completed */ ufshcd_hold(hba, false); + /* + * Wait for ongoing ufshcd_queuecommand() calls. Calling + * synchronize_rcu_expedited() instead of synchronize_rcu() reduces the + * waiting time from milliseconds to microseconds. + */ + synchronize_rcu_expedited(); + out: return ret; } @@ -2681,9 +2688,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) WARN_ONCE(tag < 0, "Invalid tag %d\n", tag); - if (!down_read_trylock(&hba->clk_scaling_lock)) - return SCSI_MLQUEUE_HOST_BUSY; - /* * Allows the UFS error handler to wait for prior ufshcd_queuecommand() * calls. @@ -2772,8 +2776,6 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd) out: rcu_read_unlock(); - up_read(&hba->clk_scaling_lock); - if (ufs_trigger_eh()) { unsigned long flags; diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h index c3c2792f309f..411c6015bbfe 100644 --- a/drivers/scsi/ufs/ufshcd.h +++ b/drivers/scsi/ufs/ufshcd.h @@ -779,6 +779,7 @@ struct ufs_hba_monitor { * @clk_list_head: UFS host controller clocks list node head * @pwr_info: holds current power mode * @max_pwr_info: keeps the device max valid pwm + * @clk_scaling_lock: used to serialize device commands and clock scaling * @desc_size: descriptor sizes reported by device * @urgent_bkops_lvl: keeps track of urgent bkops level for device * @is_urgent_bkops_lvl_checked: keeps track if the urgent bkops level for
Remove the clock scaling lock from ufshcd_queuecommand() since it is a performance bottleneck. Instead, use synchronize_rcu_expedited() to wait for ongoing ufshcd_queuecommand() calls. Cc: Asutosh Das (asd) <asutoshd@codeaurora.org> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/ufs/ufshcd.c | 12 +++++++----- drivers/scsi/ufs/ufshcd.h | 1 + 2 files changed, 8 insertions(+), 5 deletions(-)