Xen devel - Sep 2008 - c/s 18470

This changeset reverts two previous corrections, for reasons that escape
me.

First, the domain map is again being confined to NR_CPUS, which I had
submitted a patch to fix recently (yes, I realize the code has a TODO in
there, but those really get forgotten about far too often).

Second, the platform hypercall was reverted back to require all
information to be passed to Xen in one chunk, whereas I recall that even
Intel folks (not sure if it was you) agreed that allowing incremental
information collection was more appropriate.

Could you clarify why these changes were necessary and if/when you
plan to address the resulting issues?

Thanks, Jan


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

>>> "Jan Beulich" <jbeulich@novell.com> 17.09.08 09:43
>>>
>This changeset reverts two previous corrections, for reasons that escape
>me.
>
>First, the domain map is again being confined to NR_CPUS, which I had
>submitted a patch to fix recently (yes, I realize the code has a TODO in
>there, but those really get forgotten about far too often).
>
>Second, the platform hypercall was reverted back to require all
>information to be passed to Xen in one chunk, whereas I recall that even
>Intel folks (not sure if it was you) agreed that allowing incremental
>information collection was more appropriate.
>
>Could you clarify why these changes were necessary and if/when you
>plan to address the resulting issues?
Also, were these changes tested on AMD CPUs? It would seem to me
that the cpufreq_cpu_policy array would remain uninitialized here, and
hence the first access in the powernow code would dereference a NULL
pointer.

Likewise the calls to cpufreq_{add,del}_cpu() from the CPU hot(un)plug
paths seem to consider the Intel case only (as the functions themselves
are Intel specific).

Jan


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

Jan,

For the 1st issue, I notice your patch (c/s 18435), and think it''s good
not to limit dom by NR_CPUS. Your change has a precondition that we
already know the max dom number, then alloc dom map according to max dom
number, it''s good at our old cpufreq version since at that version,
hypervisor initialize cpufreq dom AFTER we get all px info from dom0 and
hence can get max dom number.
However, recently we update hypervisor cpufreq logic greatly, change
cpufreq init process by per cpu (old version is per dom), in this way we
don''t know max dom number and cannot use xmalloc_arry(), so we
temporarily use dom array limit by NR_CPUS, and mark it as TODO that
will update it in later future (i.e. by link list).

For the 2nd issue, our idea is to use flags to separate px init process
from runtime dynamic px handle (like ppc).

Thanks,
Jinsong

-----Original Message-----
From: Jan Beulich [mailto:jbeulich@novell.com] 
Sent: Wednesday, September 17, 2008 3:43 PM
To: Liu, Jinsong
Cc: xen-devel@lists.xensource.com
Subject: c/s 18470

This changeset reverts two previous corrections, for reasons that escape
me.

First, the domain map is again being confined to NR_CPUS, which I had
submitted a patch to fix recently (yes, I realize the code has a TODO in
there, but those really get forgotten about far too often).

Second, the platform hypercall was reverted back to require all
information to be passed to Xen in one chunk, whereas I recall that even
Intel folks (not sure if it was you) agreed that allowing incremental
information collection was more appropriate.

Could you clarify why these changes were necessary and if/when you
plan to address the resulting issues?

Thanks, Jan


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

Jan Beulich wrote:
>>>> "Jan Beulich" <jbeulich@novell.com> 17.09.08
09:43 >>>
>> This changeset reverts two previous corrections, for reasons that
>> escape me. 
>> 
>> First, the domain map is again being confined to NR_CPUS, which I had
>> submitted a patch to fix recently (yes, I realize the code has a
>> TODO in there, but those really get forgotten about far too often).
>> 
>> Second, the platform hypercall was reverted back to require all
>> information to be passed to Xen in one chunk, whereas I recall that
>> even Intel folks (not sure if it was you) agreed that allowing
>> incremental information collection was more appropriate.
>> 
>> Could you clarify why these changes were necessary and if/when you
>> plan to address the resulting issues?
> 
> Also, were these changes tested on AMD CPUs? It would seem to me
> that the cpufreq_cpu_policy array would remain uninitialized here, and
> hence the first access in the powernow code would dereference a NULL
> pointer.
[Jinsong]: 
  No, we didn''t test on AMD CPUs, we don''t have AMD platform.
AMD
powernow copy our cpufreq code, and share some data structure, this in
fact may result in bugs.
  Are you review/update powernow code recently? Recently, we rebase
cpufreq logic greatly, and add support to IPF arch, this work will
complete in 1 week.
  After cpufreq rebase and IPF support complete, 
  1. all arch-independent logic (like policy, governor algorithm, px
statistic, S3 suspend-resume, ppc dynamic handle, and most cpufreq init
logic, etc) will move to hypervisor common part (xen/drivers/cpufreq and
xen/drivers/acpi);
  2. all arch-dependent part (only cpufreq_driver) reside in arch
dependent dir (like xen/arch/x86/cpufreq (for x86 cpu) and
xen/arch/ia64/cpufreq (for ia64 cpu));

  So how about update AMD powernow cpufreq 1 week later? at that time,
all powernow current policy/governor/init logic can be cancelled, share
our common logic is OK, only need to leave powernow''s cpufreq_driver,
which is AMD arch-dependent.
> 
> Likewise the calls to cpufreq_{add,del}_cpu() from the CPU hot(un)plug
> paths seem to consider the Intel case only (as the functions
> themselves are Intel specific).
[Jinsong]:
  Not quite sure about your question. I just check the code:
  1. cpufreq_add/del_cpu() is arch-independent, since all arch-dependent
part has been handled by cpufreq_driver->init/exit().
  2. cpu online/offline path is also arch-independent.
  Would you please tell me more clear where is intel specific?

  Thanks,
  Jinsong
> 
> Jan

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

>>> "Liu, Jinsong" <jinsong.liu@intel.com> 18.09.08
09:39 >>>
>Jan Beulich wrote:
>> Likewise the calls to cpufreq_{add,del}_cpu() from the CPU hot(un)plug
>> paths seem to consider the Intel case only (as the functions
>> themselves are Intel specific).
>
>[Jinsong]:
>  Not quite sure about your question. I just check the code:
>  1. cpufreq_add/del_cpu() is arch-independent, since all arch-dependent
>part has been handled by cpufreq_driver->init/exit().
>  2. cpu online/offline path is also arch-independent.
>  Would you please tell me more clear where is intel specific?
In that platform_hypercall.c calls cpufreq_add_cpu() only for Intel CPUs,
but the hotplug code calls it and cpufreq_del_cpu() always. This ought
to be consistent I believe.

Jan


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

Jan Beulich wrote:
>>>> "Liu, Jinsong" <jinsong.liu@intel.com> 18.09.08
09:39 >>>
>> Jan Beulich wrote:
>>> Likewise the calls to cpufreq_{add,del}_cpu() from the CPU
>>> hot(un)plug paths seem to consider the Intel case only (as the
>>> functions themselves are Intel specific).
>> 
>> [Jinsong]:
>>  Not quite sure about your question. I just check the code:
>>  1. cpufreq_add/del_cpu() is arch-independent, since all
>> arch-dependent part has been handled by cpufreq_driver->init/exit().
>>  2. cpu online/offline path is also arch-independent.
>>  Would you please tell me more clear where is intel specific?
> 
> In that platform_hypercall.c calls cpufreq_add_cpu() only for Intel
> CPUs, but the hotplug code calls it and cpufreq_del_cpu() always.
> This ought to be consistent I believe.
Thanks for pointing out this point!
Yes, currently Intel and AMD differ at cpufreq init point, and they are
not consistent.
However, after we complete cpufreq rebase and IPF support, AMD powernow
can also stop using their init method, and using our common code
cpufreq_add_cpu() and cpufreq_del_cpu() since the code is
arch-independent. At that time, both Intel and AMD cpufreq init and
online/offline logic are consistent.

Thanks,
Jinsong
> 
> Jan

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