From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 65704207673; Wed, 16 Apr 2025 15:19:15 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1744816759; cv=none; b=QKIF1e0tXohbybjfd/5bboslM5iwT6d3ouDHr/JWtBdsE1ThO1q7RBr+yNFHh7LLHx89ANIr3A81Wilx0j5DZraIB7/EP0EXTOh2fnKbjCsIrAGMuw1ZETrYxkUVRu9p95fKtbKH9cee8Kl0mg/TBd3i//3jU2OEHNPYZRcM23g= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1744816759; c=relaxed/simple; bh=FotjLCtH909KmRe7dnW4c9ApzGddHscsXDIL7+9hWbY=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=unxhwNR3bGWM5b17UzhG3HoZL2T0yQWDW+z+1EcHkFOwlYnIXVIlAyC9mP4fcCJ+z7anydNueckPOBgaUXdFh4fHO/Narze+vqCByN755E8/Dy5qOpLof85jKzlcFpsxsND1SFv6+21+Zp3QIGDUM+Mhi8vRt95R6KsVsZ5c/Wk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 8BE401595; Wed, 16 Apr 2025 08:19:12 -0700 (PDT) Received: from [10.1.35.42] (e127648.arm.com [10.1.35.42]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B305A3F66E; Wed, 16 Apr 2025 08:19:12 -0700 (PDT) Message-ID: Date: Wed, 16 Apr 2025 16:19:10 +0100 Precedence: bulk X-Mailing-List: linux-pm@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3] cpufreq: Avoid using inconsistent policy->min and policy->max To: "Rafael J. Wysocki" , Linux PM Cc: LKML , Viresh Kumar , Srinivas Pandruvada , Mario Limonciello , Vincent Guittot , Sultan Alsawaf , Peter Zijlstra , Valentin Schneider , Ingo Molnar References: <5907080.DvuYhMxLoT@rjwysocki.net> Content-Language: en-US From: Christian Loehle In-Reply-To: <5907080.DvuYhMxLoT@rjwysocki.net> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 4/16/25 15:12, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki > > Since cpufreq_driver_resolve_freq() can run in parallel with > cpufreq_set_policy() and there is no synchronization between them, > the former may access policy->min and policy->max while the latter > is updating them and it may see intermediate values of them due > to the way the update is carried out. Also the compiler is free > to apply any optimizations it wants both to the stores in > cpufreq_set_policy() and to the loads in cpufreq_driver_resolve_freq() > which may result in additional inconsistencies. > > To address this, use WRITE_ONCE() when updating policy->min and > policy->max in cpufreq_set_policy() and use READ_ONCE() for reading > them in cpufreq_driver_resolve_freq(). Moreover, rearrange the update > in cpufreq_set_policy() to avoid storing intermediate values in > policy->min and policy->max with the help of the observation that > their new values are expected to be properly ordered upfront. > > Also modify cpufreq_driver_resolve_freq() to take the possible reverse > ordering of policy->min and policy->max, which may happen depending on > the ordering of operations when this function and cpufreq_set_policy() > run concurrently, into account by always honoring the max when it > turns out to be less than the min (in case it comes from thermal > throttling or similar). > > Fixes: 151717690694 ("cpufreq: Make policy min/max hard requirements") > Cc: 5.16+ # 5.16+ > Signed-off-by: Rafael J. Wysocki Reviewed-by: Christian Loehle > --- > > This replaces the last 3 patches in > > https://lore.kernel.org/linux-pm/6171293.lOV4Wx5bFT@rjwysocki.net/ > > v2 -> v3: > * Fold 3 patches into one. > * Drop an unrelated white space fixup change. > * Fix a typo in a comment (Christian). > > v1 -> v2: Cosmetic changes > > --- > drivers/cpufreq/cpufreq.c | 32 +++++++++++++++++++++++++------- > 1 file changed, 25 insertions(+), 7 deletions(-) > > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -495,8 +495,6 @@ > { > unsigned int idx; > > - target_freq = clamp_val(target_freq, policy->min, policy->max); > - > if (!policy->freq_table) > return target_freq; > > @@ -520,7 +518,22 @@ > unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, > unsigned int target_freq) > { > - return __resolve_freq(policy, target_freq, CPUFREQ_RELATION_LE); > + unsigned int min = READ_ONCE(policy->min); > + unsigned int max = READ_ONCE(policy->max); > + > + /* > + * If this function runs in parallel with cpufreq_set_policy(), it may > + * read policy->min before the update and policy->max after the update > + * or the other way around, so there is no ordering guarantee. > + * > + * Resolve this by always honoring the max (in case it comes from > + * thermal throttling or similar). > + */ > + if (unlikely(min > max)) > + min = max; > + > + return __resolve_freq(policy, clamp_val(target_freq, min, max), > + CPUFREQ_RELATION_LE); > } > EXPORT_SYMBOL_GPL(cpufreq_driver_resolve_freq); > > @@ -2338,6 +2351,7 @@ > if (cpufreq_disabled()) > return -ENODEV; > > + target_freq = clamp_val(target_freq, policy->min, policy->max); > target_freq = __resolve_freq(policy, target_freq, relation); > > pr_debug("target for CPU %u: %u kHz, relation %u, requested %u kHz\n", > @@ -2631,11 +2645,15 @@ > * Resolve policy min/max to available frequencies. It ensures > * no frequency resolution will neither overshoot the requested maximum > * nor undershoot the requested minimum. > + * > + * Avoid storing intermediate values in policy->max or policy->min and > + * compiler optimizations around them because they may be accessed > + * concurrently by cpufreq_driver_resolve_freq() during the update. > */ > - policy->min = new_data.min; > - policy->max = new_data.max; > - policy->min = __resolve_freq(policy, policy->min, CPUFREQ_RELATION_L); > - policy->max = __resolve_freq(policy, policy->max, CPUFREQ_RELATION_H); > + WRITE_ONCE(policy->max, __resolve_freq(policy, new_data.max, CPUFREQ_RELATION_H)); > + new_data.min = __resolve_freq(policy, new_data.min, CPUFREQ_RELATION_L); > + WRITE_ONCE(policy->min, new_data.min > policy->max ? policy->max : new_data.min); > + > trace_cpu_frequency_limits(policy); > > cpufreq_update_pressure(policy); > > >