Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2] cpufreq: schedutil: Examine the correct CPU when we
 update util
To:     Viresh Kumar <viresh.kumar@linaro.org>
Cc:     linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org,
        Morten Rasmussen <morten.rasmussen@arm.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        "Rafael J . Wysocki" <rjw@rjwysocki.net>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>
References: <20171102111022.GB4240@vireshk-i7>
 <20171102113840.17439-1-chris.redpath@arm.com>
 <20171102114048.GC4240@vireshk-i7>
From:   Chris Redpath <chris.redpath@arm.com>
Message-ID: <037169f3-61f9-b29e-9561-0f7dba213cd5@arm.com>
Date:   Thu, 2 Nov 2017 12:06:36 +0000
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.4.0
MIME-Version: 1.0
In-Reply-To: <20171102114048.GC4240@vireshk-i7>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: quoted-printable
Received-SPF: None (protection.outlook.com: arm.com does not designate
 permitted sender hosts)
SpamDiagnosticOutput: 1:99
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Nov 2017 12:06:39.0193 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 6268a7f6-3ba7-45cb-bb72-08d521ea2c85
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f34e5979-57d9-4aaa-ad4d-b122a662184d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM4PR0801MB2737
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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@arm.com>
>> Reviewed-by: Patrick Bellasi <patrick.bellasi@arm.com>
>> Reviewed-by: Brendan Jackman <brendan.jackman@arm.com>
>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net>
>> Cc: Viresh Kumar <viresh.kumar@linaro.org>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Peter Zijlstra <peterz@infradead.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_sch=
edutil.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 =3D &per_cpu(sugov_cpu, cpu);
>>
>>              memset(sg_cpu, 0, sizeof(*sg_cpu));
>> +            sg_cpu->cpu =3D cpu;
>>              sg_cpu->sg_policy =3D sg_policy;
>>              sg_cpu->flags =3D SCHED_CPUFREQ_RT;
>>              sg_cpu->iowait_boost_max =3D policy->cpuinfo.max_freq;
>> @@ -793,11 +794,6 @@ struct cpufreq_governor *cpufreq_default_governor(v=
oid)
>>
>>   static int __init sugov_register(void)
>>   {
>> -    int cpu;
>> -
>> -    for_each_possible_cpu(cpu)
>> -            per_cpu(sugov_cpu, cpu).cpu =3D 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 confid=
ential and may also be privileged. If you are not the intended recipient, p=
lease 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.

From 1582954361879693163@xxx Thu Nov 02 11:41:58 +0000 2017
X-GM-THRID: 1582951591518861812
X-Gmail-Labels: Inbox,Category Forums,HistoricalUnread