Re: [PATCH v2 5/5] scsi: ufs: UFS Host Performance Booster(HPB) driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 4/16/20 1:31 PM, huobean@xxxxxxxxx wrote:
This patch is to add support for the UFS Host Performance Booster (HPB v1.0),
which is used to improve UFS read performance, especially for the random read.

NAND flash-based storage devices, including UFS, have mechanisms to translate
logical addresses of requests to the corresponding physical addresses of the
flash storage. Traditionally this L2P mapping data is loaded to the internal
SRAM in the storage controller. When the capacity of storage is larger, a
larger size of SRAM for the L2P map data is required. Since increased SRAM
size affects the manufacturing cost significantly, it is not cost-effective
to allocate all the amount of SRAM needed to keep all the Logical-address to
Physical-address (L2P) map data. Therefore, L2P map data, which is required
to identify the physical address for the requested IOs, can only be partially
stored in SRAM from NAND flash. Due to this partial loading, accessing the
flash address area where the L2P information for that address is not loaded
in the SRAM can result in serious performance degradation.

The HPB is a software solution for the above problem, which uses the host’s
system memory as a cache for the FTL L2P mapping table. It does not need
additional hardware support from the host side. By using HPB, the L2P mapping
table can be read from host memory and stored in host-side memory. while
reading the operation, the corresponding L2P information will be sent to the
UFS device along with the reading request. Since the L2P entry is provided in
the read request, UFS device does not have to load L2P entry from flash memory.
This will significantly improve random read performance.

The above description is incomplete: what is missing is an explanation of how the L2P data is made persistent and also how it is stored on the device. Is an L2P update written to the device every time data on the device is modified or only during an orderly shutdown? In the former case, how big is the impact on write performance? In the latter case, does that mean that data is lost during an unorderly shutdown? Is HPB a feature that perhaps only works well for battery-powered devices?

Is the persistent L2P data overwritten in place or is new data appended as an update after existing data? In the former case, is all L2P data lost in case of a power failure? In the latter case, how is it guaranteed that enough free space is available to make the L2P data persistent?

What are the similarities and differences compared to the lightnvm framework that was added several years ago to the Linux kernel? Which of the code in this patch can be shared with the lightnvm framework?

+config UFSHPB_MAX_MEM_SIZE
+	int "UFS HPB maximum memory size per controller (in MiB)"
+	depends on SCSI_UFSHPB
+	default 128
+	range 0 65536
+	help
+	  This parameter defines the maximum UFS HPB memory/cache size in the
+	  host system. The recommended HPB cache size by the UFS device can be
+	  calculated from bHPBRegionSize and wDeviceMaxActiveHPBRegions. The
+	  reference formula can be
+
+		(bHPBRegionSize(in KB) / 4KB) * 8 * wDeviceMaxActiveHPBRegions.
+
+	  The HPB cache in the host system is used to contain L2P mapping entry.
+	  If the allocated HPB cache size is lower than what calculated by the
+	  above formula, the use of HPB feature may provide lower performance
+	  advantage. But the system memory resource has the limitation, we can
+	  not let HPB driver allocate its cache at will according to the UFS
+	  device recommendation, so an appropriate size of the cache for HPB
+	  should be specified before you choose to use HPB, then please input a
+	  non-zero positive integer value.
+
+	  Nevertheless, if you want to leave this right to the HPB driver, and
+	  let the HPB driver allocate the HPB cache based on the recommendation
+	  of the UFS device. Just give 0 value to this parameter.
+
+	  Leave the default value if unsure.

Can this parameter be changed from a compile-time option into something that is configurable at runtime, e.g. a sysfs or configfs attribute?

+#if defined(CONFIG_SCSI_UFSHPB)
+	if (sdev->lun < hba->dev_info.max_lu_supported)
+		hba->sdev_ufs_lu[sdev->lun] = sdev;
+#endif

Isn't maintaining a LUN to scsi_device mapping something that is already done by the SCSI core? See also __scsi_device_lookup().

+#if defined(CONFIG_SCSI_UFSHPB)
+			/*
+			 * HPB recommendations are provided in RESPONSE UPIU
+			 * packets of successfully completed commands, which
+			 * are commands terminated with GOOD status.
+			 */
+			if (scsi_status == SAM_STAT_GOOD)
+				ufshpb_rsp_handler(hba, lrbp);
+#endif

Please introduce helper functions such that no #if defined(CONFIG_SCSI_UFSHPB) statements are required in .c files. From Documentation/process/4.Coding.rst: "As a general rule, #ifdef use
should be confined to header files whenever possible."

@@ -6627,16 +6656,16 @@ static int ufs_get_device_desc(struct ufs_hba *hba)
dev_info->ufs_features = desc_buf[DEVICE_DESC_PARAM_UFS_FEAT]; - if (desc_buf[DEVICE_DESC_PARAM_UFS_FEAT] & 0x80) {
-		hba->dev_info.hpb_control_mode =
+	if (dev_info->ufs_features & 0x80) {

The above change should be moved into the combination of patches 1/5 and 3/5, and the changes below probably too.

+		dev_info->hpb_control_mode =
  			desc_buf[DEVICE_DESC_PARAM_HPB_CTRL_MODE];
-		hba->dev_info.hpb_ver =
+		dev_info->hpb_ver =
  			(u16) (desc_buf[DEVICE_DESC_PARAM_HPB_VER] << 8) |
  			desc_buf[DEVICE_DESC_PARAM_HPB_VER + 1];
  		dev_info(hba->dev, "HPB Version: 0x%2x\n",
-			 hba->dev_info.hpb_ver);
+			 dev_info->hpb_ver);
  		dev_info(hba->dev, "HPB control mode: %d\n",
-			 hba->dev_info.hpb_control_mode);
+			 dev_info->hpb_control_mode);
  	}

[ ... ]

+/* BYTE SHIFT */
+#define ZERO_BYTE_SHIFT			0
+#define ONE_BYTE_SHIFT			8
+#define TWO_BYTE_SHIFT			16
+#define THREE_BYTE_SHIFT		24
+#define FOUR_BYTE_SHIFT			32
+#define FIVE_BYTE_SHIFT			40
+#define SIX_BYTE_SHIFT			48
+#define SEVEN_BYTE_SHIFT		56
>
+#define SHIFT_BYTE_0(num)		((num) << ZERO_BYTE_SHIFT)
+#define SHIFT_BYTE_1(num)		((num) << ONE_BYTE_SHIFT)
+#define SHIFT_BYTE_2(num)		((num) << TWO_BYTE_SHIFT)
+#define SHIFT_BYTE_3(num)		((num) << THREE_BYTE_SHIFT)
+#define SHIFT_BYTE_4(num)		((num) << FOUR_BYTE_SHIFT)
+#define SHIFT_BYTE_5(num)		((num) << FIVE_BYTE_SHIFT)
+#define SHIFT_BYTE_6(num)		((num) << SIX_BYTE_SHIFT)
+#define SHIFT_BYTE_7(num)		((num) << SEVEN_BYTE_SHIFT)
>
+#define GET_BYTE_0(num)			(((num) >> ZERO_BYTE_SHIFT) & 0xff)
+#define GET_BYTE_1(num)			(((num) >> ONE_BYTE_SHIFT) & 0xff)
+#define GET_BYTE_2(num)			(((num) >> TWO_BYTE_SHIFT) & 0xff)
+#define GET_BYTE_3(num)			(((num) >> THREE_BYTE_SHIFT) & 0xff)
+#define GET_BYTE_4(num)			(((num) >> FOUR_BYTE_SHIFT) & 0xff)
+#define GET_BYTE_5(num)			(((num) >> FIVE_BYTE_SHIFT) & 0xff)
+#define GET_BYTE_6(num)			(((num) >> SIX_BYTE_SHIFT) & 0xff)
+#define GET_BYTE_7(num)			(((num) >> SEVEN_BYTE_SHIFT) & 0xff)

Please remove the above 24 macro definitions and use get_unaligned_*() / put_unaligned_*() instead.

Thanks,

Bart.



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux