[v9,0/4] scsi: ufs: Add Host Performance Booster Support

Changelog:

v8 -> v9
1. Change sysfs initialization.
2. Change reading descriptor during HPB initialization
3. Fix problems commentted in Bart's review.
4. Change base commit from 5.9/scsi-queue to 5.10/scsi-queue.

v7 -> v8
Remove wrongly added tags.

v6 -> v7
1. Remove UFS feature layer.
2. Cleanup for sparse error.

v5 -> v6
Change base commit to b53293fa662e28ae0cdd40828dc641c09f133405

v4 -> v5
Delete unused macro define.

v3 -> v4
1. Cleanup.

v2 -> v3
1. Add checking input module parameter value.
2. Change base commit from 5.8/scsi-queue to 5.9/scsi-queue.
3. Cleanup for unused variables and label.

v1 -> v2
1. Change the full boilerplate text to SPDX style.
2. Adopt dynamic allocation for sub-region data structure.
3. Cleanup.

NAND flash memory-based storage devices use Flash Translation Layer (FTL)
to translate logical addresses of I/O requests to corresponding flash
memory addresses. Mobile storage devices typically have RAM with
constrained size, thus lack in memory to keep the whole mapping table.
Therefore, mapping tables are partially retrieved from NAND flash on
demand, causing random-read performance degradation.

To improve random read performance, JESD220-3 (HPB v1.0) proposes HPB
(Host Performance Booster) which uses host system memory as a cache for the
FTL mapping table. By using HPB, FTL data can be read from host memory
faster than from NAND flash memory. 

The current version only supports the DCM (device control mode).
This patch consists of 3 parts to support HPB feature.

1) HPB probe and initialization process
2) READ -> HPB READ using cached map information
3) L2P (logical to physical) map management

In the HPB probe and init process, the device information of the UFS is
queried. After checking supported features, the data structure for the HPB
is initialized according to the device information.

A read I/O in the active sub-region where the map is cached is changed to
HPB READ by the HPB.

The HPB manages the L2P map using information received from the
device. For active sub-region, the HPB caches through ufshpb_map
request. For the in-active region, the HPB discards the L2P map.
When a write I/O occurs in an active sub-region area, associated dirty
bitmap checked as dirty for preventing stale read.

HPB is shown to have a performance improvement of 58 - 67% for random read
workload. [1]

This series patches are based on the 5.9/scsi-queue branch.

[1]:
https://www.usenix.org/conference/hotstorage17/program/presentation/jeong

Daejun park (4):
 scsi: ufs: Add HPB feature related parameters
 scsi: ufs: Introduce HPB feature
 scsi: ufs: L2P map management for HPB read
 scsi: ufs: Prepare HPB read for cached sub-region
 
 drivers/scsi/ufs/Kconfig  |    9 +
 drivers/scsi/ufs/Makefile |    1 +
 drivers/scsi/ufs/ufs.h    |   47 ++
 drivers/scsi/ufs/ufshcd.c |   58 ++
 drivers/scsi/ufs/ufshcd.h |   22 +-
 drivers/scsi/ufs/ufshpb.c | 1817 ++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/ufs/ufshpb.h |  231 +++++
 7 files changed, 2184 insertions(+), 1 deletion(-)
 created mode 100644 drivers/scsi/ufs/ufshpb.c
 created mode 100644 drivers/scsi/ufs/ufshpb.h

On 2020-08-28 00:18, Daejun Park wrote:
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index 618b253e5ec8..df30622a2b67 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -588,6 +588,24 @@ struct ufs_hba_variant_params {
>  	u16 hba_enable_delay_us;
>  	u32 wb_flush_threshold;
>  };
> +#ifdef CONFIG_SCSI_UFS_HPB
> +/**
> + * struct ufshpb_dev_info - UFSHPB device related info
> + * @num_lu: the number of user logical unit to check whether all lu finished
> + *          initialization
> + * @rgn_size: device reported HPB region size
> + * @srgn_size: device reported HPB sub-region size
> + * @slave_conf_cnt: counter to check all lu finished initialization
> + * @hpb_disabled: flag to check if HPB is disabled
> + */
> +struct ufshpb_dev_info {
> +	int num_lu;
> +	int rgn_size;
> +	int srgn_size;
> +	atomic_t slave_conf_cnt;
> +	bool hpb_disabled;
> +};
> +#endif

Please insert a blank line above "#ifdef CONFIG_SCSI_UFS_HPB"

> +/* HPB enabled lu list */
> +static LIST_HEAD(lh_hpb_lu);

Is it necessary to maintain this list? Or in other words, is it possible to
change all list_for_each_entry(hpb, &lh_hpb_lu, list_hpb_lu) calls into
shost_for_each_device() calls?

> +/* SYSFS functions */
> +#define ufshpb_sysfs_attr_show_func(__name)				\
> +static ssize_t __name##_show(struct device *dev,			\
> +	struct device_attribute *attr, char *buf)			\
> +{									\
> +	struct scsi_device *sdev = to_scsi_device(dev);			\
> +	struct ufshpb_lu *hpb = sdev->hostdata;				\
> +	if (!hpb)							\
> +		return -ENOENT;						\
> +	return snprintf(buf, PAGE_SIZE, "%d\n",				\
> +			atomic_read(&hpb->stats.__name));		\
> +}									\
> +static DEVICE_ATTR_RO(__name)

Please leave a blank line after declarations (between the "hpb" declaration
and "if (!hpb)").

> +#ifndef CONFIG_SCSI_UFS_HPB
> +[...]
> +static struct attribute *hpb_dev_attrs[] = { NULL,};
> +static struct attribute_group ufs_sysfs_hpb_stat_group = {.attrs = hpb_dev_attrs,};
> +#else
> +[...]
> +extern struct attribute_group ufs_sysfs_hpb_stat_group;
> +#endif

Defining static variables or arrays in header files is questionable because
the definition of these variables will be duplicated in each source file that
header file is included in. Although the general rule for kernel code is to
confine #ifdefs to header files, for ufs_sysfs_hpb_stat_group I think that
it is better to surround its use with #ifndef CONFIG_SCSI_UFS_HPB / #endif
instead of defining a dummy structure as static variable in a header file if
HPB support is disabled.

Otherwise this patch looks good to me.

Thanks,

Bart.