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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 2 Nov 2017 12:06:36 +0000
From:   Chris Redpath <chris.redpath@....com>
To:     Viresh Kumar <viresh.kumar@...aro.org>
Cc:     linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        Morten Rasmussen <morten.rasmussen@....com>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        "Rafael J . Wysocki" <rjw@...ysocki.net>,
        Ingo Molnar <mingo@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v2] cpufreq: schedutil: Examine the correct CPU when we
 update util

Hi Viresh

On 02/11/17 11:40, Viresh Kumar wrote:
> On 02-11-17, 11:38, Chris Redpath wrote:
>> Since:
>> 4296f23ed cpufreq: schedutil: Fix per-CPU structure initialization in
>>   sugov_start()
>
> This is still incorrect. This BUG has nothing to do with 4296f23ed
> AFAICT.
>

According to my diff, this was the commit which switched from assigning
the values directly (and not overwriting the cpu member, which was
introduced in the other commit you reference) to using a memset and
clearing the whole struct. I figured that the fix (which was for a
different issue) inadvertently exposed this, which was what I was trying
to convey here. I can remove this and just reference 674e75411fc2. It's
clear that what's broken is the remote update.

I'll reply with a v3 shortly.

--Chris


>> We lost the value of sg_cpu->cpu which is assigned during
>> sugov_register. The memset in sugov_start overwrites it with zero.
>>
>> The change here was triggered by the commit adding the remote update
>> functionality.
>> 674e75411fc2 ("sched: cpufreq: Allow remote cpufreq callbacks")
>>
>> This leads to always looking at the utilization of CPU0 instead of
>> the one we just updated when we do a utilization update callback.
>>
>> Let's fix this by consolidating the initialization code into
>> sugov_start().
>>
>> Fixes: 674e75411fc2 ("sched: cpufreq: Allow remote cpufreq callbacks")
>> Signed-off-by: Chris Redpath <chris.redpath@....com>
>> Reviewed-by: Patrick Bellasi <patrick.bellasi@....com>
>> Reviewed-by: Brendan Jackman <brendan.jackman@....com>
>> Cc: Rafael J. Wysocki <rjw@...ysocki.net>
>> Cc: Viresh Kumar <viresh.kumar@...aro.org>
>> Cc: Ingo Molnar <mingo@...hat.com>
>> Cc: Peter Zijlstra <peterz@...radead.org>
>> ---
>>   kernel/sched/cpufreq_schedutil.c | 6 +-----
>>   1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
>> index 6c1a7fcfa2a7..dc68a1ccdb33 100644
>> --- a/kernel/sched/cpufreq_schedutil.c
>> +++ b/kernel/sched/cpufreq_schedutil.c
>> @@ -728,6 +728,7 @@ static int sugov_start(struct cpufreq_policy *policy)
>>              struct sugov_cpu *sg_cpu = &per_cpu(sugov_cpu, cpu);
>>
>>              memset(sg_cpu, 0, sizeof(*sg_cpu));
>> +            sg_cpu->cpu = cpu;
>>              sg_cpu->sg_policy = sg_policy;
>>              sg_cpu->flags = SCHED_CPUFREQ_RT;
>>              sg_cpu->iowait_boost_max = policy->cpuinfo.max_freq;
>> @@ -793,11 +794,6 @@ struct cpufreq_governor *cpufreq_default_governor(void)
>>
>>   static int __init sugov_register(void)
>>   {
>> -    int cpu;
>> -
>> -    for_each_possible_cpu(cpu)
>> -            per_cpu(sugov_cpu, cpu).cpu = cpu;
>> -
>>      return cpufreq_register_governor(&schedutil_gov);
>>   }
>>   fs_initcall(sugov_register);
>> --
>> 2.13.1.449.g02a2850
>
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Powered by blists - more mailing lists