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
Reasonably Related Threads
- [PATCH] cpufreq: error path fixes
- [PATCH] cpufreq.c: shut up compiler about cpufreq_dom
- [PATCH] AMD, powernow: Update P-state directly when _PSD's CoordType is DOMAIN_COORD_TYPE_HW_ALL
- More Coverity-reported issues.
- [PATCH 0 of 2] [RFC] Patches to work with processor-passthru driver (v1).