v3 -> v4:
- rebase on Daejun's v25
v2 -> v3:
- Attend Greg's and Can's comments
- rebase on Daejun's v21
v1 -> v2:
- attend Greg's and Daejun's comments
- add patch 9 making host mode parameters configurable
- rebase on Daejun's v19
The HPB spec defines 2 control modes - device control mode and host
control mode. In oppose to device control mode, in which the host obey
to whatever recommendation received from the device - In host control
mode, the host uses its own algorithms to decide which regions should
be activated or inactivated.
We kept the host managed heuristic simple and concise.
Aside from adding a by-spec functionality, host control mode entails
some further potential benefits: makes the hpb logic transparent and
readable, while allow tuning / scaling its various parameters, and
utilize system-wide info to optimize HPB potential.
This series is based on Samsung's V25 device-control HPB2.0 driver, see
msg-id: 20210226073233epcms2p80fca2dffabea03143a9414838f757633@epcms2p8
in lore.kernel.org.
This version was tested on Galaxy S20, and Xiaomi Mi10 pro.
Your meticulous review and testing is mostly welcome and appreciated.
Thanks,
Avri
Avri Altman (9):
scsi: ufshpb: Cache HPB Control mode on init
scsi: ufshpb: Add host control mode support to rsp_upiu
scsi: ufshpb: Add region's reads counter
scsi: ufshpb: Make eviction depends on region's reads
scsi: ufshpb: Region inactivation in host mode
scsi: ufshpb: Add hpb dev reset response
scsi: ufshpb: Add "Cold" regions timer
scsi: ufshpb: Add support for host control mode
scsi: ufshpb: Make host mode parameters configurable
Documentation/ABI/testing/sysfs-driver-ufs | 67 +++
drivers/scsi/ufs/ufshcd.h | 2 +
drivers/scsi/ufs/ufshpb.c | 528 ++++++++++++++++++++-
drivers/scsi/ufs/ufshpb.h | 40 ++
4 files changed, 613 insertions(+), 24 deletions(-)
--
2.25.1
I host mode, the host is expected to send HPB-WRITE-BUFFER with
buffer-id = 0x1 when it inactivates a region.
Use the map-requests pool as there is no point in assigning a
designated cache for umap-requests.
Signed-off-by: Avri Altman <[email protected]>
---
drivers/scsi/ufs/ufshpb.c | 14 ++++++++++++++
drivers/scsi/ufs/ufshpb.h | 1 +
2 files changed, 15 insertions(+)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 44e56a6d7102..cf704b82e72a 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -908,6 +908,7 @@ static int ufshpb_execute_umap_req(struct ufshpb_lu *hpb,
blk_execute_rq_nowait(q, NULL, req, 1, ufshpb_umap_req_compl_fn);
+ hpb->stats.umap_req_cnt++;
return 0;
}
@@ -1104,6 +1105,12 @@ static int ufshpb_issue_umap_req(struct ufshpb_lu *hpb,
return -EAGAIN;
}
+static int ufshpb_issue_umap_single_req(struct ufshpb_lu *hpb,
+ struct ufshpb_region *rgn)
+{
+ return ufshpb_issue_umap_req(hpb, rgn);
+}
+
static int ufshpb_issue_umap_all_req(struct ufshpb_lu *hpb)
{
return ufshpb_issue_umap_req(hpb, NULL);
@@ -1116,6 +1123,10 @@ static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
struct ufshpb_subregion *srgn;
int srgn_idx;
+
+ if (hpb->is_hcm && ufshpb_issue_umap_single_req(hpb, rgn))
+ return;
+
lru_info = &hpb->lru_info;
dev_dbg(&hpb->sdev_ufs_lu->sdev_dev, "evict region %d\n", rgn->rgn_idx);
@@ -1861,6 +1872,7 @@ ufshpb_sysfs_attr_show_func(rb_noti_cnt);
ufshpb_sysfs_attr_show_func(rb_active_cnt);
ufshpb_sysfs_attr_show_func(rb_inactive_cnt);
ufshpb_sysfs_attr_show_func(map_req_cnt);
+ufshpb_sysfs_attr_show_func(umap_req_cnt);
static struct attribute *hpb_dev_stat_attrs[] = {
&dev_attr_hit_cnt.attr,
@@ -1869,6 +1881,7 @@ static struct attribute *hpb_dev_stat_attrs[] = {
&dev_attr_rb_active_cnt.attr,
&dev_attr_rb_inactive_cnt.attr,
&dev_attr_map_req_cnt.attr,
+ &dev_attr_umap_req_cnt.attr,
NULL,
};
@@ -1984,6 +1997,7 @@ static void ufshpb_stat_init(struct ufshpb_lu *hpb)
hpb->stats.rb_active_cnt = 0;
hpb->stats.rb_inactive_cnt = 0;
hpb->stats.map_req_cnt = 0;
+ hpb->stats.umap_req_cnt = 0;
}
static void ufshpb_param_init(struct ufshpb_lu *hpb)
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 2fbe928ae7fd..b78ccb67b765 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -186,6 +186,7 @@ struct ufshpb_stats {
u64 rb_inactive_cnt;
u64 map_req_cnt;
u64 pre_req_cnt;
+ u64 umap_req_cnt;
};
struct ufshpb_lu {
--
2.25.1
We will use it later, when we'll need to differentiate between device
and host control modes.
Signed-off-by: Avri Altman <[email protected]>
---
drivers/scsi/ufs/ufshcd.h | 2 ++
drivers/scsi/ufs/ufshpb.c | 8 +++++---
drivers/scsi/ufs/ufshpb.h | 2 ++
3 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
index 3ea7e88f5bff..2d589ee18875 100644
--- a/drivers/scsi/ufs/ufshcd.h
+++ b/drivers/scsi/ufs/ufshcd.h
@@ -656,6 +656,7 @@ struct ufs_hba_variant_params {
* @hpb_disabled: flag to check if HPB is disabled
* @max_hpb_single_cmd: maximum size of single HPB command
* @is_legacy: flag to check HPB 1.0
+ * @control_mode: either host or device
*/
struct ufshpb_dev_info {
int num_lu;
@@ -665,6 +666,7 @@ struct ufshpb_dev_info {
bool hpb_disabled;
int max_hpb_single_cmd;
bool is_legacy;
+ u8 control_mode;
};
#endif
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index f89714a9785c..d9ea0cddc3c4 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -1624,6 +1624,9 @@ static void ufshpb_lu_parameter_init(struct ufs_hba *hba,
% (hpb->srgn_mem_size / HPB_ENTRY_SIZE);
hpb->pages_per_srgn = DIV_ROUND_UP(hpb->srgn_mem_size, PAGE_SIZE);
+
+ if (hpb_dev_info->control_mode == HPB_HOST_CONTROL)
+ hpb->is_hcm = true;
}
static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu *hpb)
@@ -2308,11 +2311,10 @@ void ufshpb_get_dev_info(struct ufs_hba *hba, u8 *desc_buf)
{
struct ufshpb_dev_info *hpb_dev_info = &hba->ufshpb_dev;
int version, ret;
- u8 hpb_mode;
u32 max_hpb_sigle_cmd = 0;
- hpb_mode = desc_buf[DEVICE_DESC_PARAM_HPB_CONTROL];
- if (hpb_mode == HPB_HOST_CONTROL) {
+ hpb_dev_info->control_mode = desc_buf[DEVICE_DESC_PARAM_HPB_CONTROL];
+ if (hpb_dev_info->control_mode == HPB_HOST_CONTROL) {
dev_err(hba->dev, "%s: host control mode is not supported.\n",
__func__);
hpb_dev_info->hpb_disabled = true;
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 88f424250dd9..14b7ba9bda3a 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -227,6 +227,8 @@ struct ufshpb_lu {
u32 entries_per_srgn_shift;
u32 pages_per_srgn;
+ bool is_hcm;
+
struct ufshpb_stats stats;
struct ufshpb_params params;
--
2.25.1
Support devices that report they are using host control mode.
Signed-off-by: Avri Altman <[email protected]>
---
drivers/scsi/ufs/ufshpb.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 5b76341fd558..86f4720f4f0d 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -2573,12 +2573,6 @@ void ufshpb_get_dev_info(struct ufs_hba *hba, u8 *desc_buf)
u32 max_hpb_sigle_cmd = 0;
hpb_dev_info->control_mode = desc_buf[DEVICE_DESC_PARAM_HPB_CONTROL];
- if (hpb_dev_info->control_mode == HPB_HOST_CONTROL) {
- dev_err(hba->dev, "%s: host control mode is not supported.\n",
- __func__);
- hpb_dev_info->hpb_disabled = true;
- return;
- }
version = get_unaligned_be16(desc_buf + DEVICE_DESC_PARAM_HPB_VER);
if ((version != HPB_SUPPORT_VERSION) &&
--
2.25.1
We can make use of this commit, to elaborate some more of the host
control mode logic, explaining what role play each and every variable.
While at it, allow those parameters to be configurable.
Signed-off-by: Avri Altman <[email protected]>
---
Documentation/ABI/testing/sysfs-driver-ufs | 67 ++++++
drivers/scsi/ufs/ufshpb.c | 253 +++++++++++++++++++--
drivers/scsi/ufs/ufshpb.h | 18 ++
3 files changed, 324 insertions(+), 14 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index 0017eaf89cbe..d9eca371c507 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -1322,3 +1322,70 @@ Description: This entry shows the maximum HPB data size for using single HPB
=== ========
The file is read only.
+
+What: /sys/class/scsi_device/*/device/hpb_param_sysfs/activation_thld
+Date: February 2021
+Contact: Avri Altman <[email protected]>
+Description: In host control mode, reads are the major source of activation
+ trials. once this threshold hs met, the region is added to the
+ "to-be-activated" list. Since we reset the read counter upon
+ write, this include sending a rb command updating the region
+ ppn as well.
+
+What: /sys/class/scsi_device/*/device/hpb_param_sysfs/normalization_factor
+Date: February 2021
+Contact: Avri Altman <[email protected]>
+Description: In host control mode, We think of the regions as "buckets".
+ Those buckets are being filled with reads, and emptied on write.
+ We use entries_per_srgn - the amount of blocks in a subregion as
+ our bucket size. This applies because HPB1.0 only concern a
+ single-block reads. Once the bucket size is crossed, we trigger
+ a normalization work - not only to avoid overflow, but mainly
+ because we want to keep those counters normalized, as we are
+ using those reads as a comparative score, to make various decisions.
+ The normalization is dividing (shift right) the read counter by
+ the normalization_factor. If during consecutive normalizations
+ an active region has exhaust its reads - inactivate it.
+
+What: /sys/class/scsi_device/*/device/hpb_param_sysfs/eviction_thld_enter
+Date: February 2021
+Contact: Avri Altman <[email protected]>
+Description: Region deactivation is often due to the fact that eviction took
+ place: a region become active on the expense of another. This is
+ happening when the max-active-regions limit has crossed.
+ In host mode, eviction is considered an extreme measure. We
+ want to verify that the entering region has enough reads, and
+ the exiting region has much less reads. eviction_thld_enter is
+ the min reads that a region must have in order to be considered
+ as a candidate to evict other region.
+
+What: /sys/class/scsi_device/*/device/hpb_param_sysfs/eviction_thld_exit
+Date: February 2021
+Contact: Avri Altman <[email protected]>
+Description: same as above for the exiting region. A region is consider to
+ be a candidate to be evicted, only if it has less reads than
+ eviction_thld_exit.
+
+What: /sys/class/scsi_device/*/device/hpb_param_sysfs/read_timeout_ms
+Date: February 2021
+Contact: Avri Altman <[email protected]>
+Description: In order not to hang on to “cold” regions, we shall inactivate
+ a region that has no READ access for a predefined amount of
+ time - read_timeout_ms. If read_timeout_ms has expired, and the
+ region is dirty - it is less likely that we can make any use of
+ HPB-READing it. So we inactivate it. Still, deactivation has
+ its overhead, and we may still benefit from HPB-READing this
+ region if it is clean - see read_timeout_expiries.
+
+What: /sys/class/scsi_device/*/device/hpb_param_sysfs/read_timeout_expiries
+Date: February 2021
+Contact: Avri Altman <[email protected]>
+Description: if the region read timeout has expired, but the region is clean,
+ just re-wind its timer for another spin. Do that as long as it
+ is clean and did not exhaust its read_timeout_expiries threshold.
+
+What: /sys/class/scsi_device/*/device/hpb_param_sysfs/timeout_polling_interval_ms
+Date: February 2021
+Contact: Avri Altman <[email protected]>
+Description: the frequency in which the delayed worker that checks the
+ read_timeouts is awaken.
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 86f4720f4f0d..7387c6d0f663 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -17,7 +17,6 @@
#include "../sd.h"
#define ACTIVATION_THRESHOLD 4 /* 4 IOs */
-#define EVICTION_THRESHOLD (ACTIVATION_THRESHOLD << 6) /* 256 IOs */
#define READ_TO_MS 1000
#define READ_TO_EXPIRIES 100
#define POLLING_INTERVAL_MS 200
@@ -642,7 +641,7 @@ int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
*/
spin_lock_irqsave(&rgn->rgn_lock, flags);
rgn->reads++;
- if (rgn->reads == ACTIVATION_THRESHOLD)
+ if (rgn->reads == hpb->params.activation_thld)
activate = true;
spin_unlock_irqrestore(&rgn->rgn_lock, flags);
if (activate ||
@@ -1035,6 +1034,7 @@ static void ufshpb_read_to_handler(struct work_struct *work)
struct victim_select_info *lru_info;
struct ufshpb_region *rgn;
unsigned long flags;
+ unsigned int poll;
LIST_HEAD(expired_list);
hpb = container_of(dwork, struct ufshpb_lu, ufshpb_read_to_work);
@@ -1056,7 +1056,7 @@ static void ufshpb_read_to_handler(struct work_struct *work)
list_add(&rgn->list_expired_rgn, &expired_list);
else
rgn->read_timeout = ktime_add_ms(ktime_get(),
- READ_TO_MS);
+ hpb->params.read_timeout_ms);
}
}
@@ -1074,8 +1074,9 @@ static void ufshpb_read_to_handler(struct work_struct *work)
clear_bit(TIMEOUT_WORK_PENDING, &hpb->work_data_bits);
+ poll = hpb->params.timeout_polling_interval_ms;
schedule_delayed_work(&hpb->ufshpb_read_to_work,
- msecs_to_jiffies(POLLING_INTERVAL_MS));
+ msecs_to_jiffies(poll));
}
static void ufshpb_add_lru_info(struct victim_select_info *lru_info,
@@ -1085,8 +1086,11 @@ static void ufshpb_add_lru_info(struct victim_select_info *lru_info,
list_add_tail(&rgn->list_lru_rgn, &lru_info->lh_lru_rgn);
atomic_inc(&lru_info->active_cnt);
if (rgn->hpb->is_hcm) {
- rgn->read_timeout = ktime_add_ms(ktime_get(), READ_TO_MS);
- rgn->read_timeout_expiries = READ_TO_EXPIRIES;
+ rgn->read_timeout =
+ ktime_add_ms(ktime_get(),
+ rgn->hpb->params.read_timeout_ms);
+ rgn->read_timeout_expiries =
+ rgn->hpb->params.read_timeout_expiries;
}
}
@@ -1115,7 +1119,8 @@ static struct ufshpb_region *ufshpb_victim_lru_info(struct ufshpb_lu *hpb)
* in host control mode, verify that the exiting region
* has less reads
*/
- if (hpb->is_hcm && rgn->reads > (EVICTION_THRESHOLD >> 1))
+ if (hpb->is_hcm &&
+ rgn->reads > hpb->params.eviction_thld_exit)
continue;
victim_rgn = rgn;
@@ -1346,7 +1351,8 @@ static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
* in host control mode, verify that the entering
* region has enough reads
*/
- if (hpb->is_hcm && rgn->reads < EVICTION_THRESHOLD) {
+ if (hpb->is_hcm &&
+ rgn->reads < hpb->params.eviction_thld_enter) {
ret = -EACCES;
goto out;
}
@@ -1696,8 +1702,10 @@ static void ufshpb_normalization_work_handler(struct work_struct *work)
{
struct ufshpb_lu *hpb;
int rgn_idx;
+ u8 factor;
hpb = container_of(work, struct ufshpb_lu, ufshpb_normalization_work);
+ factor = hpb->params.normalization_factor;
for (rgn_idx = 0; rgn_idx < hpb->rgns_per_lu; rgn_idx++) {
struct ufshpb_region *rgn = hpb->rgn_tbl + rgn_idx;
@@ -1706,7 +1714,7 @@ static void ufshpb_normalization_work_handler(struct work_struct *work)
unsigned long flags;
spin_lock_irqsave(&rgn->rgn_lock, flags);
- rgn->reads = (rgn->reads >> 1);
+ rgn->reads = (rgn->reads >> factor);
spin_unlock_irqrestore(&rgn->rgn_lock, flags);
}
@@ -2028,8 +2036,216 @@ requeue_timeout_ms_store(struct device *dev, struct device_attribute *attr,
}
static DEVICE_ATTR_RW(requeue_timeout_ms);
+ufshpb_sysfs_param_show_func(activation_thld);
+static ssize_t
+activation_thld_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct scsi_device *sdev = to_scsi_device(dev);
+ struct ufshpb_lu *hpb = ufshpb_get_hpb_data(sdev);
+ int val;
+
+ if (!hpb)
+ return -ENODEV;
+
+ if (!hpb->is_hcm)
+ return -EOPNOTSUPP;
+
+ if (kstrtouint(buf, 0, &val))
+ return -EINVAL;
+
+ if (val <= 0)
+ return -EINVAL;
+
+ hpb->params.activation_thld = val;
+
+ return count;
+}
+static DEVICE_ATTR_RW(activation_thld);
+
+ufshpb_sysfs_param_show_func(normalization_factor);
+static ssize_t
+normalization_factor_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct scsi_device *sdev = to_scsi_device(dev);
+ struct ufshpb_lu *hpb = ufshpb_get_hpb_data(sdev);
+ int val;
+
+ if (!hpb)
+ return -ENODEV;
+
+ if (!hpb->is_hcm)
+ return -EOPNOTSUPP;
+
+ if (kstrtouint(buf, 0, &val))
+ return -EINVAL;
+
+ if (val <= 0 || val > ilog2(hpb->entries_per_srgn))
+ return -EINVAL;
+
+ hpb->params.normalization_factor = val;
+
+ return count;
+}
+static DEVICE_ATTR_RW(normalization_factor);
+
+ufshpb_sysfs_param_show_func(eviction_thld_enter);
+static ssize_t
+eviction_thld_enter_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct scsi_device *sdev = to_scsi_device(dev);
+ struct ufshpb_lu *hpb = ufshpb_get_hpb_data(sdev);
+ int val;
+
+ if (!hpb)
+ return -ENODEV;
+
+ if (!hpb->is_hcm)
+ return -EOPNOTSUPP;
+
+ if (kstrtouint(buf, 0, &val))
+ return -EINVAL;
+
+ if (val <= hpb->params.eviction_thld_exit)
+ return -EINVAL;
+
+ hpb->params.eviction_thld_enter = val;
+
+ return count;
+}
+static DEVICE_ATTR_RW(eviction_thld_enter);
+
+ufshpb_sysfs_param_show_func(eviction_thld_exit);
+static ssize_t
+eviction_thld_exit_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct scsi_device *sdev = to_scsi_device(dev);
+ struct ufshpb_lu *hpb = ufshpb_get_hpb_data(sdev);
+ int val;
+
+ if (!hpb)
+ return -ENODEV;
+
+ if (!hpb->is_hcm)
+ return -EOPNOTSUPP;
+
+ if (kstrtouint(buf, 0, &val))
+ return -EINVAL;
+
+ if (val <= hpb->params.activation_thld)
+ return -EINVAL;
+
+ hpb->params.eviction_thld_exit = val;
+
+ return count;
+}
+static DEVICE_ATTR_RW(eviction_thld_exit);
+
+ufshpb_sysfs_param_show_func(read_timeout_ms);
+static ssize_t
+read_timeout_ms_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct scsi_device *sdev = to_scsi_device(dev);
+ struct ufshpb_lu *hpb = ufshpb_get_hpb_data(sdev);
+ int val;
+
+ if (!hpb)
+ return -ENODEV;
+
+ if (!hpb->is_hcm)
+ return -EOPNOTSUPP;
+
+ if (kstrtouint(buf, 0, &val))
+ return -EINVAL;
+
+ if (val <= 0)
+ return -EINVAL;
+
+ hpb->params.read_timeout_ms = val;
+
+ return count;
+}
+static DEVICE_ATTR_RW(read_timeout_ms);
+
+ufshpb_sysfs_param_show_func(read_timeout_expiries);
+static ssize_t
+read_timeout_expiries_store(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct scsi_device *sdev = to_scsi_device(dev);
+ struct ufshpb_lu *hpb = ufshpb_get_hpb_data(sdev);
+ int val;
+
+ if (!hpb)
+ return -ENODEV;
+
+ if (!hpb->is_hcm)
+ return -EOPNOTSUPP;
+
+ if (kstrtouint(buf, 0, &val))
+ return -EINVAL;
+
+ if (val <= 0)
+ return -EINVAL;
+
+ hpb->params.read_timeout_expiries = val;
+
+ return count;
+}
+static DEVICE_ATTR_RW(read_timeout_expiries);
+
+ufshpb_sysfs_param_show_func(timeout_polling_interval_ms);
+static ssize_t
+timeout_polling_interval_ms_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct scsi_device *sdev = to_scsi_device(dev);
+ struct ufshpb_lu *hpb = ufshpb_get_hpb_data(sdev);
+ int val;
+
+ if (!hpb)
+ return -ENODEV;
+
+ if (!hpb->is_hcm)
+ return -EOPNOTSUPP;
+
+ if (kstrtouint(buf, 0, &val))
+ return -EINVAL;
+
+ if (val <= 0)
+ return -EINVAL;
+
+ hpb->params.timeout_polling_interval_ms = val;
+
+ return count;
+}
+static DEVICE_ATTR_RW(timeout_polling_interval_ms);
+
+static void ufshpb_hcm_param_init(struct ufshpb_lu *hpb)
+{
+ hpb->params.activation_thld = ACTIVATION_THRESHOLD;
+ hpb->params.normalization_factor = 1;
+ hpb->params.eviction_thld_enter = (ACTIVATION_THRESHOLD << 6);
+ hpb->params.eviction_thld_exit = (ACTIVATION_THRESHOLD << 5);
+ hpb->params.read_timeout_ms = READ_TO_MS;
+ hpb->params.read_timeout_expiries = READ_TO_EXPIRIES;
+ hpb->params.timeout_polling_interval_ms = POLLING_INTERVAL_MS;
+}
+
static struct attribute *hpb_dev_param_attrs[] = {
&dev_attr_requeue_timeout_ms.attr,
+ &dev_attr_activation_thld.attr,
+ &dev_attr_normalization_factor.attr,
+ &dev_attr_eviction_thld_enter.attr,
+ &dev_attr_eviction_thld_exit.attr,
+ &dev_attr_read_timeout_ms.attr,
+ &dev_attr_read_timeout_expiries.attr,
+ &dev_attr_timeout_polling_interval_ms.attr,
NULL,
};
@@ -2104,6 +2320,8 @@ static void ufshpb_stat_init(struct ufshpb_lu *hpb)
static void ufshpb_param_init(struct ufshpb_lu *hpb)
{
hpb->params.requeue_timeout_ms = HPB_REQUEUE_TIME_MS;
+ if (hpb->is_hcm)
+ ufshpb_hcm_param_init(hpb);
}
static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb)
@@ -2160,9 +2378,13 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb)
ufshpb_stat_init(hpb);
ufshpb_param_init(hpb);
- if (hpb->is_hcm)
+ if (hpb->is_hcm) {
+ unsigned int poll;
+
+ poll = hpb->params.timeout_polling_interval_ms;
schedule_delayed_work(&hpb->ufshpb_read_to_work,
- msecs_to_jiffies(POLLING_INTERVAL_MS));
+ msecs_to_jiffies(poll));
+ }
return 0;
@@ -2342,10 +2564,13 @@ void ufshpb_resume(struct ufs_hba *hba)
continue;
ufshpb_set_state(hpb, HPB_PRESENT);
ufshpb_kick_map_work(hpb);
- if (hpb->is_hcm)
- schedule_delayed_work(&hpb->ufshpb_read_to_work,
- msecs_to_jiffies(POLLING_INTERVAL_MS));
+ if (hpb->is_hcm) {
+ unsigned int poll =
+ hpb->params.timeout_polling_interval_ms;
+ schedule_delayed_work(&hpb->ufshpb_read_to_work,
+ msecs_to_jiffies(poll));
+ }
}
}
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 8d14f01b0e7b..9703fe9807ad 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -180,8 +180,26 @@ struct victim_select_info {
atomic_t active_cnt;
};
+/**
+ * ufshpb_params - ufs hpb parameters
+ * @requeue_timeout_ms - requeue threshold of wb command (0x2)
+ * @activation_thld - min reads [IOs] to activate/update a region
+ * @normalization_factor - shift right the region's reads
+ * @eviction_thld_enter - min reads [IOs] for the entering region in eviction
+ * @eviction_thld_exit - max reads [IOs] for the exiting region in eviction
+ * @read_timeout_ms - timeout [ms] from the last read IO to the region
+ * @read_timeout_expiries - amount of allowable timeout expireis
+ * @timeout_polling_interval_ms - frequency in which timeouts are checked
+ */
struct ufshpb_params {
unsigned int requeue_timeout_ms;
+ unsigned int activation_thld;
+ unsigned int normalization_factor;
+ unsigned int eviction_thld_enter;
+ unsigned int eviction_thld_exit;
+ unsigned int read_timeout_ms;
+ unsigned int read_timeout_expiries;
+ unsigned int timeout_polling_interval_ms;
};
struct ufshpb_stats {
--
2.25.1
In host mode, eviction is considered an extreme measure.
verify that the entering region has enough reads, and the exiting
region has much less reads.
Signed-off-by: Avri Altman <[email protected]>
---
drivers/scsi/ufs/ufshpb.c | 20 +++++++++++++++++++-
1 file changed, 19 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 717ccb66b33e..44e56a6d7102 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -17,6 +17,7 @@
#include "../sd.h"
#define ACTIVATION_THRESHOLD 4 /* 4 IOs */
+#define EVICTION_THRESHOLD (ACTIVATION_THRESHOLD << 6) /* 256 IOs */
/* memory management */
static struct kmem_cache *ufshpb_mctx_cache;
@@ -1051,6 +1052,13 @@ static struct ufshpb_region *ufshpb_victim_lru_info(struct ufshpb_lu *hpb)
if (ufshpb_check_srgns_issue_state(hpb, rgn))
continue;
+ /*
+ * in host control mode, verify that the exiting region
+ * has less reads
+ */
+ if (hpb->is_hcm && rgn->reads > (EVICTION_THRESHOLD >> 1))
+ continue;
+
victim_rgn = rgn;
break;
}
@@ -1236,7 +1244,7 @@ static int ufshpb_issue_map_req(struct ufshpb_lu *hpb,
static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
{
- struct ufshpb_region *victim_rgn;
+ struct ufshpb_region *victim_rgn = NULL;
struct victim_select_info *lru_info = &hpb->lru_info;
unsigned long flags;
int ret = 0;
@@ -1264,6 +1272,16 @@ static int ufshpb_add_region(struct ufshpb_lu *hpb, struct ufshpb_region *rgn)
* because the device could detect this region
* by not issuing HPB_READ
*/
+
+ /*
+ * in host control mode, verify that the entering
+ * region has enough reads
+ */
+ if (hpb->is_hcm && rgn->reads < EVICTION_THRESHOLD) {
+ ret = -EACCES;
+ goto out;
+ }
+
victim_rgn = ufshpb_victim_lru_info(hpb);
if (!victim_rgn) {
dev_warn(&hpb->sdev_ufs_lu->sdev_dev,
--
2.25.1
The spec does not define what is the host's recommended response when
the device send hpb dev reset response (oper 0x2).
We will update all active hpb regions: mark them and do that on the next
read.
Signed-off-by: Avri Altman <[email protected]>
---
drivers/scsi/ufs/ufshpb.c | 53 ++++++++++++++++++++++++++++++++++++---
drivers/scsi/ufs/ufshpb.h | 3 +++
2 files changed, 53 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index cf704b82e72a..f33aa28e0a0a 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -642,7 +642,8 @@ int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
if (rgn->reads == ACTIVATION_THRESHOLD)
activate = true;
spin_unlock_irqrestore(&rgn->rgn_lock, flags);
- if (activate) {
+ if (activate ||
+ test_and_clear_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags)) {
spin_lock_irqsave(&hpb->rsp_list_lock, flags);
ufshpb_update_active_info(hpb, rgn_idx, srgn_idx);
hpb->stats.rb_active_cnt++;
@@ -1481,6 +1482,24 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
case HPB_RSP_DEV_RESET:
dev_warn(&hpb->sdev_ufs_lu->sdev_dev,
"UFS device lost HPB information during PM.\n");
+
+ if (hpb->is_hcm) {
+ struct scsi_device *sdev;
+
+ __shost_for_each_device(sdev, hba->host) {
+ struct ufshpb_lu *h = sdev->hostdata;
+
+ if (!h)
+ continue;
+
+ if (test_and_set_bit(RESET_PENDING,
+ &hpb->work_data_bits))
+ continue;
+
+ schedule_work(&hpb->ufshpb_lun_reset_work);
+ }
+ }
+
break;
default:
dev_notice(&hpb->sdev_ufs_lu->sdev_dev,
@@ -1595,6 +1614,27 @@ static void ufshpb_run_inactive_region_list(struct ufshpb_lu *hpb)
spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
}
+static void ufshpb_reset_work_handler(struct work_struct *work)
+{
+ struct ufshpb_lu *hpb;
+ struct victim_select_info *lru_info;
+ struct ufshpb_region *rgn;
+ unsigned long flags;
+
+ hpb = container_of(work, struct ufshpb_lu, ufshpb_lun_reset_work);
+
+ lru_info = &hpb->lru_info;
+
+ spin_lock_irqsave(&hpb->rgn_state_lock, flags);
+
+ list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn)
+ set_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags);
+
+ spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
+
+ clear_bit(RESET_PENDING, &hpb->work_data_bits);
+}
+
static void ufshpb_normalization_work_handler(struct work_struct *work)
{
struct ufshpb_lu *hpb;
@@ -1804,6 +1844,8 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu *hpb)
} else {
rgn->rgn_state = HPB_RGN_INACTIVE;
}
+
+ rgn->rgn_flags = 0;
}
return 0;
@@ -2018,9 +2060,12 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb)
INIT_LIST_HEAD(&hpb->list_hpb_lu);
INIT_WORK(&hpb->map_work, ufshpb_map_work_handler);
- if (hpb->is_hcm)
+ if (hpb->is_hcm) {
INIT_WORK(&hpb->ufshpb_normalization_work,
ufshpb_normalization_work_handler);
+ INIT_WORK(&hpb->ufshpb_lun_reset_work,
+ ufshpb_reset_work_handler);
+ }
hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache",
sizeof(struct ufshpb_req), 0, 0, NULL);
@@ -2120,8 +2165,10 @@ static void ufshpb_discard_rsp_lists(struct ufshpb_lu *hpb)
static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb)
{
- if (hpb->is_hcm)
+ if (hpb->is_hcm) {
+ cancel_work_sync(&hpb->ufshpb_lun_reset_work);
cancel_work_sync(&hpb->ufshpb_normalization_work);
+ }
cancel_work_sync(&hpb->map_work);
}
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index b78ccb67b765..60bf5564397b 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -121,6 +121,7 @@ struct ufshpb_region {
struct list_head list_lru_rgn;
unsigned long rgn_flags;
#define RGN_FLAG_DIRTY 0
+#define RGN_FLAG_UPDATE 1
/* region reads - for host mode */
spinlock_t rgn_lock;
@@ -217,8 +218,10 @@ struct ufshpb_lu {
/* for selecting victim */
struct victim_select_info lru_info;
struct work_struct ufshpb_normalization_work;
+ struct work_struct ufshpb_lun_reset_work;
unsigned long work_data_bits;
#define WORK_PENDING 0
+#define RESET_PENDING 1
/* pinned region information */
u32 lu_pinned_start;
--
2.25.1
In host control mode, reads are the major source of activation trials.
Keep track of those reads counters, for both active as well inactive
regions.
We reset the read counter upon write - we are only interested in "clean"
reads. less intuitive however, is that we also reset it upon region's
deactivation. Region deactivation is often due to the fact that
eviction took place: a region become active on the expense of another.
This is happening when the max-active-regions limit has crossed. If we
don’t reset the counter, we will trigger a lot of trashing of the HPB
database, since few reads (or even one) to the region that was
deactivated, will trigger a re-activation trial.
Keep those counters normalized, as we are using those reads as a
comparative score, to make various decisions.
If during consecutive normalizations an active region has exhaust its
reads - inactivate it.
Signed-off-by: Avri Altman <[email protected]>
---
drivers/scsi/ufs/ufshpb.c | 108 ++++++++++++++++++++++++++++++++------
drivers/scsi/ufs/ufshpb.h | 7 +++
2 files changed, 100 insertions(+), 15 deletions(-)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index 044fec9854a0..717ccb66b33e 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -16,6 +16,8 @@
#include "ufshpb.h"
#include "../sd.h"
+#define ACTIVATION_THRESHOLD 4 /* 4 IOs */
+
/* memory management */
static struct kmem_cache *ufshpb_mctx_cache;
static mempool_t *ufshpb_mctx_pool;
@@ -554,6 +556,21 @@ static int ufshpb_issue_pre_req(struct ufshpb_lu *hpb, struct scsi_cmnd *cmd,
return ret;
}
+static void ufshpb_update_active_info(struct ufshpb_lu *hpb, int rgn_idx,
+ int srgn_idx)
+{
+ struct ufshpb_region *rgn;
+ struct ufshpb_subregion *srgn;
+
+ rgn = hpb->rgn_tbl + rgn_idx;
+ srgn = rgn->srgn_tbl + srgn_idx;
+
+ list_del_init(&rgn->list_inact_rgn);
+
+ if (list_empty(&srgn->list_act_srgn))
+ list_add_tail(&srgn->list_act_srgn, &hpb->lh_act_srgn);
+}
+
/*
* This function will set up HPB read command using host-side L2P map data.
*/
@@ -600,12 +617,45 @@ int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
ufshpb_set_ppn_dirty(hpb, rgn_idx, srgn_idx, srgn_offset,
transfer_len);
spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
+
+ if (hpb->is_hcm) {
+ spin_lock_irqsave(&rgn->rgn_lock, flags);
+ rgn->reads = 0;
+ spin_unlock_irqrestore(&rgn->rgn_lock, flags);
+ }
+
return 0;
}
if (!ufshpb_is_support_chunk(hpb, transfer_len))
return 0;
+ if (hpb->is_hcm) {
+ bool activate = false;
+ /*
+ * in host control mode, reads are the main source for
+ * activation trials.
+ */
+ spin_lock_irqsave(&rgn->rgn_lock, flags);
+ rgn->reads++;
+ if (rgn->reads == ACTIVATION_THRESHOLD)
+ activate = true;
+ spin_unlock_irqrestore(&rgn->rgn_lock, flags);
+ if (activate) {
+ spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+ ufshpb_update_active_info(hpb, rgn_idx, srgn_idx);
+ hpb->stats.rb_active_cnt++;
+ spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
+ dev_dbg(&hpb->sdev_ufs_lu->sdev_dev,
+ "activate region %d-%d\n", rgn_idx, srgn_idx);
+ }
+
+ /* keep those counters normalized */
+ if (rgn->reads > hpb->entries_per_srgn &&
+ !test_and_set_bit(WORK_PENDING, &hpb->work_data_bits))
+ schedule_work(&hpb->ufshpb_normalization_work);
+ }
+
spin_lock_irqsave(&hpb->rgn_state_lock, flags);
if (ufshpb_test_ppn_dirty(hpb, rgn_idx, srgn_idx, srgn_offset,
transfer_len)) {
@@ -745,21 +795,6 @@ static int ufshpb_clear_dirty_bitmap(struct ufshpb_lu *hpb,
return 0;
}
-static void ufshpb_update_active_info(struct ufshpb_lu *hpb, int rgn_idx,
- int srgn_idx)
-{
- struct ufshpb_region *rgn;
- struct ufshpb_subregion *srgn;
-
- rgn = hpb->rgn_tbl + rgn_idx;
- srgn = rgn->srgn_tbl + srgn_idx;
-
- list_del_init(&rgn->list_inact_rgn);
-
- if (list_empty(&srgn->list_act_srgn))
- list_add_tail(&srgn->list_act_srgn, &hpb->lh_act_srgn);
-}
-
static void ufshpb_update_inactive_info(struct ufshpb_lu *hpb, int rgn_idx)
{
struct ufshpb_region *rgn;
@@ -1079,6 +1114,14 @@ static void __ufshpb_evict_region(struct ufshpb_lu *hpb,
ufshpb_cleanup_lru_info(lru_info, rgn);
+ if (hpb->is_hcm) {
+ unsigned long flags;
+
+ spin_lock_irqsave(&rgn->rgn_lock, flags);
+ rgn->reads = 0;
+ spin_unlock_irqrestore(&rgn->rgn_lock, flags);
+ }
+
for_each_sub_region(rgn, srgn_idx, srgn)
ufshpb_purge_active_subregion(hpb, srgn);
}
@@ -1523,6 +1566,36 @@ static void ufshpb_run_inactive_region_list(struct ufshpb_lu *hpb)
spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
}
+static void ufshpb_normalization_work_handler(struct work_struct *work)
+{
+ struct ufshpb_lu *hpb;
+ int rgn_idx;
+
+ hpb = container_of(work, struct ufshpb_lu, ufshpb_normalization_work);
+
+ for (rgn_idx = 0; rgn_idx < hpb->rgns_per_lu; rgn_idx++) {
+ struct ufshpb_region *rgn = hpb->rgn_tbl + rgn_idx;
+
+ if (rgn->reads) {
+ unsigned long flags;
+
+ spin_lock_irqsave(&rgn->rgn_lock, flags);
+ rgn->reads = (rgn->reads >> 1);
+ spin_unlock_irqrestore(&rgn->rgn_lock, flags);
+ }
+
+ if (rgn->rgn_state != HPB_RGN_ACTIVE || rgn->reads)
+ continue;
+
+ /* if region is active but has no reads - inactivate it */
+ spin_lock(&hpb->rsp_list_lock);
+ ufshpb_update_inactive_info(hpb, rgn->rgn_idx);
+ spin_unlock(&hpb->rsp_list_lock);
+ }
+
+ clear_bit(WORK_PENDING, &hpb->work_data_bits);
+}
+
static void ufshpb_map_work_handler(struct work_struct *work)
{
struct ufshpb_lu *hpb = container_of(work, struct ufshpb_lu, map_work);
@@ -1913,6 +1986,9 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb)
INIT_LIST_HEAD(&hpb->list_hpb_lu);
INIT_WORK(&hpb->map_work, ufshpb_map_work_handler);
+ if (hpb->is_hcm)
+ INIT_WORK(&hpb->ufshpb_normalization_work,
+ ufshpb_normalization_work_handler);
hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache",
sizeof(struct ufshpb_req), 0, 0, NULL);
@@ -2012,6 +2088,8 @@ static void ufshpb_discard_rsp_lists(struct ufshpb_lu *hpb)
static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb)
{
+ if (hpb->is_hcm)
+ cancel_work_sync(&hpb->ufshpb_normalization_work);
cancel_work_sync(&hpb->map_work);
}
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 8119b1a3d1e5..2fbe928ae7fd 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -121,6 +121,10 @@ struct ufshpb_region {
struct list_head list_lru_rgn;
unsigned long rgn_flags;
#define RGN_FLAG_DIRTY 0
+
+ /* region reads - for host mode */
+ spinlock_t rgn_lock;
+ unsigned int reads;
};
#define for_each_sub_region(rgn, i, srgn) \
@@ -211,6 +215,9 @@ struct ufshpb_lu {
/* for selecting victim */
struct victim_select_info lru_info;
+ struct work_struct ufshpb_normalization_work;
+ unsigned long work_data_bits;
+#define WORK_PENDING 0
/* pinned region information */
u32 lu_pinned_start;
--
2.25.1
In order not to hang on to “cold” regions, we shall inactivate a
region that has no READ access for a predefined amount of time -
READ_TO_MS. For that purpose we shall monitor the active regions list,
polling it on every POLLING_INTERVAL_MS. On timeout expiry we shall add
the region to the "to-be-inactivated" list, unless it is clean and did
not exhaust its READ_TO_EXPIRIES - another parameter.
All this does not apply to pinned regions.
Signed-off-by: Avri Altman <[email protected]>
---
drivers/scsi/ufs/ufshpb.c | 70 +++++++++++++++++++++++++++++++++++++++
drivers/scsi/ufs/ufshpb.h | 7 ++++
2 files changed, 77 insertions(+)
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index f33aa28e0a0a..5b76341fd558 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -18,6 +18,9 @@
#define ACTIVATION_THRESHOLD 4 /* 4 IOs */
#define EVICTION_THRESHOLD (ACTIVATION_THRESHOLD << 6) /* 256 IOs */
+#define READ_TO_MS 1000
+#define READ_TO_EXPIRIES 100
+#define POLLING_INTERVAL_MS 200
/* memory management */
static struct kmem_cache *ufshpb_mctx_cache;
@@ -1025,12 +1028,66 @@ static int ufshpb_check_srgns_issue_state(struct ufshpb_lu *hpb,
return 0;
}
+static void ufshpb_read_to_handler(struct work_struct *work)
+{
+ struct delayed_work *dwork = to_delayed_work(work);
+ struct ufshpb_lu *hpb;
+ struct victim_select_info *lru_info;
+ struct ufshpb_region *rgn;
+ unsigned long flags;
+ LIST_HEAD(expired_list);
+
+ hpb = container_of(dwork, struct ufshpb_lu, ufshpb_read_to_work);
+
+ if (test_and_set_bit(TIMEOUT_WORK_PENDING, &hpb->work_data_bits))
+ return;
+
+ spin_lock_irqsave(&hpb->rgn_state_lock, flags);
+
+ lru_info = &hpb->lru_info;
+
+ list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) {
+ bool timedout = ktime_after(ktime_get(), rgn->read_timeout);
+
+ if (timedout) {
+ rgn->read_timeout_expiries--;
+ if (is_rgn_dirty(rgn) ||
+ rgn->read_timeout_expiries == 0)
+ list_add(&rgn->list_expired_rgn, &expired_list);
+ else
+ rgn->read_timeout = ktime_add_ms(ktime_get(),
+ READ_TO_MS);
+ }
+ }
+
+ spin_unlock_irqrestore(&hpb->rgn_state_lock, flags);
+
+ list_for_each_entry(rgn, &expired_list, list_expired_rgn) {
+ list_del_init(&rgn->list_expired_rgn);
+ spin_lock_irqsave(&hpb->rsp_list_lock, flags);
+ ufshpb_update_inactive_info(hpb, rgn->rgn_idx);
+ hpb->stats.rb_inactive_cnt++;
+ spin_unlock_irqrestore(&hpb->rsp_list_lock, flags);
+ }
+
+ ufshpb_kick_map_work(hpb);
+
+ clear_bit(TIMEOUT_WORK_PENDING, &hpb->work_data_bits);
+
+ schedule_delayed_work(&hpb->ufshpb_read_to_work,
+ msecs_to_jiffies(POLLING_INTERVAL_MS));
+}
+
static void ufshpb_add_lru_info(struct victim_select_info *lru_info,
struct ufshpb_region *rgn)
{
rgn->rgn_state = HPB_RGN_ACTIVE;
list_add_tail(&rgn->list_lru_rgn, &lru_info->lh_lru_rgn);
atomic_inc(&lru_info->active_cnt);
+ if (rgn->hpb->is_hcm) {
+ rgn->read_timeout = ktime_add_ms(ktime_get(), READ_TO_MS);
+ rgn->read_timeout_expiries = READ_TO_EXPIRIES;
+ }
}
static void ufshpb_hit_lru_info(struct victim_select_info *lru_info,
@@ -1825,6 +1882,7 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu *hpb)
INIT_LIST_HEAD(&rgn->list_inact_rgn);
INIT_LIST_HEAD(&rgn->list_lru_rgn);
+ INIT_LIST_HEAD(&rgn->list_expired_rgn);
if (rgn_idx == hpb->rgns_per_lu - 1) {
srgn_cnt = ((hpb->srgns_per_lu - 1) %
@@ -1846,6 +1904,7 @@ static int ufshpb_alloc_region_tbl(struct ufs_hba *hba, struct ufshpb_lu *hpb)
}
rgn->rgn_flags = 0;
+ rgn->hpb = hpb;
}
return 0;
@@ -2065,6 +2124,8 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb)
ufshpb_normalization_work_handler);
INIT_WORK(&hpb->ufshpb_lun_reset_work,
ufshpb_reset_work_handler);
+ INIT_DELAYED_WORK(&hpb->ufshpb_read_to_work,
+ ufshpb_read_to_handler);
}
hpb->map_req_cache = kmem_cache_create("ufshpb_req_cache",
@@ -2099,6 +2160,10 @@ static int ufshpb_lu_hpb_init(struct ufs_hba *hba, struct ufshpb_lu *hpb)
ufshpb_stat_init(hpb);
ufshpb_param_init(hpb);
+ if (hpb->is_hcm)
+ schedule_delayed_work(&hpb->ufshpb_read_to_work,
+ msecs_to_jiffies(POLLING_INTERVAL_MS));
+
return 0;
release_pre_req_mempool:
@@ -2166,6 +2231,7 @@ static void ufshpb_discard_rsp_lists(struct ufshpb_lu *hpb)
static void ufshpb_cancel_jobs(struct ufshpb_lu *hpb)
{
if (hpb->is_hcm) {
+ cancel_delayed_work_sync(&hpb->ufshpb_read_to_work);
cancel_work_sync(&hpb->ufshpb_lun_reset_work);
cancel_work_sync(&hpb->ufshpb_normalization_work);
}
@@ -2276,6 +2342,10 @@ void ufshpb_resume(struct ufs_hba *hba)
continue;
ufshpb_set_state(hpb, HPB_PRESENT);
ufshpb_kick_map_work(hpb);
+ if (hpb->is_hcm)
+ schedule_delayed_work(&hpb->ufshpb_read_to_work,
+ msecs_to_jiffies(POLLING_INTERVAL_MS));
+
}
}
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index 60bf5564397b..8d14f01b0e7b 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -109,6 +109,7 @@ struct ufshpb_subregion {
};
struct ufshpb_region {
+ struct ufshpb_lu *hpb;
struct ufshpb_subregion *srgn_tbl;
enum HPB_RGN_STATE rgn_state;
int rgn_idx;
@@ -126,6 +127,10 @@ struct ufshpb_region {
/* region reads - for host mode */
spinlock_t rgn_lock;
unsigned int reads;
+ /* region "cold" timer - for host mode */
+ ktime_t read_timeout;
+ unsigned int read_timeout_expiries;
+ struct list_head list_expired_rgn;
};
#define for_each_sub_region(rgn, i, srgn) \
@@ -219,9 +224,11 @@ struct ufshpb_lu {
struct victim_select_info lru_info;
struct work_struct ufshpb_normalization_work;
struct work_struct ufshpb_lun_reset_work;
+ struct delayed_work ufshpb_read_to_work;
unsigned long work_data_bits;
#define WORK_PENDING 0
#define RESET_PENDING 1
+#define TIMEOUT_WORK_PENDING 2
/* pinned region information */
u32 lu_pinned_start;
--
2.25.1
> Hi Avri,
>
> > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
> > index cf704b82e72a..f33aa28e0a0a 100644
> > --- a/drivers/scsi/ufs/ufshpb.c
> > +++ b/drivers/scsi/ufs/ufshpb.c
> > @@ -642,7 +642,8 @@ int ufshpb_prep(struct ufs_hba *hba, struct
> ufshcd_lrb *lrbp)
> > if (rgn->reads == ACTIVATION_THRESHOLD)
> > activate = true;
> > spin_unlock_irqrestore(&rgn->rgn_lock, flags);
> > - if (activate) {
> > + if (activate ||
> > + test_and_clear_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags)) {
>
> How about merge rgn->rgn_flags to rgn_state?
Done.
Thanks,
Avri
>
> Thanks,
> Daejun
>
> Hi Avri,
>
> > +static void ufshpb_read_to_handler(struct work_struct *work)
> > +{
> > + struct delayed_work *dwork = to_delayed_work(work);
> > + struct ufshpb_lu *hpb;
> > + struct victim_select_info *lru_info;
> > + struct ufshpb_region *rgn;
> > + unsigned long flags;
> > + LIST_HEAD(expired_list);
> > +
> > + hpb = container_of(dwork, struct ufshpb_lu, ufshpb_read_to_work);
> > +
> > + if (test_and_set_bit(TIMEOUT_WORK_PENDING, &hpb-
> >work_data_bits))
> > + return;
> > +
> > + spin_lock_irqsave(&hpb->rgn_state_lock, flags);
> > +
> > + lru_info = &hpb->lru_info;
> > +
> > + list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) {
> > + bool timedout = ktime_after(ktime_get(), rgn->read_timeout);
> > +
> > + if (timedout) {
>
> It is important not just to check the timeout, but how much time has passed.
> If the time exceeded is twice the threshold, the read_timeout_expiries
> value should be reduced by 2 instead of 1.
Theoretically this shouldn't happened.
Please note that POLLING_INTERVAL_MS << READ_TO_MS.
Better add appropriate check when making those configurable.
Thanks,
Avri
>
> > + rgn->read_timeout_expiries--;
>
> Thanks,
> Daejun
Hi Avri,
> +static void ufshpb_read_to_handler(struct work_struct *work)
> +{
> + struct delayed_work *dwork = to_delayed_work(work);
> + struct ufshpb_lu *hpb;
> + struct victim_select_info *lru_info;
> + struct ufshpb_region *rgn;
> + unsigned long flags;
> + LIST_HEAD(expired_list);
> +
> + hpb = container_of(dwork, struct ufshpb_lu, ufshpb_read_to_work);
> +
> + if (test_and_set_bit(TIMEOUT_WORK_PENDING, &hpb->work_data_bits))
> + return;
> +
> + spin_lock_irqsave(&hpb->rgn_state_lock, flags);
> +
> + lru_info = &hpb->lru_info;
> +
> + list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) {
> + bool timedout = ktime_after(ktime_get(), rgn->read_timeout);
> +
> + if (timedout) {
It is important not just to check the timeout, but how much time has passed.
If the time exceeded is twice the threshold, the read_timeout_expiries
value should be reduced by 2 instead of 1.
> + rgn->read_timeout_expiries--;
Thanks,
Daejun
Hi Avri,
> >
> > Hi Avri,
> >
> > > +static void ufshpb_read_to_handler(struct work_struct *work)
> > > +{
> > > + struct delayed_work *dwork = to_delayed_work(work);
> > > + struct ufshpb_lu *hpb;
> > > + struct victim_select_info *lru_info;
> > > + struct ufshpb_region *rgn;
> > > + unsigned long flags;
> > > + LIST_HEAD(expired_list);
> > > +
> > > + hpb = container_of(dwork, struct ufshpb_lu, ufshpb_read_to_work);
> > > +
> > > + if (test_and_set_bit(TIMEOUT_WORK_PENDING, &hpb-
> > >work_data_bits))
> > > + return;
> > > +
> > > + spin_lock_irqsave(&hpb->rgn_state_lock, flags);
> > > +
> > > + lru_info = &hpb->lru_info;
> > > +
> > > + list_for_each_entry(rgn, &lru_info->lh_lru_rgn, list_lru_rgn) {
> > > + bool timedout = ktime_after(ktime_get(), rgn->read_timeout);
> > > +
> > > + if (timedout) {
> >
> > It is important not just to check the timeout, but how much time has passed.
> > If the time exceeded is twice the threshold, the read_timeout_expiries
> > value should be reduced by 2 instead of 1.
> Theoretically this shouldn't happened.
> Please note that POLLING_INTERVAL_MS << READ_TO_MS.
> Better add appropriate check when making those configurable.
OK, I agree.
Thanks,
Daejun
>
> Thanks,
> Avri
> >
> > > + rgn->read_timeout_expiries--;
> >
> > Thanks,
> > Daejun
>
>
>
>
>
> Hi Avri,
>
> > +static void ufshpb_normalization_work_handler(struct work_struct *work)
> > +{
> > + struct ufshpb_lu *hpb;
> > + int rgn_idx;
> > +
> > + hpb = container_of(work, struct ufshpb_lu,
> ufshpb_normalization_work);
> > +
> > + for (rgn_idx = 0; rgn_idx < hpb->rgns_per_lu; rgn_idx++) {
> > + struct ufshpb_region *rgn = hpb->rgn_tbl + rgn_idx;
>
> *HERE*
> > + if (rgn->reads) {
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&rgn->rgn_lock, flags);
>
> I thinks this lock should protect rgn->reads when it is accessed.
>
> > + rgn->reads = (rgn->reads >> 1);
> > + spin_unlock_irqrestore(&rgn->rgn_lock, flags);
> > + }
> *HERE*
Done.
>
> > +
> > + if (rgn->rgn_state != HPB_RGN_ACTIVE || rgn->reads)
> > + continue;
> > +
> > + /* if region is active but has no reads - inactivate it */
> > + spin_lock(&hpb->rsp_list_lock);
> > + ufshpb_update_inactive_info(hpb, rgn->rgn_idx);
> > + spin_unlock(&hpb->rsp_list_lock);
> > + }
> > +
> > + clear_bit(WORK_PENDING, &hpb->work_data_bits);
>
> Why we use work_data_bits? It may be checked by worker API.
Done.
>
> Thanks,
> Daejun
>
> Hi Avri,
>
> > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
> > index cf704b82e72a..f33aa28e0a0a 100644
> > --- a/drivers/scsi/ufs/ufshpb.c
> > +++ b/drivers/scsi/ufs/ufshpb.c
> > @@ -642,7 +642,8 @@ int ufshpb_prep(struct ufs_hba *hba, struct
> ufshcd_lrb *lrbp)
> > if (rgn->reads == ACTIVATION_THRESHOLD)
> > activate = true;
> > spin_unlock_irqrestore(&rgn->rgn_lock, flags);
> > - if (activate) {
> > + if (activate ||
> > + test_and_clear_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags)) {
>
> How about merge rgn->rgn_flags to rgn_state?
This will actually not work, because a region can be e.g. active/inactive and dirty,
And I don't want to mess with the regions state machine.
Thanks,
Avri
>
> Thanks,
> Daejun
Hi Avri,
> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
> index cf704b82e72a..f33aa28e0a0a 100644
> --- a/drivers/scsi/ufs/ufshpb.c
> +++ b/drivers/scsi/ufs/ufshpb.c
> @@ -642,7 +642,8 @@ int ufshpb_prep(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
> if (rgn->reads == ACTIVATION_THRESHOLD)
> activate = true;
> spin_unlock_irqrestore(&rgn->rgn_lock, flags);
> - if (activate) {
> + if (activate ||
> + test_and_clear_bit(RGN_FLAG_UPDATE, &rgn->rgn_flags)) {
How about merge rgn->rgn_flags to rgn_state?
Thanks,
Daejun
Hi Avri,
> +static void ufshpb_normalization_work_handler(struct work_struct *work)
> +{
> + struct ufshpb_lu *hpb;
> + int rgn_idx;
> +
> + hpb = container_of(work, struct ufshpb_lu, ufshpb_normalization_work);
> +
> + for (rgn_idx = 0; rgn_idx < hpb->rgns_per_lu; rgn_idx++) {
> + struct ufshpb_region *rgn = hpb->rgn_tbl + rgn_idx;
*HERE*
> + if (rgn->reads) {
> + unsigned long flags;
> +
> + spin_lock_irqsave(&rgn->rgn_lock, flags);
I thinks this lock should protect rgn->reads when it is accessed.
> + rgn->reads = (rgn->reads >> 1);
> + spin_unlock_irqrestore(&rgn->rgn_lock, flags);
> + }
*HERE*
> +
> + if (rgn->rgn_state != HPB_RGN_ACTIVE || rgn->reads)
> + continue;
> +
> + /* if region is active but has no reads - inactivate it */
> + spin_lock(&hpb->rsp_list_lock);
> + ufshpb_update_inactive_info(hpb, rgn->rgn_idx);
> + spin_unlock(&hpb->rsp_list_lock);
> + }
> +
> + clear_bit(WORK_PENDING, &hpb->work_data_bits);
Why we use work_data_bits? It may be checked by worker API.
Thanks,
Daejun