diff mbox series

scsi: ufs: add a quirk to disable FUA support

Message ID 20220531201053.3300018-1-jaegeuk@kernel.org (mailing list archive)
State Changes Requested
Headers show
Series scsi: ufs: add a quirk to disable FUA support | expand

Commit Message

Jaegeuk Kim May 31, 2022, 8:10 p.m. UTC
UFS stack shows very low performance of FUA comparing to write and cache_flush.
Let's add a quirk to adjust it.

E.g., average latency according to the chunk size of write

Write(us/KB)	4	64	256	1024	2048
FUA		873.792	754.604	995.624	1011.67	1067.99
CACHE_FLUSH	824.703	712.98	800.307	1019.5	1037.37

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 drivers/scsi/ufs/ufshcd.c | 3 +++
 drivers/scsi/ufs/ufshcd.h | 5 +++++
 2 files changed, 8 insertions(+)

Comments

Eric Biggers May 31, 2022, 8:34 p.m. UTC | #1
On Tue, May 31, 2022 at 01:10:53PM -0700, Jaegeuk Kim wrote:
> UFS stack shows very low performance of FUA comparing to write and cache_flush.
> Let's add a quirk to adjust it.
> 
> E.g., average latency according to the chunk size of write
> 
> Write(us/KB)	4	64	256	1024	2048
> FUA		873.792	754.604	995.624	1011.67	1067.99
> CACHE_FLUSH	824.703	712.98	800.307	1019.5	1037.37
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 3 +++
>  drivers/scsi/ufs/ufshcd.h | 5 +++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 3f9caafa91bf..811f3467879c 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5035,6 +5035,9 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
>  	 */
>  	sdev->silence_suspend = 1;
>  
> +	if (hba->quirks & UFSHCD_QUIRK_BROKEN_FUA)
> +		sdev->broken_fua = 1;
> +
>  	ufshcd_crypto_register(hba, q);
>  
>  	return 0;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 94f545be183a..6c480c6741d6 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -602,6 +602,11 @@ enum ufshcd_quirks {
>  	 * support physical host configuration.
>  	 */
>  	UFSHCD_QUIRK_SKIP_PH_CONFIGURATION		= 1 << 16,
> +
> +	/*
> +	 * This quirk disables FUA support.
> +	 */
> +	UFSHCD_QUIRK_BROKEN_FUA				= 1 << 17,
>  };

"Broken" is ambiguous.  IIUC, the issue is that FUA performance is very bad, not
that it doesn't work.  Can you clarify the intent in the comment?

Also, this patch does nothing by itself.  Which UFS host driver(s) need this
quirk bit?  Can you update them to use it?  Or do they all need this, in which
case a quirk bit would be unnecessary?

- Eric
Jaegeuk Kim May 31, 2022, 8:53 p.m. UTC | #2
On 05/31, Eric Biggers wrote:
> On Tue, May 31, 2022 at 01:10:53PM -0700, Jaegeuk Kim wrote:
> > UFS stack shows very low performance of FUA comparing to write and cache_flush.
> > Let's add a quirk to adjust it.
> > 
> > E.g., average latency according to the chunk size of write
> > 
> > Write(us/KB)	4	64	256	1024	2048
> > FUA		873.792	754.604	995.624	1011.67	1067.99
> > CACHE_FLUSH	824.703	712.98	800.307	1019.5	1037.37
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 3 +++
> >  drivers/scsi/ufs/ufshcd.h | 5 +++++
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 3f9caafa91bf..811f3467879c 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -5035,6 +5035,9 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
> >  	 */
> >  	sdev->silence_suspend = 1;
> >  
> > +	if (hba->quirks & UFSHCD_QUIRK_BROKEN_FUA)
> > +		sdev->broken_fua = 1;
> > +
> >  	ufshcd_crypto_register(hba, q);
> >  
> >  	return 0;
> > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> > index 94f545be183a..6c480c6741d6 100644
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -602,6 +602,11 @@ enum ufshcd_quirks {
> >  	 * support physical host configuration.
> >  	 */
> >  	UFSHCD_QUIRK_SKIP_PH_CONFIGURATION		= 1 << 16,
> > +
> > +	/*
> > +	 * This quirk disables FUA support.
> > +	 */
> > +	UFSHCD_QUIRK_BROKEN_FUA				= 1 << 17,
> >  };
> 
> "Broken" is ambiguous.  IIUC, the issue is that FUA performance is very bad, not
> that it doesn't work.  Can you clarify the intent in the comment?

My intent is FUA was supposed to be better than write+cache_flush.

> 
> Also, this patch does nothing by itself.  Which UFS host driver(s) need this
> quirk bit?  Can you update them to use it?  Or do they all need this, in which
> case a quirk bit would be unnecessary?

Likewise other quick bits, using this is up to SoC or UFS vendors. I
think that combination is up to OEMs who is building the product.

> 
> - Eric
Eric Biggers May 31, 2022, 9:25 p.m. UTC | #3
On Tue, May 31, 2022 at 01:53:33PM -0700, Jaegeuk Kim wrote:
> > Also, this patch does nothing by itself.  Which UFS host driver(s) need this
> > quirk bit?  Can you update them to use it?  Or do they all need this, in which
> > case a quirk bit would be unnecessary?
> 
> Likewise other quick bits, using this is up to SoC or UFS vendors. I
> think that combination is up to OEMs who is building the product.

Of the UFS host drivers in the upstream kernel, which ones actually need this?

- Eric
Adrian Hunter June 1, 2022, 5:24 a.m. UTC | #4
On 31/05/22 23:10, Jaegeuk Kim wrote:
> UFS stack shows very low performance of FUA comparing to write and cache_flush.
> Let's add a quirk to adjust it.
> 
> E.g., average latency according to the chunk size of write
> 
> Write(us/KB)	4	64	256	1024	2048
> FUA		873.792	754.604	995.624	1011.67	1067.99
> CACHE_FLUSH	824.703	712.98	800.307	1019.5	1037.37

Wouldn't it depend on how much data might be in the cache?
Do you have real-world use-cases where the difference is measurable?

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 3 +++
>  drivers/scsi/ufs/ufshcd.h | 5 +++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 3f9caafa91bf..811f3467879c 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -5035,6 +5035,9 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
>  	 */
>  	sdev->silence_suspend = 1;
>  
> +	if (hba->quirks & UFSHCD_QUIRK_BROKEN_FUA)
> +		sdev->broken_fua = 1;
> +
>  	ufshcd_crypto_register(hba, q);
>  
>  	return 0;
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 94f545be183a..6c480c6741d6 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -602,6 +602,11 @@ enum ufshcd_quirks {
>  	 * support physical host configuration.
>  	 */
>  	UFSHCD_QUIRK_SKIP_PH_CONFIGURATION		= 1 << 16,
> +
> +	/*
> +	 * This quirk disables FUA support.
> +	 */
> +	UFSHCD_QUIRK_BROKEN_FUA				= 1 << 17,

Wouldn't it be more appropriate to make it a UFS_DEVICE_QUIRK_
since it presumably depends on the UFS device not the host controller?

Also, as already commented by others, there needs to be a user of
the quirk

>  };
>  
>  enum ufshcd_caps {
Bart Van Assche June 1, 2022, 11:59 a.m. UTC | #5
On 5/31/22 22:24, Adrian Hunter wrote:
> On 31/05/22 23:10, Jaegeuk Kim wrote:
>> +	/*
>> +	 * This quirk disables FUA support.
>> +	 */
>> +	UFSHCD_QUIRK_BROKEN_FUA				= 1 << 17,
> 
> Wouldn't it be more appropriate to make it a UFS_DEVICE_QUIRK_
> since it presumably depends on the UFS device not the host controller?
> 
> Also, as already commented by others, there needs to be a user of
> the quirk

Another possibility is to use the generic SCSI blacklist mechanism. See 
also the scsi_static_device_list array. See also /proc/scsi/device_info. 
 From scsi_devinfo.c:

/*
  * proc_scsi_dev_info_write - allow additions to scsi_dev_info_list via
  * /proc.
  *
  * Description: Adds a black/white list entry for vendor and model with
  * an integer value of flag to the scsi device info list.
  * To use, echo "vendor:model:flag" > /proc/scsi/device_info
  */

Thanks,

Bart.
Jaegeuk Kim June 1, 2022, 5:05 p.m. UTC | #6
On 06/01, Adrian Hunter wrote:
> On 31/05/22 23:10, Jaegeuk Kim wrote:
> > UFS stack shows very low performance of FUA comparing to write and cache_flush.
> > Let's add a quirk to adjust it.
> > 
> > E.g., average latency according to the chunk size of write
> > 
> > Write(us/KB)	4	64	256	1024	2048
> > FUA		873.792	754.604	995.624	1011.67	1067.99
> > CACHE_FLUSH	824.703	712.98	800.307	1019.5	1037.37
> 
> Wouldn't it depend on how much data might be in the cache?

I've got this average latency from 100 commands of write+cache_flush vs.
write(FUA). I think the cached data should be the same as this chunk
size.

> Do you have real-world use-cases where the difference is measurable?

I'm approaching this based on 1) f2fs uses FUA for checkpoint and fsync,
and 2) iomap uses FUA for O_DIRECT|O_DSYNC case [1].

[1] https://lore.kernel.org/lkml/20220527205955.3251982-1-jaegeuk@kernel.org/

> 
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  drivers/scsi/ufs/ufshcd.c | 3 +++
> >  drivers/scsi/ufs/ufshcd.h | 5 +++++
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> > index 3f9caafa91bf..811f3467879c 100644
> > --- a/drivers/scsi/ufs/ufshcd.c
> > +++ b/drivers/scsi/ufs/ufshcd.c
> > @@ -5035,6 +5035,9 @@ static int ufshcd_slave_configure(struct scsi_device *sdev)
> >  	 */
> >  	sdev->silence_suspend = 1;
> >  
> > +	if (hba->quirks & UFSHCD_QUIRK_BROKEN_FUA)
> > +		sdev->broken_fua = 1;
> > +
> >  	ufshcd_crypto_register(hba, q);
> >  
> >  	return 0;
> > diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> > index 94f545be183a..6c480c6741d6 100644
> > --- a/drivers/scsi/ufs/ufshcd.h
> > +++ b/drivers/scsi/ufs/ufshcd.h
> > @@ -602,6 +602,11 @@ enum ufshcd_quirks {
> >  	 * support physical host configuration.
> >  	 */
> >  	UFSHCD_QUIRK_SKIP_PH_CONFIGURATION		= 1 << 16,
> > +
> > +	/*
> > +	 * This quirk disables FUA support.
> > +	 */
> > +	UFSHCD_QUIRK_BROKEN_FUA				= 1 << 17,
> 
> Wouldn't it be more appropriate to make it a UFS_DEVICE_QUIRK_
> since it presumably depends on the UFS device not the host controller?
> 
> Also, as already commented by others, there needs to be a user of
> the quirk

Since I asked SoC vendors can verify the performance with this quirk,
I need to wait for their reply. Meanwhile, I'm willing to disable FUA in Pixel
devices, which I cannot post any patch directly to LKML.

Agreed that, if there's no other user in upstream, I'm okay to drop
this.

> 
> >  };
> >  
> >  enum ufshcd_caps {
Jaegeuk Kim June 1, 2022, 5:06 p.m. UTC | #7
On 06/01, Bart Van Assche wrote:
> On 5/31/22 22:24, Adrian Hunter wrote:
> > On 31/05/22 23:10, Jaegeuk Kim wrote:
> > > +	/*
> > > +	 * This quirk disables FUA support.
> > > +	 */
> > > +	UFSHCD_QUIRK_BROKEN_FUA				= 1 << 17,
> > 
> > Wouldn't it be more appropriate to make it a UFS_DEVICE_QUIRK_
> > since it presumably depends on the UFS device not the host controller?
> > 
> > Also, as already commented by others, there needs to be a user of
> > the quirk
> 
> Another possibility is to use the generic SCSI blacklist mechanism. See also
> the scsi_static_device_list array. See also /proc/scsi/device_info. From
> scsi_devinfo.c:
> 
> /*
>  * proc_scsi_dev_info_write - allow additions to scsi_dev_info_list via
>  * /proc.
>  *
>  * Description: Adds a black/white list entry for vendor and model with
>  * an integer value of flag to the scsi device info list.
>  * To use, echo "vendor:model:flag" > /proc/scsi/device_info
>  */

Good to know. Thank you, Bart.

> 
> Thanks,
> 
> Bart.
Asutosh Das June 7, 2022, 4:45 p.m. UTC | #8
On Tue, May 31 2022 at 14:25 -0700, Eric Biggers wrote:
>On Tue, May 31, 2022 at 01:53:33PM -0700, Jaegeuk Kim wrote:
>> > Also, this patch does nothing by itself.  Which UFS host driver(s) need this
>> > quirk bit?  Can you update them to use it?  Or do they all need this, in which
>> > case a quirk bit would be unnecessary?
>>
>> Likewise other quick bits, using this is up to SoC or UFS vendors. I
>> think that combination is up to OEMs who is building the product.
>
>Of the UFS host drivers in the upstream kernel, which ones actually need this?
>

Qualcomm UFSHC would need this.
>- Eric
diff mbox series

Patch

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 3f9caafa91bf..811f3467879c 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -5035,6 +5035,9 @@  static int ufshcd_slave_configure(struct scsi_device *sdev)
 	 */
 	sdev->silence_suspend = 1;
 
+	if (hba->quirks & UFSHCD_QUIRK_BROKEN_FUA)
+		sdev->broken_fua = 1;
+
 	ufshcd_crypto_register(hba, q);
 
 	return 0;
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 94f545be183a..6c480c6741d6 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -602,6 +602,11 @@  enum ufshcd_quirks {
 	 * support physical host configuration.
 	 */
 	UFSHCD_QUIRK_SKIP_PH_CONFIGURATION		= 1 << 16,
+
+	/*
+	 * This quirk disables FUA support.
+	 */
+	UFSHCD_QUIRK_BROKEN_FUA				= 1 << 17,
 };
 
 enum ufshcd_caps {