| Message ID | 20200611052109.22700-1-stanley.chu@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] scsi: ufs: Fix imprecise load calculation in devfreq window | expand |
> > The UFS load calculation is based on "total_time" and "busy_time" in a > devfreq window. However, the source of time is different for both > parameters: "busy_time" is assigned from "jiffies" thus has different > accuracy from "total_time" which is assigned from ktime_get(). > > Besides, the time of window boundary is not exactly the same as > the starting busy time in this window if UFS is actually busy > in the beginning of the window. A similar accuracy error may also > happen for the end of busy time in current window. > > To guarantee the precision of load calculation, we need to > > 1. Align time accuracy of both devfreq_dev_status.total_time and > devfreq_dev_status.busy_time. For example, use "ktime_get()" > directly. > > 2. Align below timelines, > - The beginning time of devfreq windows > - The beginning of busy time in a new window > - The end of busy time in the current window > > Fixes: a3cd5ec55f6c ("scsi: ufs: add load based scaling of UFS gear") > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> Reviewed-by: Avri Altman <avri.altman@wdc.com> Just a small nit. > - stat->total_time = jiffies_to_usecs((long)jiffies - > - (long)scaling->window_start_t); > + stat->total_time = ktime_to_us(curr_t) - scaling->window_start_t; ktime_sub ? Thanks, Avri
Hi Avri, Thanks for the review. On Thu, 2020-06-11 at 08:03 +0000, Avri Altman wrote: > > > > Fixes: a3cd5ec55f6c ("scsi: ufs: add load based scaling of UFS gear") > > Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> > Reviewed-by: Avri Altman <avri.altman@wdc.com> > > Just a small nit. > > > - stat->total_time = jiffies_to_usecs((long)jiffies - > > - (long)scaling->window_start_t); > > + stat->total_time = ktime_to_us(curr_t) - scaling->window_start_t; > ktime_sub ? scaling->window_start_t is already in "us" thus ktime_sub() is not suitable here. Another way is changing scaling->window_start_t as type "ktime_t". This is worth to do because of a little performance gain. I will change it in next version. Thanks, Stanley Chu
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index ad4fc829cbb2..bf5aaf334ccd 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -1314,6 +1314,7 @@ static int ufshcd_devfreq_get_dev_status(struct device *dev, unsigned long flags; struct list_head *clk_list = &hba->clk_list_head; struct ufs_clk_info *clki; + ktime_t curr_t; if (!ufshcd_is_clkscaling_supported(hba)) return -EINVAL; @@ -1321,6 +1322,7 @@ static int ufshcd_devfreq_get_dev_status(struct device *dev, memset(stat, 0, sizeof(*stat)); spin_lock_irqsave(hba->host->host_lock, flags); + curr_t = ktime_get(); if (!scaling->window_start_t) goto start_window; @@ -1332,18 +1334,17 @@ static int ufshcd_devfreq_get_dev_status(struct device *dev, */ stat->current_frequency = clki->curr_freq; if (scaling->is_busy_started) - scaling->tot_busy_t += ktime_to_us(ktime_sub(ktime_get(), + scaling->tot_busy_t += ktime_to_us(ktime_sub(curr_t, scaling->busy_start_t)); - stat->total_time = jiffies_to_usecs((long)jiffies - - (long)scaling->window_start_t); + stat->total_time = ktime_to_us(curr_t) - scaling->window_start_t; stat->busy_time = scaling->tot_busy_t; start_window: - scaling->window_start_t = jiffies; + scaling->window_start_t = ktime_to_us(curr_t); scaling->tot_busy_t = 0; if (hba->outstanding_reqs) { - scaling->busy_start_t = ktime_get(); + scaling->busy_start_t = curr_t; scaling->is_busy_started = true; } else { scaling->busy_start_t = 0; @@ -1877,6 +1878,7 @@ static void ufshcd_exit_clk_gating(struct ufs_hba *hba) static void ufshcd_clk_scaling_start_busy(struct ufs_hba *hba) { bool queue_resume_work = false; + ktime_t curr_t = ktime_get(); if (!ufshcd_is_clkscaling_supported(hba)) return; @@ -1892,13 +1894,13 @@ static void ufshcd_clk_scaling_start_busy(struct ufs_hba *hba) &hba->clk_scaling.resume_work); if (!hba->clk_scaling.window_start_t) { - hba->clk_scaling.window_start_t = jiffies; + hba->clk_scaling.window_start_t = ktime_to_us(curr_t); hba->clk_scaling.tot_busy_t = 0; hba->clk_scaling.is_busy_started = false; } if (!hba->clk_scaling.is_busy_started) { - hba->clk_scaling.busy_start_t = ktime_get(); + hba->clk_scaling.busy_start_t = curr_t; hba->clk_scaling.is_busy_started = true; } }
The UFS load calculation is based on "total_time" and "busy_time" in a devfreq window. However, the source of time is different for both parameters: "busy_time" is assigned from "jiffies" thus has different accuracy from "total_time" which is assigned from ktime_get(). Besides, the time of window boundary is not exactly the same as the starting busy time in this window if UFS is actually busy in the beginning of the window. A similar accuracy error may also happen for the end of busy time in current window. To guarantee the precision of load calculation, we need to 1. Align time accuracy of both devfreq_dev_status.total_time and devfreq_dev_status.busy_time. For example, use "ktime_get()" directly. 2. Align below timelines, - The beginning time of devfreq windows - The beginning of busy time in a new window - The end of busy time in the current window Fixes: a3cd5ec55f6c ("scsi: ufs: add load based scaling of UFS gear") Signed-off-by: Stanley Chu <stanley.chu@mediatek.com> --- drivers/scsi/ufs/ufshcd.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)