RE: [Xen-devel] [PATCH] use per-cpu variables in cpufreq

["Liu, Jinsong" <jinsong.liu@xxxxxxxxx>]

Tian, Kevin wrote:
>> From: Keir Fraser
>> Sent: Saturday, May 28, 2011 3:53 PM
>> 
>> On 27/05/2011 14:29, "Juergen Gross" <juergen.gross@xxxxxxxxxxxxxx>
>> wrote: 
>> 
>>> On 05/27/11 15:11, Keir Fraser wrote:
>>>> On 27/05/2011 12:11, "Juergen Gross"<juergen.gross@xxxxxxxxxxxxxx>
>>>> wrote: 
>>>> 
>>>>> The cpufreq driver used some local arrays indexed by cpu number.
>>>>> This patch replaces those arrays by per-cpu variables. The AMD
>>>>> and INTEL specific parts used different per-cpu data structures
>>>>> with nearly identical semantics. Fold the two structures into one
>>>>> by adding a generic architecture data 
>> item.
>>>> Xen's per-cpu data gets freed across cpu offline/online, whereas
>>>> cpu-indexed arrays of course do not. Will the cpufreq state be
>>>> correctly handled across offline/online if we switch to per-cpu
>>>> vars? 
>>> 
>>> As far as I could see, yes. The data should only be used for cpus
>>> with a valid acpid->cpuid translation, which is created when a cpu
>>> is going online and destroyed when it is going offline again.
>> 
>> That simply isn't true. acpiid_to_apicid[] is populated during boot
>> and entries are never destroyed. 
>> 
>> Specifically, my fear is that this data gets pushed into the
>> hypervisor once-only during dom0 boot (via
>> XENPF_set_processor_pminfo). If it is freed during processor
>> offline, we lose it forever and have no power management 
> 
> exactly. Suppose many of those arrays are only referenced at driver
> initialization phase, or not referenced frequently. Not to use percpu
> variable here is just 
> fine unless you find some actual hot lines which then can be fixed
> accordingly. 
> 
> Thanks
> Kevin

Agree Keir and Kevin.
Static array during boot maybe simple and reliable.

> 
>> when/if a CPU is brought back online. Worse I suspect your patch as
>> it is will crash if some CPUs are offline during boot as you'll
>> deference their per_cpu area which doesn't actually exist unless a
>> CPU is online. You can test this for yourself by adding a maxcpus=1
>> boot parameter for Xen. 
>> 
>> The folding of the Intel/AMD structures might still be interesting,
>> and probably belongs as a separate patch anyway.
>> 
>> Cc'ing Intel and AMD guys to confirm this.
>> 
>>  -- Keir
>> 
>>> It would be nice, however, if the INTEL and/or AMD code owners
>>> could give an ack on this... 

It's fine to me to merge acpi_cpufreq_data and powernow_cpufreq_data.
In fact acpi_cpufreq_data can be commonly used by both Intel and AMD cpufreq 
logic. 
I didn't image the benefit of adding a similar struct powernow_cpufreq_data.

Thanks,
Jinsong

>>> 
>>> 
>>> Juergen
>> 
>> 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@xxxxxxxxxxxxxxxxxxx
>> http://lists.xensource.com/xen-devel


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel