* [PATCH] ufs: core: Only collect timestamps if the monitoring functionality is enabled
@ 2025-08-15 16:00 Bart Van Assche
[not found] ` <CGME20250815160114epcas2p43832aa1b9ad0b0efb98ec59f5b568c96@epcms2p8>
2025-08-18 11:56 ` Peter Wang (王信友)
0 siblings, 2 replies; 6+ messages in thread
From: Bart Van Assche @ 2025-08-15 16:00 UTC (permalink / raw)
To: Martin K . Petersen
Cc: linux-scsi, Bart Van Assche, Can Guo, Bean Huo, Daejun Park,
James E.J. Bottomley, Peter Wang, Avri Altman, Bao D. Nguyen,
Adrian Hunter
Every ktime_get() call in the hot path has a measurable impact on IOPS. Hence,
only collect timestamps if the monitoring functionality is enabled.
See also commit 1d8613a23f3c ("scsi: ufs: core: Introduce HBA performance
monitor sysfs nodes"; v5.14).
Cc: Can Guo <cang@codeaurora.org>
Cc: Bean Huo <beanhuo@micron.com>
Cc: Daejun Park <daejun7.park@samsung.com>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
drivers/ufs/core/ufshcd.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 714d9966431e..027dc0355ae6 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -2357,10 +2357,12 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag,
struct ufshcd_lrb *lrbp = &hba->lrb[task_tag];
unsigned long flags;
- lrbp->issue_time_stamp = ktime_get();
- lrbp->issue_time_stamp_local_clock = local_clock();
- lrbp->compl_time_stamp = ktime_set(0, 0);
- lrbp->compl_time_stamp_local_clock = 0;
+ if (hba->monitor.enabled) {
+ lrbp->issue_time_stamp = ktime_get();
+ lrbp->issue_time_stamp_local_clock = local_clock();
+ lrbp->compl_time_stamp = ktime_set(0, 0);
+ lrbp->compl_time_stamp_local_clock = 0;
+ }
ufshcd_add_command_trace(hba, task_tag, UFS_CMD_SEND);
if (lrbp->cmd)
ufshcd_clk_scaling_start_busy(hba);
@@ -5622,8 +5624,10 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba, int task_tag,
enum utp_ocs ocs;
lrbp = &hba->lrb[task_tag];
- lrbp->compl_time_stamp = ktime_get();
- lrbp->compl_time_stamp_local_clock = local_clock();
+ if (hba->monitor.enabled) {
+ lrbp->compl_time_stamp = ktime_get();
+ lrbp->compl_time_stamp_local_clock = local_clock();
+ }
cmd = lrbp->cmd;
if (cmd) {
if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
^ permalink raw reply related [flat|nested] 6+ messages in thread
* RE: [PATCH] ufs: core: Only collect timestamps if the monitoring functionality is enabled
[not found] ` <CGME20250815160114epcas2p43832aa1b9ad0b0efb98ec59f5b568c96@epcms2p8>
@ 2025-08-18 2:36 ` Daejun Park
2025-08-18 15:50 ` Bart Van Assche
0 siblings, 1 reply; 6+ messages in thread
From: Daejun Park @ 2025-08-18 2:36 UTC (permalink / raw)
To: Bart Van Assche, Martin K . Petersen
Cc: linux-scsi@vger.kernel.org, Can Guo, Bean Huo,
James E.J. Bottomley, Peter Wang, Avri Altman, Bao D. Nguyen,
Adrian Hunter, Daejun Park
Hi Bart,
> Every ktime_get() call in the hot path has a measurable impact on IOPS. Hence,
> only collect timestamps if the monitoring functionality is enabled.
>
> See also commit 1d8613a23f3c ("scsi: ufs: core: Introduce HBA performance
> monitor sysfs nodes"; v5.14).
>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Bean Huo <beanhuo@micron.com>
> Cc: Daejun Park <daejun7.park@samsung.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
> drivers/ufs/core/ufshcd.c 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 714d9966431e..027dc0355ae6 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2357,10 +2357,12 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag,
> struct ufshcd_lrb *lrbp = &hba->lrb[task_tag];
> unsigned long flags;
>
> - lrbp->issue_time_stamp = ktime_get();
> - lrbp->issue_time_stamp_local_clock = local_clock();
> - lrbp->compl_time_stamp = ktime_set(0, 0);
> - lrbp->compl_time_stamp_local_clock = 0;
How about modifying ufshcd_compl_one_cqe() to skip updating compl_time_stamp?
Thanks,
Daejun
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ufs: core: Only collect timestamps if the monitoring functionality is enabled
2025-08-15 16:00 [PATCH] ufs: core: Only collect timestamps if the monitoring functionality is enabled Bart Van Assche
[not found] ` <CGME20250815160114epcas2p43832aa1b9ad0b0efb98ec59f5b568c96@epcms2p8>
@ 2025-08-18 11:56 ` Peter Wang (王信友)
2025-08-18 15:53 ` Bart Van Assche
1 sibling, 1 reply; 6+ messages in thread
From: Peter Wang (王信友) @ 2025-08-18 11:56 UTC (permalink / raw)
To: bvanassche@acm.org, martin.petersen@oracle.com
Cc: beanhuo@micron.com, cang@codeaurora.org, quic_nguyenb@quicinc.com,
linux-scsi@vger.kernel.org, daejun7.park@samsung.com,
adrian.hunter@intel.com, avri.altman@sandisk.com,
James.Bottomley@HansenPartnership.com
On Fri, 2025-08-15 at 09:00 -0700, Bart Van Assche wrote:
>
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index 714d9966431e..027dc0355ae6 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -2357,10 +2357,12 @@ void ufshcd_send_command(struct ufs_hba *hba,
> unsigned int task_tag,
> struct ufshcd_lrb *lrbp = &hba->lrb[task_tag];
> unsigned long flags;
>
> - lrbp->issue_time_stamp = ktime_get();
> - lrbp->issue_time_stamp_local_clock = local_clock();
> - lrbp->compl_time_stamp = ktime_set(0, 0);
> - lrbp->compl_time_stamp_local_clock = 0;
> + if (hba->monitor.enabled) {
> + lrbp->issue_time_stamp = ktime_get();
> + lrbp->issue_time_stamp_local_clock = local_clock();
> + lrbp->compl_time_stamp = ktime_set(0, 0);
> + lrbp->compl_time_stamp_local_clock = 0;
> + }
> ufshcd_add_command_trace(hba, task_tag, UFS_CMD_SEND);
> if (lrbp->cmd)
> ufshcd_clk_scaling_start_busy(hba);
> @@ -5622,8 +5624,10 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba,
> int task_tag,
> enum utp_ocs ocs;
>
> lrbp = &hba->lrb[task_tag];
> - lrbp->compl_time_stamp = ktime_get();
> - lrbp->compl_time_stamp_local_clock = local_clock();
> + if (hba->monitor.enabled) {
> + lrbp->compl_time_stamp = ktime_get();
> + lrbp->compl_time_stamp_local_clock = local_clock();
> + }
> cmd = lrbp->cmd;
> if (cmd) {
> if (unlikely(ufshcd_should_inform_monitor(hba,
> lrbp)))
Hi Bart,
Should this information also be provided to
ufshcd_print_tr for debugging purposes?
Thanks
Peter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ufs: core: Only collect timestamps if the monitoring functionality is enabled
2025-08-18 2:36 ` Daejun Park
@ 2025-08-18 15:50 ` Bart Van Assche
0 siblings, 0 replies; 6+ messages in thread
From: Bart Van Assche @ 2025-08-18 15:50 UTC (permalink / raw)
To: daejun7.park, Martin K . Petersen
Cc: linux-scsi@vger.kernel.org, Can Guo, Bean Huo,
James E.J. Bottomley, Peter Wang, Avri Altman, Bao D. Nguyen,
Adrian Hunter
On 8/17/25 7:36 PM, Daejun Park wrote:
> How about modifying ufshcd_compl_one_cqe() to skip updating compl_time_stamp?
I think my patch already does that:
@@ -5622,8 +5624,10 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba,
int task_tag,
enum utp_ocs ocs;
lrbp = &hba->lrb[task_tag];
- lrbp->compl_time_stamp = ktime_get();
- lrbp->compl_time_stamp_local_clock = local_clock();
+ if (hba->monitor.enabled) {
+ lrbp->compl_time_stamp = ktime_get();
+ lrbp->compl_time_stamp_local_clock = local_clock();
+ }
cmd = lrbp->cmd;
if (cmd) {
if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
Bart.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ufs: core: Only collect timestamps if the monitoring functionality is enabled
2025-08-18 11:56 ` Peter Wang (王信友)
@ 2025-08-18 15:53 ` Bart Van Assche
2025-08-19 6:01 ` Peter Wang (王信友)
0 siblings, 1 reply; 6+ messages in thread
From: Bart Van Assche @ 2025-08-18 15:53 UTC (permalink / raw)
To: Peter Wang (王信友), martin.petersen@oracle.com
Cc: beanhuo@micron.com, cang@codeaurora.org, quic_nguyenb@quicinc.com,
linux-scsi@vger.kernel.org, daejun7.park@samsung.com,
adrian.hunter@intel.com, avri.altman@sandisk.com,
James.Bottomley@HansenPartnership.com
On 8/18/25 4:56 AM, Peter Wang (王信友) wrote:
> Should this information also be provided to
> ufshcd_print_tr for debugging purposes?
How about the patch below?
Thanks,
Bart.
---
drivers/ufs/core/ufshcd.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index 714d9966431e..29a2ecb7b5b9 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -607,10 +607,12 @@ void ufshcd_print_tr(struct ufs_hba *hba, int tag,
bool pr_prdt)
lrbp = &hba->lrb[tag];
- dev_err(hba->dev, "UPIU[%d] - issue time %lld us\n",
- tag, div_u64(lrbp->issue_time_stamp_local_clock, 1000));
- dev_err(hba->dev, "UPIU[%d] - complete time %lld us\n",
- tag, div_u64(lrbp->compl_time_stamp_local_clock, 1000));
+ if (hba->monitor.enabled) {
+ dev_err(hba->dev, "UPIU[%d] - issue time %lld us\n", tag,
+ div_u64(lrbp->issue_time_stamp_local_clock, 1000));
+ dev_err(hba->dev, "UPIU[%d] - complete time %lld us\n", tag,
+ div_u64(lrbp->compl_time_stamp_local_clock, 1000));
+ }
dev_err(hba->dev,
"UPIU[%d] - Transfer Request Descriptor phys@0x%llx\n",
tag, (u64)lrbp->utrd_dma_addr);
@@ -2357,10 +2359,12 @@ void ufshcd_send_command(struct ufs_hba *hba,
unsigned int task_tag,
struct ufshcd_lrb *lrbp = &hba->lrb[task_tag];
unsigned long flags;
- lrbp->issue_time_stamp = ktime_get();
- lrbp->issue_time_stamp_local_clock = local_clock();
- lrbp->compl_time_stamp = ktime_set(0, 0);
- lrbp->compl_time_stamp_local_clock = 0;
+ if (hba->monitor.enabled) {
+ lrbp->issue_time_stamp = ktime_get();
+ lrbp->issue_time_stamp_local_clock = local_clock();
+ lrbp->compl_time_stamp = ktime_set(0, 0);
+ lrbp->compl_time_stamp_local_clock = 0;
+ }
ufshcd_add_command_trace(hba, task_tag, UFS_CMD_SEND);
if (lrbp->cmd)
ufshcd_clk_scaling_start_busy(hba);
@@ -5622,8 +5626,10 @@ void ufshcd_compl_one_cqe(struct ufs_hba *hba,
int task_tag,
enum utp_ocs ocs;
lrbp = &hba->lrb[task_tag];
- lrbp->compl_time_stamp = ktime_get();
- lrbp->compl_time_stamp_local_clock = local_clock();
+ if (hba->monitor.enabled) {
+ lrbp->compl_time_stamp = ktime_get();
+ lrbp->compl_time_stamp_local_clock = local_clock();
+ }
cmd = lrbp->cmd;
if (cmd) {
if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ufs: core: Only collect timestamps if the monitoring functionality is enabled
2025-08-18 15:53 ` Bart Van Assche
@ 2025-08-19 6:01 ` Peter Wang (王信友)
0 siblings, 0 replies; 6+ messages in thread
From: Peter Wang (王信友) @ 2025-08-19 6:01 UTC (permalink / raw)
To: bvanassche@acm.org, martin.petersen@oracle.com
Cc: beanhuo@micron.com, cang@codeaurora.org, quic_nguyenb@quicinc.com,
linux-scsi@vger.kernel.org, daejun7.park@samsung.com,
adrian.hunter@intel.com, avri.altman@sandisk.com,
James.Bottomley@HansenPartnership.com
On Mon, 2025-08-18 at 08:53 -0700, Bart Van Assche wrote:
>
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>
>
> On 8/18/25 4:56 AM, Peter Wang (王信友) wrote:
> > Should this information also be provided to
> > ufshcd_print_tr for debugging purposes?
> How about the patch below?
>
> Thanks,
>
> Bart.
>
Hi Bart,
Yes, that looks good to me.
Reviewed-by: Peter Wang <peter.wang@mediatek.com>
Thanks.
Peter
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-08-19 6:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-15 16:00 [PATCH] ufs: core: Only collect timestamps if the monitoring functionality is enabled Bart Van Assche
[not found] ` <CGME20250815160114epcas2p43832aa1b9ad0b0efb98ec59f5b568c96@epcms2p8>
2025-08-18 2:36 ` Daejun Park
2025-08-18 15:50 ` Bart Van Assche
2025-08-18 11:56 ` Peter Wang (王信友)
2025-08-18 15:53 ` Bart Van Assche
2025-08-19 6:01 ` Peter Wang (王信友)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).