| Message ID | 20220531201053.3300018-1-jaegeuk@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: ufs: add a quirk to disable FUA support | expand |
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
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
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
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 {
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.
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 {
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.
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 --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 {
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(+)