This patch adds a host_lock which existed before on ufshcd_vops_setup_xfer_req.
Cc: Stanley Chu <[email protected]>
Cc: Can Guo <[email protected]>
Cc: Bean Huo <[email protected]>
Cc: Bart Van Assche <[email protected]>
Cc: Asutosh Das <[email protected]>
Fixes: a45f937110fa ("scsi: ufs: Optimize host lock on transfer requests send/compl paths")
Signed-off-by: Jaegeuk Kim <[email protected]>
---
drivers/scsi/ufs/ufshcd.h | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index c98d540ac044..194755c9ddfe 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -1229,8 +1229,13 @@ static inline int ufshcd_vops_pwr_change_notify(struct ufs_hba *hba,
static inline void ufshcd_vops_setup_xfer_req(struct ufs_hba *hba, int tag,
bool is_scsi_cmd)
{
- if (hba->vops && hba->vops->setup_xfer_req)
- return hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
+ if (hba->vops && hba->vops->setup_xfer_req) {
+ unsigned long flags;
+
+ spin_lock_irqsave(hba->host->host_lock, flags);
+ hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
+ spin_unlock_irqrestore(hba->host->host_lock, flags);
+ }
}
static inline void ufshcd_vops_setup_task_mgmt(struct ufs_hba *hba,
--
2.32.0.93.g670b81a890-goog
On 6/30/21 5:51 PM, Jaegeuk Kim wrote:
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index c98d540ac044..194755c9ddfe 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -1229,8 +1229,13 @@ static inline int ufshcd_vops_pwr_change_notify(struct ufs_hba *hba,
> static inline void ufshcd_vops_setup_xfer_req(struct ufs_hba *hba, int tag,
> bool is_scsi_cmd)
> {
> - if (hba->vops && hba->vops->setup_xfer_req)
> - return hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
> + if (hba->vops && hba->vops->setup_xfer_req) {
> + unsigned long flags;
> +
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> + }
> }
Since this function has only one caller, how about moving it into ufshcd.c?
Thanks,
Bart.
On 7/1/21 8:23 AM, Bart Van Assche wrote:
> On 6/30/21 5:51 PM, Jaegeuk Kim wrote:
>> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
>> index c98d540ac044..194755c9ddfe 100644
>> --- a/drivers/scsi/ufs/ufshcd.h
>> +++ b/drivers/scsi/ufs/ufshcd.h
>> @@ -1229,8 +1229,13 @@ static inline int ufshcd_vops_pwr_change_notify(struct ufs_hba *hba,
>> static inline void ufshcd_vops_setup_xfer_req(struct ufs_hba *hba, int tag,
>> bool is_scsi_cmd)
>> {
>> - if (hba->vops && hba->vops->setup_xfer_req)
>> - return hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
>> + if (hba->vops && hba->vops->setup_xfer_req) {
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(hba->host->host_lock, flags);
>> + hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
>> + spin_unlock_irqrestore(hba->host->host_lock, flags);
>> + }
>> }
>
> Since this function has only one caller, how about moving it into ufshcd.c?
(replying to my own email)
Since I just noticed that there are many other similar function
definitions in ufshcd.h, let's postpone moving these definitions until a
later time.
Reviewed-by: Bart Van Assche <[email protected]>
On 6/30/21 5:51 PM, Jaegeuk Kim wrote:
> This patch adds a host_lock which existed before on ufshcd_vops_setup_xfer_req.
>
> Cc: Stanley Chu <[email protected]>
> Cc: Can Guo <[email protected]>
> Cc: Bean Huo <[email protected]>
> Cc: Bart Van Assche <[email protected]>
> Cc: Asutosh Das <[email protected]>
> Fixes: a45f937110fa ("scsi: ufs: Optimize host lock on transfer requests send/compl paths")
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> drivers/scsi/ufs/ufshcd.h | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index c98d540ac044..194755c9ddfe 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -1229,8 +1229,13 @@ static inline int ufshcd_vops_pwr_change_notify(struct ufs_hba *hba,
> static inline void ufshcd_vops_setup_xfer_req(struct ufs_hba *hba, int tag,
> bool is_scsi_cmd)
> {
> - if (hba->vops && hba->vops->setup_xfer_req)
> - return hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
> + if (hba->vops && hba->vops->setup_xfer_req) {
> + unsigned long flags;
> +
> + spin_lock_irqsave(hba->host->host_lock, flags);
> + hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
> + spin_unlock_irqrestore(hba->host->host_lock, flags);
> + }
> }
>
> static inline void ufshcd_vops_setup_task_mgmt(struct ufs_hba *hba,
Can anyone help with reviewing this patch?
Thanks,
Bart.
On Tue, 2021-07-13 at 12:06 -0700, Bart Van Assche wrote:
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -1229,8 +1229,13 @@ static inline int
> > ufshcd_vops_pwr_change_notify(struct ufs_hba *hba,
> > static inline void ufshcd_vops_setup_xfer_req(struct ufs_hba
> > *hba, int tag,
> > bool is_scsi_cmd)
> > {
> > - if (hba->vops && hba->vops->setup_xfer_req)
> > - return hba->vops->setup_xfer_req(hba, tag,
> > is_scsi_cmd);
> > + if (hba->vops && hba->vops->setup_xfer_req) {
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(hba->host->host_lock, flags);
> > + hba->vops->setup_xfer_req(hba, tag, is_scsi_cmd);
> > + spin_unlock_irqrestore(hba->host->host_lock, flags);
> > + }
> > }
> >
> > static inline void ufshcd_vops_setup_task_mgmt(struct ufs_hba
> > *hba,
>
>
> Can anyone help with reviewing this patch?
>
>
>
> Thanks,
>
>
>
Hi Bart,
This change only impacts on the Samsung exynos platform. and Can's
optimization patch is to optimise the host_lock,, and removed
host_lock, now add back in this function makes sense to me.
but I am thinking how about hba->host_sem?
Bean
Bean
> Bart.
On 7/13/21 12:45 PM, Bean Huo wrote:
> This change only impacts on the Samsung exynos platform. and Can's
> optimization patch is to optimise the host_lock,, and removed
> host_lock, now add back in this function makes sense to me.
> but I am thinking how about hba->host_sem?
Hi Bean,
Calls of exynos_ufs_specify_nexus_t_xfer_req() must be serialized, hence
Jaegeuks' patch. This function is called from the I/O submission path so
performance matters. My understanding is that spinlocks have less
overhead than semaphores. Hence the choice for a spinlock.
Thanks,
Bart.
On Wed, 2021-07-14 at 11:09 -0700, Bart Van Assche wrote:
> On 7/13/21 12:45 PM, Bean Huo wrote:
>
> > This change only impacts on the Samsung exynos platform. and Can's
> > optimization patch is to optimise the host_lock,, and removed
> > host_lock, now add back in this function makes sense to me.
> > but I am thinking how about hba->host_sem?
>
>
> Hi Bean,
>
>
>
> Calls of exynos_ufs_specify_nexus_t_xfer_req() must be serialized,
> hence
>
> Jaegeuks' patch. This function is called from the I/O submission path
> so
>
> performance matters. My understanding is that spinlocks have less
>
> overhead than semaphores. Hence the choice for a spinlock.
>
>
>
> Thanks,
>
Bart,
After adding spin_lock/unlock_irqsave()
in ufshcd_vops_setup_xfer_req(), there will be 4 times of call of
host_lock lock/unlock in ufshcd_send_command(). Reduce the code scope
of protection, but increase the number of calls to
spin_lock/unlock_irqsave(). Almost each sub-funciton
in ufshcd_send_command() calls spin_lock/unlock_irqsave(). why not
directly take spin_lock/unlock_irqsave() away from each sub-fun, and
increase the scope in ufshcd_send_command()?
Bean
> Bart