Bug 2615

Summary:	Adding SMP support for ACPI processor driver
Product:	ACPI	Reporter:	Venkatesh Pallipadi (venki)
Component:	Power-Processor	Assignee:	acpi_power-processor
Status:	CLOSED CODE_FIX
Severity:	normal	CC:	linux
Priority:	P2
Hardware:	i386
OS:	Linux
Kernel Version:	2.6	Subsystem:
Regression:	---	Bisected commit-id:
Bug Depends on:
Bug Blocks:	2712
Attachments:	02acpiid.patch 03estsmp.patch 02acpiid-new.patch 02acpiid-new.patch export_symbols_for_modules.patch One combined patch

Description Venkatesh Pallipadi 2004-04-28 22:25:51 UTC

Distribution: 
Base 2.6 kernel

Hardware Environment:
Software Environment:

Problem Description:
There are 2 issues here:
1)
MADT has 8 bit ACPIID, which refers to the CPU namespace in ACPI. 
This id should be used by ACPI processor driver to look up corresponding CPU 
namespace. As of now the ACPIID is being ignored by the kernel. And the 
processor driver assumes that ACPIID is in the same order as smp_processor_id
(). The processor driver is used by i386, x86-64 and ia64 archs. The first 
patch below proposes a fix for this problem.

2)
The ACPI P-state driver, does not work on SMP systems. As per the current 
cpufreq architecture, A process running on CPU X can get/set frequency of CPU 
Y. It is the responsibility of low level P-state driver (ACPI P-state driver 
in this case) to do whatever necessary (switch to proper CPU) and get/set the 
frequency. The second patch below adds this capability to ACPI P-state driver. 
The mechanism is very much similar to the ones in other frequency drivers like 
p4-clockmod.

Steps to reproduce:

Comment 1 Venkatesh Pallipadi 2004-04-28 22:27:18 UTC

Created attachment 2744 [details]
02acpiid.patch

Comment 2 Venkatesh Pallipadi 2004-04-28 22:27:45 UTC

Created attachment 2745 [details]
03estsmp.patch

Comment 3 Dominik Brodowski 2004-04-28 23:35:25 UTC

patch 1: looks good

patch 2: A couple of comments:
a) there's no place in the ACPI specification, IIRC, that states that P-States
transitions need to be run on the affected CPU - maybe clear that up for
ACPIspec30? Also, if you talk to the guys writing the new specification: a
method for detecting the _current_ P-State would be very helpful... I know it
works on Centrino systems, it does not work on other P-State enabled systems,
and PERF_STATUS is only defined after a P-State transition...

b) set_cpus_allowed() should be safe here. Of course, some evil superuser may
try to move the thread away; or a CPU hotplug event might grab the thread away
from the correct CPU. But we can't protect from evil superusers anyway...

Comment 4 Venkatesh Pallipadi 2004-04-29 12:20:50 UTC

Created attachment 2748 [details]
02acpiid-new.patch

Fixes the UP compile bug in the previous version

Comment 5 Venkatesh Pallipadi 2004-04-29 15:31:35 UTC

About Dominik's comment b) above.

Agreed that is a real corner case. 

I had another implementation, which didn't have this corner case. Where in I 
did an smp_call_function() and only the target CPU does the frequency change, 
while rest of the CPUs nop on that function. But, finally decided that it was 
way too much overhead. If we have a generic interface, where in we can do 
smp_call_function kind of thing on targetted CPUs, then that should be the 
ideal solution, I believe.

Comment 6 Venkatesh Pallipadi 2004-04-29 16:19:50 UTC

Checking the ACPI 2.0b spec..

Around section 8.3.3.1 I says:
"Perfomance objects must be present under each processor object in the system 
for OSPM to utilize this feature"
"_PCT - This optional object declares an interface that allows OSPM to 
transition the processor into a performance state"

Its not totally clear about control event (write to IO or FFH) should happen 
on the CPU that wants to change the state. But, I feel two of the above 
statments together comes close to saying that.

Comment 7 Venkatesh Pallipadi 2004-04-29 17:56:58 UTC

Created attachment 2750 [details]
02acpiid-new.patch

Comment 8 Venkatesh Pallipadi 2004-05-16 16:54:29 UTC

As Anil Keshavamurthy pointed out, we have to have additional  
lock/unlock_cpu_hotplug(), while migrating to the target CPU. This is required 
in oorder to prevent any forced migrations by CPU hotplug events (unlikely 
though), in which case we may end up reducing the frequency of wrong CPU. Will 
attach the additional patch to do this soon.

Comment 9 Dominik Brodowski 2004-05-16 23:08:40 UTC

Please consider doing that in drivers/cpufreq/cpufreq.c

Comment 10 Venkatesh Pallipadi 2004-05-18 21:45:50 UTC

Created attachment 2906 [details]
export_symbols_for_modules.patch

Additional patch over the above two, to export the symbols used in
acpi/processor.c. needed when it is compiled as a module. Thanks to Anil
Keshavamurthy for identifying the bug and providing this patch.

Comment 11 Venkatesh Pallipadi 2004-07-09 13:28:24 UTC

Created attachment 3325 [details]
One combined patch

One combined patch for this. The combined patch also has one coding style
change, based on the feedback from Len.

Comment 12 Anil S Keshavamurthy 2004-07-26 12:49:48 UTC

I am waiting for this patch. When can I expect to see this patch in mm tree?

Comment 13 Len Brown 2004-11-04 13:08:04 UTC

shipped in 2.6.9
closing