linux-kernel - Re: [PATCH] cpufreq: schedutil: set sg_policy->next

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20201029112624.zvl6yh7fgqautzry@vireshk-i7>
Date:   Thu, 29 Oct 2020 16:56:24 +0530
From:   Viresh Kumar <viresh.kumar@...aro.org>
To:     zhuguangqing83 <zhuguangqing83@...il.com>
Cc:     rjw@...ysocki.net, mingo@...hat.com, peterz@...radead.org,
        juri.lelli@...hat.com, vincent.guittot@...aro.org,
        dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
        mgorman@...e.de, bristot@...hat.com, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        'zhuguangqing' <zhuguangqing@...omi.com>
Subject: Re: [PATCH] cpufreq: schedutil: set sg_policy->next_freq to the
 final cpufreq

On 29-10-20, 19:17, zhuguangqing83 wrote:
> I think your patch is ok for tackling this problem.

Great.

> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> > index 0c5c61a095f6..8991cc31b011 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -121,13 +121,8 @@ static void sugov_fast_switch(struct sugov_policy *sg_policy, u64 time,
> >  static void sugov_deferred_update(struct sugov_policy *sg_policy, u64 time,
> >                                   unsigned int next_freq)
> >  {
> > -       if (!sugov_update_next_freq(sg_policy, time, next_freq))
> > -               return;
> > -
> > -       if (!sg_policy->work_in_progress) {
> > -               sg_policy->work_in_progress = true;
> > +       if (sugov_update_next_freq(sg_policy, time, next_freq))
> >                 irq_work_queue(&sg_policy->irq_work);
> > -       }
> >  }
> > 
> >  /**
> > @@ -159,6 +154,15 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,
> >         unsigned int freq = arch_scale_freq_invariant() ?
> >                                 policy->cpuinfo.max_freq : policy->cur;
> > 
> > +       /*
> > +        * The previous frequency update didn't go as we expected it to be. Lets
> > +        * start again to make sure we don't miss any updates.
> > +        */
> > +       if (unlikely(policy->cur != sg_policy->next_freq)) {
> > +               sg_policy->next_freq = 0;
> > +               sg_policy->cached_raw_freq = 0;
> > +       }
> > +
> >         freq = map_util_freq(util, freq, max);
> > 
> >         if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
> > @@ -337,8 +341,14 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,
> > 
> >         ignore_dl_rate_limit(sg_cpu, sg_policy);
> > 
> > +       if (!sg_policy->policy->fast_switch_enabled) {
> > +               raw_spin_lock(&sg_policy->update_lock);
> > +               if (sg_policy->work_in_progress)
> > +                       goto unlock;
> > +       }
> > +
> 
> Maybe it's better to bring the following code before the code above.
>          if (!sugov_should_update_freq(sg_policy, time))
>                return;

Maybe not. We want to avoid everything in case a freq-update is on the
way elsewhere as there are other flags we are touching in
sugov_should_update_freq().

I will send a proper patch for this shortly. It would be helpful if
you can give it a go and provide your tested-by.

-- 
viresh