Re: 6.11/regression/bisected - The commit c1385c1f0ba3 caused a new possible recursive locking detected warning at computer boot.

[Terry Bowman]

On 7/25/24 12:13, Jonathan Cameron wrote:
> On Tue, 23 Jul 2024 18:20:06 +0100
> Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:
> 
>> On Tue, 23 Jul 2024 11:24:56 +0100
>> Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> wrote:
>>
>>> On Tue, 23 Jul 2024 00:36:18 +0500
>>> Mikhail Gavrilov <mikhail.v.gavrilov@xxxxxxxxx> wrote:
>>>   
>>>> Hi,
>>>> The first Fedora update to the 6.11 kernel
>>>> (kernel-debug-6.11.0-0.rc0.20240716gitd67978318827.2.fc41.x86_64)
>>>> brings a new warning: possible recursive locking detected.    
>>>
>>> Hi Mikhail,
>>>
>>> Thanks for the report.
>>>
>>> This is an interesting corner and perhaps reflects a flawed
>>> assumption we were making that for this path anything that can happen for an
>>> initially present CPU can also happen for a hotplugged one. On the hotplugged
>>> path the lock was always held and hence the static_key_enable() would
>>> have failed.
>>>
>>> I'm somewhat stumped on working out why this path couldn't happen
>>> for a hotplugged CPU so why this is a new problem?
>>>
>>> Maybe this is just a case of no one is providing _CPC for CPUs in virtual
>>> machines so the path wasn't seen? QEMU doesn't generate ACPI tables with
>>> _CPC today, so maybe that's it.
>>>
>>> So maybe this is has revealed an existing latent  bug.  There have been
>>> QEMU patches for _CPC in the past but never merged. I'll hack them
>>> into an x86 virtual machine and see if we hit the same bug you have
>>> here before and after the series.
>>>
>>> Either way obviously we need to fix it for the current kernel (and maybe
>>> backport the fix if I can verify it's a latent bug).  I'll get a test
>>> setup running asap and see if I can replicate.
>>>
>>> +CC x86 maintainers.  
>>
>> It will take me a little longer to emulate a suitable setup to hit the
>> AMD case on (I have it run on arm64 now, but no similar issue occurs)
>>
>> Ultimately the problem is occurring in arch_init_invariance_cppc
>> I note that the arm64 version of that topology_init_cpu_capacity_cppc
>> delays some activity via a work queue specifically to avoid some
>> locking issues.
>>
>> On AMD systems arch_init_invariance_cppc is defined
>> as init_freq_invariance_cppc which calls amd_set_max_freq_ratio just
>> once (there is a static bool) which in turn calls
>> freq_invariance_set_perf_ratio() / freq_invariance_enable()
>>
>> Until I have a setup to test on I'm not going to draw firm conclusions
>> but how much would it matter if we set that static key a bit late
>> via a workqueue?  In the meantime go with a default value similar to
>> that disable_freq_invariance_work sets (which is done via a workqueue).
>>
>> The intel equivalent is called via an early_init() so not
>> the hotplug path.
>>
>> Any hints on from people familiar with this code would be most
>> welcome.  Whilst git suggests tglx touched these paths most recently that
>> was in tidying them up to split the Intel and AMD paths.
>>
> 
> Hi Mikhail.
> 
> So the short story, ignoring the journey (which should only be described
> with beer in hand), is that I now have an emulated test setup in QEMU
> that fakes enough of the previously missing bits to bring up this path
> and can trigger the splat you shared.  With the below fix I can get to
> something approaching a running system.
> 
> However, without more work the emulation isn't actually doing any control
> of frequency etc so I have no idea if the code actually works after this
> patch.
> 
> If you are in a position to test a patch, could you try the following?
> 
> One bit I need to check out tomorrow is to make sure this doesn't race with the
> workfn that is used to tear down the same static key on error.
> 
> From 8f7ad4c73954aae74265a3ec50a1d56e0c56050d Mon Sep 17 00:00:00 2001
> From: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> Date: Thu, 25 Jul 2024 17:56:00 +0100
> Subject: [RFC PATCH] x86/aperfmperf: Push static_branch_enable(&arch_scale_freq_key) onto work queue
> 
> This to avoid a deadlock reported by lockdep.
> 
> TODO: Fix up this commit message before posting to actually give
> some details and tags etc.
> 
> Reported-by: Mikhail Gavrilov <mikhail.v.gavrilov@xxxxxxxxx>
> Signed-off-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/aperfmperf.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/aperfmperf.c b/arch/x86/kernel/cpu/aperfmperf.c
> index b3fa61d45352..41c729d3517c 100644
> --- a/arch/x86/kernel/cpu/aperfmperf.c
> +++ b/arch/x86/kernel/cpu/aperfmperf.c
> @@ -300,15 +300,22 @@ static void register_freq_invariance_syscore_ops(void)
>  static inline void register_freq_invariance_syscore_ops(void) {}
>  #endif
>  
> +static void enable_freq_invariance_workfn(struct work_struct *work)
> +{
> +	static_branch_enable(&arch_scale_freq_key);
> +	register_freq_invariance_syscore_ops();
> +	pr_info("Estimated ratio of average max frequency by base frequency (times 1024): %llu\n", arch_max_freq_ratio);
> +}
> +static DECLARE_WORK(enable_freq_invariance_work,
> +		    enable_freq_invariance_workfn);
> +
>  static void freq_invariance_enable(void)
>  {
>  	if (static_branch_unlikely(&arch_scale_freq_key)) {
>  		WARN_ON_ONCE(1);
>  		return;
>  	}
> -	static_branch_enable(&arch_scale_freq_key);
> -	register_freq_invariance_syscore_ops();
> -	pr_info("Estimated ratio of average max frequency by base frequency (times 1024): %llu\n", arch_max_freq_ratio);
> +	schedule_work(&enable_freq_invariance_work);
>  }
>  
>  void freq_invariance_set_perf_ratio(u64 ratio, bool turbo_disabled)

Hi Jonathan,

I was able to recreate the problem using base d67978318827 with a Ryzen laptop.

I was unable to recreate the issue using base 72ec37390a71 with the same Ryzen laptop.

Ive attached the dmesg logs and dmidecode.

Regards,
Terry

Attachment: dmesg.tgz
Description: application/compressed-tar