| Message ID | 20240228053421.19700-1-quic_rampraka@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/1] Revert "scsi: ufs: core: Only suspend clock scaling if scaling down" | expand |
On 2/27/24 21:34, Ram Prakash Gupta wrote: > This reverts commit 1d969731b87f122108c50a64acfdbaa63486296e. > Approx 28% random perf IO degradation is observed by suspending clk > scaling only when clks are scaled down. Concern for original fix was > power consumption, which is already taken care by clk gating by putting > the link into hibern8 state. > > Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com> > --- > drivers/ufs/core/ufshcd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index c416826762e9..f6be18db031c 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -1586,7 +1586,7 @@ static int ufshcd_devfreq_target(struct device *dev, > ktime_to_us(ktime_sub(ktime_get(), start)), ret); > > out: > - if (sched_clk_scaling_suspend_work && !scale_up) > + if (sched_clk_scaling_suspend_work) > queue_work(hba->clk_scaling.workq, > &hba->clk_scaling.suspend_work); What is the base kernel version for your tests? Was this patch series included in your kernel tree: "[PATCH V6 0/2] Add CPU latency QoS support for ufs driver" (https://lore.kernel.org/all/20231219123706.6463-1-quic_mnaresh@quicinc.com/)? Thanks, Bart.
On Wed, 2024-02-28 at 11:04 +0530, Ram Prakash Gupta wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > This reverts commit 1d969731b87f122108c50a64acfdbaa63486296e. > Approx 28% random perf IO degradation is observed by suspending clk > scaling only when clks are scaled down. Concern for original fix was > power consumption, which is already taken care by clk gating by > putting > the link into hibern8 state. > > Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com> > --- > drivers/ufs/core/ufshcd.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index c416826762e9..f6be18db031c 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -1586,7 +1586,7 @@ static int ufshcd_devfreq_target(struct device > *dev, > ktime_to_us(ktime_sub(ktime_get(), start)), ret); > > out: > - if (sched_clk_scaling_suspend_work && !scale_up) > + if (sched_clk_scaling_suspend_work) > queue_work(hba->clk_scaling.workq, > &hba->clk_scaling.suspend_work); > > -- > 2.17.1 Hi Ram, It is logic wrong to keep high gear when no read/write traffic. Even high gear turn off clock and enter hibernate, there still have other power consume hardhware to keep IO in high gear, ex. CPU latency, CPU power. Besides, clock scaling is designed for power concern, not for performance. If you want to keep high performance, you can just turn off clock scaling and keep in highest gear. Finally, mediatek dosen't suffer performance drop with this patch. Could you help list the test procedure and performance drop data more detail? I am curious that in what scenario your random drop 28%. And I think your dvfs parameter could be the drop reason. Thanks Peter
[TLDR: I'm adding this report to the list of tracked Linux kernel regressions; the text you find below is based on a few templates paragraphs you might have encountered already in similar form. See link in footer if these mails annoy you.] On 28.02.24 06:34, Ram Prakash Gupta wrote: > This reverts commit 1d969731b87f122108c50a64acfdbaa63486296e. > Approx 28% random perf IO degradation is observed by suspending clk > scaling only when clks are scaled down. Concern for original fix was > power consumption, which is already taken care by clk gating by putting > the link into hibern8 state. Thanks for the report. To be sure the issue doesn't fall through the cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression tracking bot: #regzbot ^introduced 1d969731b87f122108c50a64acfdbaa63486296e #regzbot title scsi: ufs: core: approx 28% random perf IO degradation #regzbot ignore-activity Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat) -- Everything you wanna know about Linux kernel regression tracking: https://linux-regtracking.leemhuis.info/about/#tldr That page also explains what to do if mails like this annoy you.
On 2/29/2024 12:22 AM, Bart Van Assche wrote: > On 2/27/24 21:34, Ram Prakash Gupta wrote: >> This reverts commit 1d969731b87f122108c50a64acfdbaa63486296e. >> Approx 28% random perf IO degradation is observed by suspending clk >> scaling only when clks are scaled down. Concern for original fix was >> power consumption, which is already taken care by clk gating by putting >> the link into hibern8 state. >> >> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com> >> --- >> drivers/ufs/core/ufshcd.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c >> index c416826762e9..f6be18db031c 100644 >> --- a/drivers/ufs/core/ufshcd.c >> +++ b/drivers/ufs/core/ufshcd.c >> @@ -1586,7 +1586,7 @@ static int ufshcd_devfreq_target(struct device >> *dev, >> ktime_to_us(ktime_sub(ktime_get(), start)), ret); >> out: >> - if (sched_clk_scaling_suspend_work && !scale_up) >> + if (sched_clk_scaling_suspend_work) >> queue_work(hba->clk_scaling.workq, >> &hba->clk_scaling.suspend_work); > > What is the base kernel version for your tests? Was this patch series > included in your kernel tree: "[PATCH V6 0/2] Add CPU latency QoS > support for ufs driver" > (https://lore.kernel.org/all/20231219123706.6463-1-quic_mnaresh@quicinc.com/)? > > Thanks, > > Bart. Hi Bart, kernel version used is 6.1 and QoS support for CPU latency is present. Thanks, Ram
On 2/29/2024 1:21 PM, Peter Wang (王信友) wrote: > On Wed, 2024-02-28 at 11:04 +0530, Ram Prakash Gupta wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> This reverts commit 1d969731b87f122108c50a64acfdbaa63486296e. >> Approx 28% random perf IO degradation is observed by suspending clk >> scaling only when clks are scaled down. Concern for original fix was >> power consumption, which is already taken care by clk gating by >> putting >> the link into hibern8 state. >> >> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com> >> --- >> drivers/ufs/core/ufshcd.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c >> index c416826762e9..f6be18db031c 100644 >> --- a/drivers/ufs/core/ufshcd.c >> +++ b/drivers/ufs/core/ufshcd.c >> @@ -1586,7 +1586,7 @@ static int ufshcd_devfreq_target(struct device >> *dev, >> ktime_to_us(ktime_sub(ktime_get(), start)), ret); >> >> out: >> - if (sched_clk_scaling_suspend_work && !scale_up) >> + if (sched_clk_scaling_suspend_work) >> queue_work(hba->clk_scaling.workq, >> &hba->clk_scaling.suspend_work); >> >> -- >> 2.17.1 > > Hi Ram, > > It is logic wrong to keep high gear when no read/write traffic. > Even high gear turn off clock and enter hibernate, there still have > other power consume hardhware to keep IO in high gear, ex. CPU latency, > CPU power. > By CPU latency and power, do you mean the QoS/bus vote from ufs driver? if yes, in that case if clk gating kicks in, it means ufs is already in hibernate state and there is no point keeping the votes (pm qos & bus votes) from ufs driver. And as part of clk gating, qos and other votes on SoC can be removed then no power concern would be there. Now with your implementation, since scaling is not suspended, in next window of devfreq polling will get lowest possible load when active request count is zero, because of which devfreq will scale down the clock. So next request would always be completed with low clock frequency, which is not desirable from performance point of view when power consumption is already taken care in clk gating. > Besides, clock scaling is designed for power concern, not for > performance. If you want to keep high performance, you can just turn > off clock scaling and keep in highest gear. > I think its about striking a right balance. And if there is really a big power concern on mediatek boards, clock gating can be made bit more aggressive there by removing the all votes (qos, bus) from ufs driver on SoC as part of clock gating. Also is clock scaling disabled on mediatek platforms? > Finally, mediatek dosen't suffer performance drop with this patch. > Could you help list the test procedure and performance drop data more > detail? I am curious that in what scenario your random drop 28%. > And I think your dvfs parameter could be the drop reason. > There is no specific environment or procedure used, I am just using antutu benchmark to get the numbers. And random IO numbers are degraded by approx 28%. > Thanks > Peter > > > > > >
On Tue, 2024-03-05 at 12:59 +0530, Ram Prakash Gupta wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > > > On 2/29/2024 1:21 PM, Peter Wang (王信友) wrote: > > On Wed, 2024-02-28 at 11:04 +0530, Ram Prakash Gupta wrote: > >> > >> External email : Please do not click links or open attachments > until > >> you have verified the sender or the content. > >> This reverts commit 1d969731b87f122108c50a64acfdbaa63486296e. > >> Approx 28% random perf IO degradation is observed by suspending > clk > >> scaling only when clks are scaled down. Concern for original fix > was > >> power consumption, which is already taken care by clk gating by > >> putting > >> the link into hibern8 state. > >> > >> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com> > >> --- > >> drivers/ufs/core/ufshcd.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > >> index c416826762e9..f6be18db031c 100644 > >> --- a/drivers/ufs/core/ufshcd.c > >> +++ b/drivers/ufs/core/ufshcd.c > >> @@ -1586,7 +1586,7 @@ static int ufshcd_devfreq_target(struct > device > >> *dev, > >> ktime_to_us(ktime_sub(ktime_get(), start)), ret); > >> > >> out: > >> -if (sched_clk_scaling_suspend_work && !scale_up) > >> +if (sched_clk_scaling_suspend_work) > >> queue_work(hba->clk_scaling.workq, > >> &hba->clk_scaling.suspend_work); > >> > >> -- > >> 2.17.1 > > > > Hi Ram, > > > > It is logic wrong to keep high gear when no read/write traffic. > > Even high gear turn off clock and enter hibernate, there still have > > other power consume hardhware to keep IO in high gear, ex. CPU > latency, > > CPU power. > > > By CPU latency and power, do you mean the QoS/bus vote from ufs > driver? > if yes, in that case if clk gating kicks in, it means ufs is already > in > hibernate state and there is no point keeping the votes (pm qos & > bus > votes) from ufs driver. And as part of clk gating, qos and other > votes > on SoC can be removed then no power concern would be there. > Not only pm qos vote, but also vcore raise is needed for mediatek hw design. And this core voltage is used for our entire SoC. Which means The power impact is very huge for mediatek. > Now with your implementation, since scaling is not suspended, in > next > window of devfreq polling will get lowest possible load when active > request count is zero, because of which devfreq will scale down the > clock. So next request would always be completed with low clock > frequency, which is not desirable from performance point of view If a polling period without any IO traffic, it should scale down right? If IO is busy then scale up. If IO is not busy then scale down. It is basic logic. > when > power consumption is already taken care in clk gating. > > > Besides, clock scaling is designed for power concern, not for > > performance. If you want to keep high performance, you can just > turn > > off clock scaling and keep in highest gear. > > > I think its about striking a right balance. And if there is really a > big > power concern on mediatek boards, clock gating can be made bit more > aggressive there by removing the all votes (qos, bus) from ufs driver > on > SoC as part of clock gating. Also is clock scaling disabled on > mediatek > platforms? > Mediatek has use clock gating. But, there still have a window after enter auto-hibernate and clock gati ng. This window power waste is not reasonalbe. > > Finally, mediatek dosen't suffer performance drop with this patch. > > Could you help list the test procedure and performance drop data > more > > detail? I am curious that in what scenario your random drop 28%. > > And I think your dvfs parameter could be the drop reason. > > > There is no specific environment or procedure used, I am just using > antutu benchmark to get the numbers. And random IO numbers are > degraded > by approx 28%. > Have you try another dvfs setting? For example, enlarge polling period to make scale down harder? Anyway, I think it is not correct to benefit from performance gain through a kernel bug (scale up when no IO on-going) Thanks. Peter
On 3/5/2024 6:25 PM, Peter Wang (王信友) wrote: > On Tue, 2024-03-05 at 12:59 +0530, Ram Prakash Gupta wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> >> >> On 2/29/2024 1:21 PM, Peter Wang (王信友) wrote: >>> On Wed, 2024-02-28 at 11:04 +0530, Ram Prakash Gupta wrote: >>>> >>>> External email : Please do not click links or open attachments >> until >>>> you have verified the sender or the content. >>>> This reverts commit 1d969731b87f122108c50a64acfdbaa63486296e. >>>> Approx 28% random perf IO degradation is observed by suspending >> clk >>>> scaling only when clks are scaled down. Concern for original fix >> was >>>> power consumption, which is already taken care by clk gating by >>>> putting >>>> the link into hibern8 state. >>>> >>>> Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com> >>>> --- >>>> drivers/ufs/core/ufshcd.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c >>>> index c416826762e9..f6be18db031c 100644 >>>> --- a/drivers/ufs/core/ufshcd.c >>>> +++ b/drivers/ufs/core/ufshcd.c >>>> @@ -1586,7 +1586,7 @@ static int ufshcd_devfreq_target(struct >> device >>>> *dev, >>>> ktime_to_us(ktime_sub(ktime_get(), start)), ret); >>>> >>>> out: >>>> -if (sched_clk_scaling_suspend_work && !scale_up) >>>> +if (sched_clk_scaling_suspend_work) >>>> queue_work(hba->clk_scaling.workq, >>>> &hba->clk_scaling.suspend_work); >>>> >>>> -- >>>> 2.17.1 >>> >>> Hi Ram, >>> >>> It is logic wrong to keep high gear when no read/write traffic. >>> Even high gear turn off clock and enter hibernate, there still have >>> other power consume hardhware to keep IO in high gear, ex. CPU >> latency, >>> CPU power. >>> >> By CPU latency and power, do you mean the QoS/bus vote from ufs >> driver? >> if yes, in that case if clk gating kicks in, it means ufs is already >> in >> hibernate state and there is no point keeping the votes (pm qos & >> bus >> votes) from ufs driver. And as part of clk gating, qos and other >> votes >> on SoC can be removed then no power concern would be there. >> > > Not only pm qos vote, but also vcore raise is needed for mediatek hw > design. And this core voltage is used for our entire SoC. Which means > The power impact is very huge for mediatek. > > >> Now with your implementation, since scaling is not suspended, in >> next >> window of devfreq polling will get lowest possible load when active >> request count is zero, because of which devfreq will scale down the >> clock. So next request would always be completed with low clock >> frequency, which is not desirable from performance point of view > > If a polling period without any IO traffic, it should scale down right? > If IO is busy then scale up. > If IO is not busy then scale down. > It is basic logic. > > >> when >> power consumption is already taken care in clk gating. >> >>> Besides, clock scaling is designed for power concern, not for >>> performance. If you want to keep high performance, you can just >> turn >>> off clock scaling and keep in highest gear. >>> >> I think its about striking a right balance. And if there is really a >> big >> power concern on mediatek boards, clock gating can be made bit more >> aggressive there by removing the all votes (qos, bus) from ufs driver >> on >> SoC as part of clock gating. Also is clock scaling disabled on >> mediatek >> platforms? >> > > Mediatek has use clock gating. > But, there still have a window after enter auto-hibernate and clock > gati > ng. This window power waste is not reasonalbe. > > >>> Finally, mediatek dosen't suffer performance drop with this patch. >>> Could you help list the test procedure and performance drop data >> more >>> detail? I am curious that in what scenario your random drop 28%. >>> And I think your dvfs parameter could be the drop reason. >>> >> There is no specific environment or procedure used, I am just using >> antutu benchmark to get the numbers. And random IO numbers are >> degraded >> by approx 28%. >> > > Have you try another dvfs setting? > For example, enlarge polling period to make scale down harder? > > Anyway, I think it is not correct to benefit from performance gain > through a kernel bug (scale up when no IO on-going) > > > Thanks. > Peter > Hi Peter, I tried different dvfs settings, none is helping including enlarged polling period time, its degrading perf numbers as its taking longer time to scale up when the load is high and clk is low. I checked from power side on qualcomm boards, suspending with zero request is not impacting power hence I am consider a vops to add which can help your use case too, I tested this vops and it works fine on qualcomm boards. here is a small snippet of a different approach using vops, which I am planning to push under a separate mail subject to remove this deadlock between mediatek and qualcomm, scaling config. - if (sched_clk_scaling_suspend_work && !scale_up) + if (sched_clk_scaling_suspend_work && hba->clk_scaling.no_req_suspend) + queue_work(hba->clk_scaling.workq, + &hba->clk_scaling.suspend_work); + else if (sched_clk_scaling_suspend_work && !scale_up) Here no_req_suspend would be false by default, so would hit else if case, which is desirable for mediatek boards. For qualcomm, no_req_suspend would set it to true via vops. please let me know if this is ok for you. Thanks, Ram
On Wed, 2024-04-24 at 14:44 +0530, Ram Prakash Gupta wrote: > > Hi Peter, > > I tried different dvfs settings, none is helping including enlarged > polling period time, its degrading perf numbers as its taking longer > time to scale up when the load is high and clk is low. > > I checked from power side on qualcomm boards, suspending with zero > request is not impacting power hence I am consider a vops to add > which > can help your use case too, I tested this vops and it works fine on > qualcomm boards. > > here is a small snippet of a different approach using vops, which I > am > planning to push under a separate mail subject to remove this > deadlock > between mediatek and qualcomm, scaling config. > > - if (sched_clk_scaling_suspend_work && !scale_up) > + if (sched_clk_scaling_suspend_work && > hba->clk_scaling.no_req_suspend) > + queue_work(hba->clk_scaling.workq, > + &hba->clk_scaling.suspend_work); > Hi Ram, It is weird for me that if no_req_suspend is true, queue suspend work? Dosen't "no_req_suspend" simply mean "do not suspend"? Thanks Peter > + else if (sched_clk_scaling_suspend_work && !scale_up) > > Here no_req_suspend would be false by default, so would hit else if > case, which is desirable for mediatek boards. For qualcomm, > no_req_suspend would set it to true via vops. please let me know if > this > is ok for you. > > Thanks, > Ram
On 4/25/2024 6:45 PM, Peter Wang (王信友) wrote: > On Wed, 2024-04-24 at 14:44 +0530, Ram Prakash Gupta wrote: >> >> Hi Peter, >> >> I tried different dvfs settings, none is helping including enlarged >> polling period time, its degrading perf numbers as its taking longer >> time to scale up when the load is high and clk is low. >> >> I checked from power side on qualcomm boards, suspending with zero >> request is not impacting power hence I am consider a vops to add >> which >> can help your use case too, I tested this vops and it works fine on >> qualcomm boards. >> >> here is a small snippet of a different approach using vops, which I >> am >> planning to push under a separate mail subject to remove this >> deadlock >> between mediatek and qualcomm, scaling config. >> >> - if (sched_clk_scaling_suspend_work && !scale_up) >> + if (sched_clk_scaling_suspend_work && >> hba->clk_scaling.no_req_suspend) >> + queue_work(hba->clk_scaling.workq, >> + &hba->clk_scaling.suspend_work); >> > > Hi Ram, > > It is weird for me that if no_req_suspend is true, queue suspend work? > Dosen't "no_req_suspend" simply mean "do not suspend"? > > Thanks > Peter Hi Peter, well I intended to shorthand the naming of macro for "no request suspend", meaning suspend when there is no request, which I want to control in the scaling logic. I am open to suggestion on name of the variable if you suggest any other meaningful name. and earlier the logic was, scaling was getting suspended when there were no request, that we would like to keep via vops for qcom board, rest everyone can use the default existing logic. Thanks, Ram > > > >> + else if (sched_clk_scaling_suspend_work && !scale_up) >> >> Here no_req_suspend would be false by default, so would hit else if >> case, which is desirable for mediatek boards. For qualcomm, >> no_req_suspend would set it to true via vops. please let me know if >> this >> is ok for you. >> >> Thanks, >> Ram
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index c416826762e9..f6be18db031c 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -1586,7 +1586,7 @@ static int ufshcd_devfreq_target(struct device *dev, ktime_to_us(ktime_sub(ktime_get(), start)), ret); out: - if (sched_clk_scaling_suspend_work && !scale_up) + if (sched_clk_scaling_suspend_work) queue_work(hba->clk_scaling.workq, &hba->clk_scaling.suspend_work);
This reverts commit 1d969731b87f122108c50a64acfdbaa63486296e. Approx 28% random perf IO degradation is observed by suspending clk scaling only when clks are scaled down. Concern for original fix was power consumption, which is already taken care by clk gating by putting the link into hibern8 state. Signed-off-by: Ram Prakash Gupta <quic_rampraka@quicinc.com> --- drivers/ufs/core/ufshcd.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)