From: Bean Huo <[email protected]>
According to the documentation of the sysfs nodes rb_noti_cnt, rb_active_cnt
and rb_inactive_cnt, they are all related to HPB recommendation in UPIU response
packet. I don't know what 'rb' refers to, I think 'rcmd'
(recommendation) should be the correct abbreviation.
Change the sysfs documentation about these sysfs nodes to highlight what they mean
under different HPB control modes.
Signed-off-by: Bean Huo <[email protected]>
---
Documentation/ABI/testing/sysfs-driver-ufs | 18 +++++++++-------
drivers/scsi/ufs/ufshpb.c | 24 +++++++++++-----------
drivers/scsi/ufs/ufshpb.h | 6 +++---
3 files changed, 26 insertions(+), 22 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
index a44ef8bfbadf..6b248abb1bd7 100644
--- a/Documentation/ABI/testing/sysfs-driver-ufs
+++ b/Documentation/ABI/testing/sysfs-driver-ufs
@@ -1518,7 +1518,7 @@ Description: This entry shows the number of reads that cannot be changed to
The file is read only.
-What: /sys/class/scsi_device/*/device/hpb_stats/rb_noti_cnt
+What: /sys/class/scsi_device/*/device/hpb_stats/rcmd_noti_cnt
Date: June 2021
Contact: Daejun Park <[email protected]>
Description: This entry shows the number of response UPIUs that has
@@ -1526,19 +1526,23 @@ Description: This entry shows the number of response UPIUs that has
The file is read only.
-What: /sys/class/scsi_device/*/device/hpb_stats/rb_active_cnt
+What: /sys/class/scsi_device/*/device/hpb_stats/rcmd_active_cnt
Date: June 2021
Contact: Daejun Park <[email protected]>
-Description: This entry shows the number of active sub-regions recommended by
- response UPIUs.
+Description: For the HPB device control mode, this entry shows the number of
+ active sub-regions recommended by response UPIUs. For the HPB host control
+ mode, this entry shows the number of active sub-regions recommended by the
+ HPB host control mode heuristic algorithm.
The file is read only.
-What: /sys/class/scsi_device/*/device/hpb_stats/rb_inactive_cnt
+What: /sys/class/scsi_device/*/device/hpb_stats/rcmd_inactive_cnt
Date: June 2021
Contact: Daejun Park <[email protected]>
-Description: This entry shows the number of inactive regions recommended by
- response UPIUs.
+Description: For the HPB device control mode, this entry shows the number of
+ inactive regions recommended by response UPIUs. For the HPB host control
+ mode, this entry shows the number of inactive regions recommended by the
+ HPB host control mode heuristic algorithm.
The file is read only.
diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
index f1f30d4c3d65..e7f311bb4401 100644
--- a/drivers/scsi/ufs/ufshpb.c
+++ b/drivers/scsi/ufs/ufshpb.c
@@ -563,7 +563,7 @@ static void ufshpb_update_active_info(struct ufshpb_lu *hpb, int rgn_idx,
if (list_empty(&srgn->list_act_srgn))
list_add_tail(&srgn->list_act_srgn, &hpb->lh_act_srgn);
- hpb->stats.rb_active_cnt++;
+ hpb->stats.rcmd_active_cnt++;
}
static void ufshpb_update_inactive_info(struct ufshpb_lu *hpb, int rgn_idx)
@@ -580,7 +580,7 @@ static void ufshpb_update_inactive_info(struct ufshpb_lu *hpb, int rgn_idx)
if (list_empty(&rgn->list_inact_rgn))
list_add_tail(&rgn->list_inact_rgn, &hpb->lh_inact_rgn);
- hpb->stats.rb_inactive_cnt++;
+ hpb->stats.rcmd_inactive_cnt++;
}
static void ufshpb_activate_subregion(struct ufshpb_lu *hpb,
@@ -1321,7 +1321,7 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
if (!ufshpb_is_hpb_rsp_valid(hba, lrbp, rsp_field))
return;
- hpb->stats.rb_noti_cnt++;
+ hpb->stats.rcmd_noti_cnt++;
switch (rsp_field->hpb_op) {
case HPB_RSP_REQ_REGION_UPDATE:
@@ -1724,18 +1724,18 @@ static DEVICE_ATTR_RO(__name)
ufshpb_sysfs_attr_show_func(hit_cnt);
ufshpb_sysfs_attr_show_func(miss_cnt);
-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(rcmd_noti_cnt);
+ufshpb_sysfs_attr_show_func(rcmd_active_cnt);
+ufshpb_sysfs_attr_show_func(rcmd_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,
&dev_attr_miss_cnt.attr,
- &dev_attr_rb_noti_cnt.attr,
- &dev_attr_rb_active_cnt.attr,
- &dev_attr_rb_inactive_cnt.attr,
+ &dev_attr_rcmd_noti_cnt.attr,
+ &dev_attr_rcmd_active_cnt.attr,
+ &dev_attr_rcmd_inactive_cnt.attr,
&dev_attr_map_req_cnt.attr,
&dev_attr_umap_req_cnt.attr,
NULL,
@@ -2098,9 +2098,9 @@ static void ufshpb_stat_init(struct ufshpb_lu *hpb)
{
hpb->stats.hit_cnt = 0;
hpb->stats.miss_cnt = 0;
- hpb->stats.rb_noti_cnt = 0;
- hpb->stats.rb_active_cnt = 0;
- hpb->stats.rb_inactive_cnt = 0;
+ hpb->stats.rcmd_noti_cnt = 0;
+ hpb->stats.rcmd_active_cnt = 0;
+ hpb->stats.rcmd_inactive_cnt = 0;
hpb->stats.map_req_cnt = 0;
hpb->stats.umap_req_cnt = 0;
}
diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
index b83b9ec9044a..0d6e6004d783 100644
--- a/drivers/scsi/ufs/ufshpb.h
+++ b/drivers/scsi/ufs/ufshpb.h
@@ -211,9 +211,9 @@ struct ufshpb_params {
struct ufshpb_stats {
u64 hit_cnt;
u64 miss_cnt;
- u64 rb_noti_cnt;
- u64 rb_active_cnt;
- u64 rb_inactive_cnt;
+ u64 rcmd_noti_cnt;
+ u64 rcmd_active_cnt;
+ u64 rcmd_inactive_cnt;
u64 map_req_cnt;
u64 pre_req_cnt;
u64 umap_req_cnt;
--
2.34.1
>From: Bean Huo <[email protected]>
>
>According to the documentation of the sysfs nodes rb_noti_cnt, rb_active_cnt
>and rb_inactive_cnt, they are all related to HPB recommendation in UPIU response
>packet. I don't know what 'rb' refers to, I think 'rcmd'
>(recommendation) should be the correct abbreviation.
>
>Change the sysfs documentation about these sysfs nodes to highlight what they mean
>under different HPB control modes.
I think it's better than what I suggested.
>
>Signed-off-by: Bean Huo <[email protected]>
>---
> Documentation/ABI/testing/sysfs-driver-ufs | 18 +++++++++-------
> drivers/scsi/ufs/ufshpb.c | 24 +++++++++++-----------
> drivers/scsi/ufs/ufshpb.h | 6 +++---
> 3 files changed, 26 insertions(+), 22 deletions(-)
>
>diff --git a/Documentation/ABI/testing/sysfs-driver-ufs b/Documentation/ABI/testing/sysfs-driver-ufs
>index a44ef8bfbadf..6b248abb1bd7 100644
>--- a/Documentation/ABI/testing/sysfs-driver-ufs
>+++ b/Documentation/ABI/testing/sysfs-driver-ufs
>@@ -1518,7 +1518,7 @@ Description: This entry shows the number of reads that cannot be changed to
>
> The file is read only.
>
>-What: /sys/class/scsi_device/*/device/hpb_stats/rb_noti_cnt
>+What: /sys/class/scsi_device/*/device/hpb_stats/rcmd_noti_cnt
> Date: June 2021
> Contact: Daejun Park <[email protected]>
> Description: This entry shows the number of response UPIUs that has
>@@ -1526,19 +1526,23 @@ Description: This entry shows the number of response UPIUs that has
>
> The file is read only.
>
>-What: /sys/class/scsi_device/*/device/hpb_stats/rb_active_cnt
>+What: /sys/class/scsi_device/*/device/hpb_stats/rcmd_active_cnt
> Date: June 2021
> Contact: Daejun Park <[email protected]>
>-Description: This entry shows the number of active sub-regions recommended by
>- response UPIUs.
>+Description: For the HPB device control mode, this entry shows the number of
>+ active sub-regions recommended by response UPIUs. For the HPB host control
>+ mode, this entry shows the number of active sub-regions recommended by the
>+ HPB host control mode heuristic algorithm.
Please check indentation.
>
> The file is read only.
>
>-What: /sys/class/scsi_device/*/device/hpb_stats/rb_inactive_cnt
>+What: /sys/class/scsi_device/*/device/hpb_stats/rcmd_inactive_cnt
> Date: June 2021
> Contact: Daejun Park <[email protected]>
>-Description: This entry shows the number of inactive regions recommended by
>- response UPIUs.
>+Description: For the HPB device control mode, this entry shows the number of
>+ inactive regions recommended by response UPIUs. For the HPB host control
>+ mode, this entry shows the number of inactive regions recommended by the
>+ HPB host control mode heuristic algorithm.
Please check indentation.
>
> The file is read only.
>
>diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
>index f1f30d4c3d65..e7f311bb4401 100644
>--- a/drivers/scsi/ufs/ufshpb.c
>+++ b/drivers/scsi/ufs/ufshpb.c
>@@ -563,7 +563,7 @@ static void ufshpb_update_active_info(struct ufshpb_lu *hpb, int rgn_idx,
> if (list_empty(&srgn->list_act_srgn))
> list_add_tail(&srgn->list_act_srgn, &hpb->lh_act_srgn);
>
>- hpb->stats.rb_active_cnt++;
>+ hpb->stats.rcmd_active_cnt++;
> }
>
> static void ufshpb_update_inactive_info(struct ufshpb_lu *hpb, int rgn_idx)
>@@ -580,7 +580,7 @@ static void ufshpb_update_inactive_info(struct ufshpb_lu *hpb, int rgn_idx)
> if (list_empty(&rgn->list_inact_rgn))
> list_add_tail(&rgn->list_inact_rgn, &hpb->lh_inact_rgn);
>
>- hpb->stats.rb_inactive_cnt++;
>+ hpb->stats.rcmd_inactive_cnt++;
> }
>
> static void ufshpb_activate_subregion(struct ufshpb_lu *hpb,
>@@ -1321,7 +1321,7 @@ void ufshpb_rsp_upiu(struct ufs_hba *hba, struct ufshcd_lrb *lrbp)
> if (!ufshpb_is_hpb_rsp_valid(hba, lrbp, rsp_field))
> return;
>
>- hpb->stats.rb_noti_cnt++;
>+ hpb->stats.rcmd_noti_cnt++;
>
> switch (rsp_field->hpb_op) {
> case HPB_RSP_REQ_REGION_UPDATE:
>@@ -1724,18 +1724,18 @@ static DEVICE_ATTR_RO(__name)
>
> ufshpb_sysfs_attr_show_func(hit_cnt);
> ufshpb_sysfs_attr_show_func(miss_cnt);
>-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(rcmd_noti_cnt);
>+ufshpb_sysfs_attr_show_func(rcmd_active_cnt);
>+ufshpb_sysfs_attr_show_func(rcmd_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,
> &dev_attr_miss_cnt.attr,
>- &dev_attr_rb_noti_cnt.attr,
>- &dev_attr_rb_active_cnt.attr,
>- &dev_attr_rb_inactive_cnt.attr,
>+ &dev_attr_rcmd_noti_cnt.attr,
>+ &dev_attr_rcmd_active_cnt.attr,
>+ &dev_attr_rcmd_inactive_cnt.attr,
> &dev_attr_map_req_cnt.attr,
> &dev_attr_umap_req_cnt.attr,
> NULL,
>@@ -2098,9 +2098,9 @@ static void ufshpb_stat_init(struct ufshpb_lu *hpb)
> {
> hpb->stats.hit_cnt = 0;
> hpb->stats.miss_cnt = 0;
>- hpb->stats.rb_noti_cnt = 0;
>- hpb->stats.rb_active_cnt = 0;
>- hpb->stats.rb_inactive_cnt = 0;
>+ hpb->stats.rcmd_noti_cnt = 0;
>+ hpb->stats.rcmd_active_cnt = 0;
>+ hpb->stats.rcmd_inactive_cnt = 0;
> hpb->https://protect2.fireeye.com/v1/url?k=9b807207-fa0b6728-9b81f948-74fe485cbfe7-c8e650b167f03dee&q=1&e=ec9d9a5f-605d-45e5-b145-e3a4c9a96f3e&u=http%3A%2F%2Fstats.map%2F_req_cnt = 0;
It's not related to the patch, but the code seems to be wrong.
> hpb->stats.umap_req_cnt = 0;
> }
>diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
>index b83b9ec9044a..0d6e6004d783 100644
>--- a/drivers/scsi/ufs/ufshpb.h
>+++ b/drivers/scsi/ufs/ufshpb.h
>@@ -211,9 +211,9 @@ struct ufshpb_params {
> struct ufshpb_stats {
> u64 hit_cnt;
> u64 miss_cnt;
>- u64 rb_noti_cnt;
>- u64 rb_active_cnt;
>- u64 rb_inactive_cnt;
>+ u64 rcmd_noti_cnt;
>+ u64 rcmd_active_cnt;
>+ u64 rcmd_inactive_cnt;
> u64 map_req_cnt;
> u64 pre_req_cnt;
> u64 umap_req_cnt;
>--
>2.34.1
>
>
Apart from the indentation, the patch looks good to me, so once
you've fixed them, feel free to add my:
Reviewed-by: Keoseong Park <[email protected]>
Best Regards,
Keoseong Park
> From: Bean Huo <[email protected]>
>
> According to the documentation of the sysfs nodes rb_noti_cnt, rb_active_cnt
> and rb_inactive_cnt, they are all related to HPB recommendation in UPIU
> response packet. I don't know what 'rb' refers to, I think 'rcmd'
> (recommendation) should be the correct abbreviation.
rb stands for READ-BUFFER
>
> Change the sysfs documentation about these sysfs nodes to highlight what
> they mean under different HPB control modes.
>
> Signed-off-by: Bean Huo <[email protected]>
I don't think it is allowed to change the kernel's ABI.
Thanks,
Avri
On Mon, 2022-04-25 at 12:43 +0900, Keoseong Park wrote:
> > 2.34.1
> >
> >
> Apart from the indentation, the patch looks good to me, so once
> you've fixed them, feel free to add my:
>
> Reviewed-by: Keoseong Park <[email protected]>
thanks for your review, I will change them based on your comments in
the next version.
Kind regards,
Bean
On Mon, 2022-04-25 at 06:19 +0000, Avri Altman wrote:
> > From: Bean Huo <[email protected]>
> >
> > According to the documentation of the sysfs nodes rb_noti_cnt,
> > rb_active_cnt
> > and rb_inactive_cnt, they are all related to HPB recommendation in
> > UPIU
> > response packet. I don't know what 'rb' refers to, I think 'rcmd'
> > (recommendation) should be the correct abbreviation.
> rb stands for READ-BUFFER
>
Thanks. right. probably it is HPB READ BUFFER. I think based on the
documentation,rcmd much makes sense, there is region inactive
recommendation, it is no need to issue HPB read buffer.
> >
> > Change the sysfs documentation about these sysfs nodes to highlight
> > what
> > they mean under different HPB control modes.
> >
> > Signed-off-by: Bean Huo <[email protected]>
> I don't think it is allowed to change the kernel's ABI.
>
I don't know where this limitation is stated, I see a lot of ABI change
patches on the mailing list. Also, I didn't change its original
meaning, I added a new node meaning to make it clearer. According to
the documentation on these nodes, they should be used in HPB host
control mode, right? Otherwise, we should add a new note for HPB host
control mode. Considering we already have these nodes, we don't need to
change the HPB host control mode code just by changing the ABI to have
the HPB host control mode reuse them. Here are my thoughts on change.
Please consider my changes again.
Kind regards,
Bean
> Thanks,
> Avri