Re: [PATCH 0/3] x86: structs for cpuid info in x86
* Nadav Amit nadav.a...@gmail.com wrote: On 9/16/14 4:22 PM, Ingo Molnar wrote: * Nadav Amit na...@cs.technion.ac.il wrote: The code that deals with x86 cpuid fields is hard to follow since it performs many bit operations and does not refer to cpuid field explicitly. To eliminate the need of openning a spec whenever dealing with cpuid fields, this patch-set introduces structs that reflect the various cpuid functions. Thanks for reviewing the patch-set. Nadav Amit (3): x86: Adding structs to reflect cpuid fields x86: Use new cpuid structs in cpuid functions KVM: x86: Using cpuid structs in KVM arch/x86/include/asm/cpuid_def.h | 163 +++ arch/x86/kernel/cpu/common.c | 56 -- arch/x86/kvm/cpuid.c | 36 + 3 files changed, 219 insertions(+), 36 deletions(-) create mode 100644 arch/x86/include/asm/cpuid_def.h I personally like bitfields in theory (they provide type clarity and abstract robustness, compared to open-coded bitmask numeric literals that are often used in cpuid using code, obfuscating cpuid usage), with the big caveat that for many years I didn't like bitfields in practice: older versions of GCC did a really poor job of optimizing them. So such a series would only be acceptable if it's demonstrated that both 'latest' and 'reasonably old' GCC versions do a good job in that department, compared to the old open-coded bitmask ops ... Comparing the 'size vmlinux' output of before/after kernels would probably be a good start in seeing the impact of such a change. If those results are positive then this technique could be propagated to all cpuid using code in arch/x86/, of which there's plenty. Thanks for the quick response. I was not aware GCC behaves this way. I made some small experiments with GCC-4.8 and GCC-4.4 and in brief my conclusions are: 1. The assembled code of bitmask and bitfields is indeed different. 2. GCC-4.8 and GCC-4.4 behave pretty much the same, yet GCC-4.8 appears to make better instructions reordering. 3. Loading/storing a single bitfield seems to be pretty much optimized (marginal advantage from code size point-of-view for bitmask, same number of instructions). 4. Loading/storing multiple bitfields seems to be somewhat under-optimized - multiple accesses to the original value result in ~30% more instructions and code-size. That's better than what I remembered. So you are correct - bitfields are less optimized. Nonetheless, since cpuid data is mostly used during startup, and otherwise a single bitfield is usually accessed in each function - I wonder whether it worth keeping the optimized but obfuscate code. Obviously, I can guess your answer to this question... So with the condition that you are actively watching out for performance critical code paths, I think the type clarity (i.e. bitfields) is a win. If hpa, tglx or Linus objects I'll yield to that objection though. Opinions, objections? Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] x86: structs for cpuid info in x86
On Wed, Sep 17, 2014 at 02:37:10PM +0200, Ingo Molnar wrote: Opinions, objections? Can I see those patches please? I can't find them on lkml or on the net - I only see this sub-thread... Thanks. -- Regards/Gruss, Boris. -- -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RESEND PATCH 0/3] x86: structs for cpuid info in x86
The code that deals with x86 cpuid fields is hard to follow since it performs many bit operations and does not refer to cpuid field explicitly. To eliminate the need of openning a spec whenever dealing with cpuid fields, this patch-set introduces structs that reflect the various cpuid functions. Thanks for reviewing the patch-set. Nadav Amit (3): x86: Adding structs to reflect cpuid fields x86: Use new cpuid structs in cpuid functions KVM: x86: Using cpuid structs in KVM arch/x86/include/asm/cpuid_def.h | 163 +++ arch/x86/kernel/cpu/common.c | 56 -- arch/x86/kvm/cpuid.c | 36 + 3 files changed, 219 insertions(+), 36 deletions(-) create mode 100644 arch/x86/include/asm/cpuid_def.h -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] x86: structs for cpuid info in x86
On Wed, Sep 17, 2014 at 02:37:10PM +0200, Ingo Molnar wrote: If hpa, tglx or Linus objects I'll yield to that objection though. Opinions, objections? They generally look fine to me. I appreciate the bitfields for readability. I often use the same when having to deal with hardware bitfields. See for example the cpuid10_a?x unions in asm/perf_event.h -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] x86: structs for cpuid info in x86
The code that deals with x86 cpuid fields is hard to follow since it performs many bit operations and does not refer to cpuid field explicitly. To eliminate the need of openning a spec whenever dealing with cpuid fields, this patch-set introduces structs that reflect the various cpuid functions. Thanks for reviewing the patch-set. Nadav Amit (3): x86: Adding structs to reflect cpuid fields x86: Use new cpuid structs in cpuid functions KVM: x86: Using cpuid structs in KVM arch/x86/include/asm/cpuid_def.h | 163 +++ arch/x86/kernel/cpu/common.c | 56 -- arch/x86/kvm/cpuid.c | 36 + 3 files changed, 219 insertions(+), 36 deletions(-) create mode 100644 arch/x86/include/asm/cpuid_def.h -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] x86: structs for cpuid info in x86
* Nadav Amit na...@cs.technion.ac.il wrote: The code that deals with x86 cpuid fields is hard to follow since it performs many bit operations and does not refer to cpuid field explicitly. To eliminate the need of openning a spec whenever dealing with cpuid fields, this patch-set introduces structs that reflect the various cpuid functions. Thanks for reviewing the patch-set. Nadav Amit (3): x86: Adding structs to reflect cpuid fields x86: Use new cpuid structs in cpuid functions KVM: x86: Using cpuid structs in KVM arch/x86/include/asm/cpuid_def.h | 163 +++ arch/x86/kernel/cpu/common.c | 56 -- arch/x86/kvm/cpuid.c | 36 + 3 files changed, 219 insertions(+), 36 deletions(-) create mode 100644 arch/x86/include/asm/cpuid_def.h I personally like bitfields in theory (they provide type clarity and abstract robustness, compared to open-coded bitmask numeric literals that are often used in cpuid using code, obfuscating cpuid usage), with the big caveat that for many years I didn't like bitfields in practice: older versions of GCC did a really poor job of optimizing them. So such a series would only be acceptable if it's demonstrated that both 'latest' and 'reasonably old' GCC versions do a good job in that department, compared to the old open-coded bitmask ops ... Comparing the 'size vmlinux' output of before/after kernels would probably be a good start in seeing the impact of such a change. If those results are positive then this technique could be propagated to all cpuid using code in arch/x86/, of which there's plenty. Thanks, Ingo -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/3] x86: structs for cpuid info in x86
On 9/16/14 4:22 PM, Ingo Molnar wrote: * Nadav Amit na...@cs.technion.ac.il wrote: The code that deals with x86 cpuid fields is hard to follow since it performs many bit operations and does not refer to cpuid field explicitly. To eliminate the need of openning a spec whenever dealing with cpuid fields, this patch-set introduces structs that reflect the various cpuid functions. Thanks for reviewing the patch-set. Nadav Amit (3): x86: Adding structs to reflect cpuid fields x86: Use new cpuid structs in cpuid functions KVM: x86: Using cpuid structs in KVM arch/x86/include/asm/cpuid_def.h | 163 +++ arch/x86/kernel/cpu/common.c | 56 -- arch/x86/kvm/cpuid.c | 36 + 3 files changed, 219 insertions(+), 36 deletions(-) create mode 100644 arch/x86/include/asm/cpuid_def.h I personally like bitfields in theory (they provide type clarity and abstract robustness, compared to open-coded bitmask numeric literals that are often used in cpuid using code, obfuscating cpuid usage), with the big caveat that for many years I didn't like bitfields in practice: older versions of GCC did a really poor job of optimizing them. So such a series would only be acceptable if it's demonstrated that both 'latest' and 'reasonably old' GCC versions do a good job in that department, compared to the old open-coded bitmask ops ... Comparing the 'size vmlinux' output of before/after kernels would probably be a good start in seeing the impact of such a change. If those results are positive then this technique could be propagated to all cpuid using code in arch/x86/, of which there's plenty. Thanks for the quick response. I was not aware GCC behaves this way. I made some small experiments with GCC-4.8 and GCC-4.4 and in brief my conclusions are: 1. The assembled code of bitmask and bitfields is indeed different. 2. GCC-4.8 and GCC-4.4 behave pretty much the same, yet GCC-4.8 appears to make better instructions reordering. 3. Loading/storing a single bitfield seems to be pretty much optimized (marginal advantage from code size point-of-view for bitmask, same number of instructions). 4. Loading/storing multiple bitfields seems to be somewhat under-optimized - multiple accesses to the original value result in ~30% more instructions and code-size. So you are correct - bitfields are less optimized. Nonetheless, since cpuid data is mostly used during startup, and otherwise a single bitfield is usually accessed in each function - I wonder whether it worth keeping the optimized but obfuscate code. Obviously, I can guess your answer to this question... Nadav -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
