lkml.org 
[lkml]   [2020]   [Apr]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v2 5/5] scsi: ufs: UFS Host Performance Booster(HPB) driver
    From
    Date
    On 4/16/20 1:31 PM, huobean@gmail.com 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.

    \
     
     \ /
      Last update: 2020-04-23 02:00    [W:5.945 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site