CPUfreq lockdep issue

Hi,

The Intel P-state driver has a lockdep issue as described below. It could in theory cause a deadlock if initialization and suspend were to be performed simultaneously. Conflicting calling paths are as follows:

intel_pstate_init(...)

	...cpufreq_online(...)
		down_write(&policy->rwsem); // Locks policy->rwsem
		...
		cpufreq_init_policy(policy);
			...intel_pstate_hwp_set();
				get_online_cpus(); // Temporarily locks cpu_hotplug.lock

		...
		up_write(&policy->rwsem);

pm_suspend(...)

	...disable_nonboot_cpus()
		_cpu_down()
			cpu_hotplug_begin(); // Locks cpu_hotplug.lock
			__cpu_notify(CPU_DOWN_PREPARE, ...);
				...cpufreq_offline_prepare();
					down_write(&policy->rwsem); // Locks policy->rwsem

Quickly looking at the code, some refactoring has to be done to fix the issue. I think it would a good idea to document some of the driver callbacks related to what locks are held etc. in order to avoid future situations like this.

Because get_online_cpus() is of recursive nature and the way it currently works, adding wider get_online_cpus() scope up around cpufreq_online() does not fix the issue because it only momentarily locks cpu_hotplug.lock and proceeds to do so again at next call.

Moving get_online_cpus() completely away from pstate_hwp_set() and assuring it is called higher in the call chain might be a viable solution. Then it could be made sure get_online_cpus() is not called while policy->rwsem is being held already.

Do you think that would be an appropriate way of fixing it?

Quoted 6 lines · Joonas Lahtinen

Hi,

The Intel P-state driver has a lockdep issue as described below. It could in theory cause a deadlock if initialization and suspend were to be performed simultaneously. Conflicting calling paths are as follows:

Quoted 7 lines

intel_pstate_init(...)

	...cpufreq_online(...)
		down_write(&policy->rwsem); // Locks policy->rwsem
		...
		cpufreq_init_policy(policy);
			...intel_pstate_hwp_set();
				get_online_cpus(); // Temporarily locks cpu_hotplug.lock

Why is this one required?

Quoted 22 lines

		...
		up_write(&policy->rwsem);

pm_suspend(...)

	...disable_nonboot_cpus()
		_cpu_down()
			cpu_hotplug_begin(); // Locks cpu_hotplug.lock
			__cpu_notify(CPU_DOWN_PREPARE, ...);
				...cpufreq_offline_prepare();
					down_write(&policy->rwsem); // Locks policy->rwsem

Quickly looking at the code, some refactoring has to be done to fix the issue. I think it would a good idea to document some of the driver callbacks related to what locks are held etc. in order to avoid future situations like this.

Because get_online_cpus() is of recursive nature and the way it currently works, adding wider get_online_cpus() scope up around cpufreq_online() does not fix the issue because it only momentarily locks cpu_hotplug.lock and proceeds to do so again at next call.

Quoted 4 lines

Moving get_online_cpus() completely away from pstate_hwp_set() and assuring it is called higher in the call chain might be a viable solution. Then it could be made sure get_online_cpus() is not called while policy->rwsem is being held already.

I don't think that will be a good solution. So what you are essentially saying is, take policy->rwsem after get_online_cpus() only.

Quoted 2 lines

Do you think that would be an appropriate way of fixing it?

At least I don't. Why do we need to call get_online_cpus() intel-pstate governor ?

Quoted 15 lines · Viresh Kumar

Quoted 6 lines · Joonas Lahtinen

Hi,

The Intel P-state driver has a lockdep issue as described below. It could in theory cause a deadlock if initialization and suspend were to be performed simultaneously. Conflicting calling paths are as follows:

Quoted 7 lines

intel_pstate_init(...)

	...cpufreq_online(...)
		down_write(&policy->rwsem); // Locks policy->rwsem
		...
		cpufreq_init_policy(policy);
			...intel_pstate_hwp_set();
				get_online_cpus(); // Temporarily locks cpu_hotplug.lock

Quoted 1 line

Why is this one required?

Otherwise CPUs could be added or removed during the execution of intel_pstate_hwp_set(), which has the following code:

        get_online_cpus();

        for_each_online_cpu(cpu) {
		...
		wrmsrl_on_cpu(cpu, MSR_HWP_REQUEST, value);
	}

Quoted 26 lines

Quoted 26 lines

		...
		up_write(&policy->rwsem);

pm_suspend(...)

	...disable_nonboot_cpus()
		_cpu_down()
			cpu_hotplug_begin(); // Locks cpu_hotplug.lock
			__cpu_notify(CPU_DOWN_PREPARE, ...);
				...cpufreq_offline_prepare();
					down_write(&policy->rwsem); // Locks policy->rwsem

Quickly looking at the code, some refactoring has to be done to fix the issue. I think it would a good idea to document some of the driver callbacks related to what locks are held etc. in order to avoid future situations like this.

Because get_online_cpus() is of recursive nature and the way it currently works, adding wider get_online_cpus() scope up around cpufreq_online() does not fix the issue because it only momentarily locks cpu_hotplug.lock and proceeds to do so again at next call.

Moving get_online_cpus() completely away from pstate_hwp_set() and assuring it is called higher in the call chain might be a viable solution. Then it could be made sure get_online_cpus() is not called while policy->rwsem is being held already.

Adding the maintainers of the driver in cc now.

Hi Guys,

Joonas reported a lockdep between cpufreq and intel-pstate driver and we are looking for probable solutions.

I failed to understand why should we run intel_pstate_hwp_set() for each online CPU, while the frequency is changed only for a group of CPUs that belong to a policy.

Ofcourse intel_pstate_hwp_set() is
required to be run for all CPUs, while the sysfs files are touched. And so, do we have a problem with below diff?

Quoted 1 line · Viresh Kumar

Adding the maintainers of the driver in cc now.

[...]
Hi Viresh,

Quoted 8 lines

Hi Guys,

Joonas reported a lockdep between cpufreq and intel-pstate driver and we are looking for probable solutions.

I failed to understand why should we run intel_pstate_hwp_set() for each online CPU, while the frequency is changed only for a group of CPUs that belong to a policy.

Don't need from set_policy interface.

Quoted 3 lines

Ofcourse intel_pstate_hwp_set() is
required to be run for all CPUs, while the sysfs files are touched. And so, do we have a problem with below diff?

I gave a quick test, the change works fine.

Hi,

The Intel P-state driver has a lockdep issue as described below. It could in theory cause a deadlock if initialization and suspend were to be performed simultaneously. Conflicting calling paths are as follows: