x86 oprofile: use rdmsrl/wrmsrl
PATCH 1/1 x86 oprofile: use rdmsrl/wrmsrl
Hi!
Attached patch makes x86 oprofile code use rdmsrl/wrmsrl
Signature · 6 lines
---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632
xen/arch/x86/oprofile/op_model_athlon.c +36 -38
@@ -26,23 +26,22 @@#define NUM_COUNTERS 4#define NUM_CONTROLS 4 -#define CTR_READ (l,h,msrs,c) do {rdmsr(msrs->counters[(c)].addr, (l), (h));} while (0) +#define CTR_READ (msr_content,msrs,c) do {rdmsrl(msrs->counters[(c)].addr, (msr_content));} while (0)#define CTR_WRITE (l,msrs,c) do {wrmsr(msrs->counters[(c)].addr, -(unsigned int)(l), -1);} while (0) -#define CTR_OVERFLOWED (n) (!((n) & (1U<<31))) +#define CTR_OVERFLOWED (n) (!((n) & (1ULL<<31))) -#define CTRL_READ (l,h,msrs,c) do {rdmsr(msrs->controls[(c)].addr, (l), (h));} while (0) -#define CTRL_WRITE (l,h,msrs,c) do {wrmsr(msrs->controls[(c)].addr, (l), (h));} while (0) -#define CTRL_SET_ACTIVE (n) (n |= (1<<22)) -#define CTRL_SET_INACTIVE (n) (n &= ~(1<<22)) -#define CTRL_CLEAR (lo, hi) (lo &= (1<<21), hi = 0) -#define CTRL_SET_ENABLE (val) (val |= 1<<20) +#define CTRL_READ (msr_content,msrs,c) do {rdmsrl(msrs->controls[(c)].addr, (msr_content));} while (0) +#define CTRL_WRITE (msr_content,msrs,c) do {wrmsrl(msrs->controls[(c)].addr, (msr_content));} while (0) +#define CTRL_SET_ACTIVE (n) (n |= (1ULL<<22)) +#define CTRL_SET_INACTIVE (n) (n &= ~(1ULL<<22)) +#define CTRL_CLEAR (val) (val &= (1ULL<<21)) +#define CTRL_SET_ENABLE (val) (val |= 1ULL<<20)#define CTRL_SET_USR (val,u) (val |= ((u & 1) << 16))#define CTRL_SET_KERN (val,k) (val |= ((k & 1) << 17))#define CTRL_SET_UM (val, m) (val |= ((m & 0xff) << 8)) -#define CTRL_SET_EVENT_LOW (val, e) (val |= (e & 0xff)) -#define CTRL_SET_EVENT_HIGH (val, e) (val |= ((e >> 8) & 0xf)) -#define CTRL_SET_HOST_ONLY (val, h) (val |= ((h & 1) << 9)) -#define CTRL_SET_GUEST_ONLY (val, h) (val |= ((h & 1) << 8)) +#define CTRL_SET_EVENT (val, e) (val |= (((e >> 8) & 0xf) | (e & 0xff))) +#define CTRL_SET_HOST_ONLY (val, h) (val |= ((h & 0x1ULL) << 41)) +#define CTRL_SET_GUEST_ONLY (val, h) (val |= ((h & 0x1ULL) << 40))static unsigned long reset_value[NUM_COUNTERS ];
@@ -64,14 +63,14 @@ static void athlon_fill_in_addresses(strstatic void athlon_setup_ctrs (struct op_msrs const *const msrs) { -unsigned int low, high; +uint64_t msr_content;int i;/* clear all counters */ for (i =0 ; i <NUM_CONTROLS ; ++i) { -CTRL_READ (low, high, msrs, i); -CTRL_CLEAR (low, high); -CTRL_WRITE (low, high, msrs, i); +CTRL_READ (msr_content, msrs, i); +CTRL_CLEAR (msr_content); +CTRL_WRITE (msr_content, msrs, i); }/* avoid a false detection of ctr overflows in NMI handler */
@@ -86,17 +85,16 @@ static void athlon_setup_ctrs(struct op_CTR_WRITE (counter_config[i].count, msrs, i); -CTRL_READ (low, high, msrs, i); -CTRL_CLEAR (low, high); -CTRL_SET_ENABLE (low); -CTRL_SET_USR (low, counter_config[i].user); -CTRL_SET_KERN (low, counter_config[i].kernel); -CTRL_SET_UM (low, counter_config[i].unit_mask); -CTRL_SET_EVENT_LOW (low, counter_config[i].event); -CTRL_SET_EVENT_HIGH (high, counter_config[i].event); -CTRL_SET_HOST_ONLY (high,0 ); -CTRL_SET_GUEST_ONLY (high,0 ); -CTRL_WRITE (low, high, msrs, i); +CTRL_READ (msr_content, msrs, i); +CTRL_CLEAR (msr_content); +CTRL_SET_ENABLE (msr_content); +CTRL_SET_USR (msr_content, counter_config[i].user); +CTRL_SET_KERN (msr_content, counter_config[i].kernel); +CTRL_SET_UM (msr_content, counter_config[i].unit_mask); +CTRL_SET_EVENT (msr_content, counter_config[i].event); +CTRL_SET_HOST_ONLY (msr_content,0 ); +CTRL_SET_GUEST_ONLY (msr_content,0 ); +CTRL_WRITE (msr_content, msrs, i); }else { reset_value[i] =0 ; }
@@ -108,7 +106,7 @@ static int athlon_check_ctrs(unsigned in struct cpu_user_regs * const regs) { -unsigned int low, high; +uint64_t msr_content;int i;int ovf =0 ;unsigned long eip = regs->eip;
@@ -128,8 +126,8 @@ static int athlon_check_ctrs(unsigned in }for (i =0 ; i <NUM_COUNTERS ; ++i) { -CTR_READ (low, high, msrs, i); -if (CTR_OVERFLOWED (low)) { +CTR_READ (msr_content, msrs, i); +if (CTR_OVERFLOWED (msr_content)) {xenoprof_log_event (current, regs, eip, mode, i);CTR_WRITE (reset_value[i], msrs, i); ovf =1 ;
@@ -143,13 +141,13 @@ static int athlon_check_ctrs(unsigned instatic void athlon_start (struct op_msrs const *const msrs) { -unsigned int low, high; +uint64_t msr_content;int i;for (i =0 ; i <NUM_COUNTERS ; ++i) {if (reset_value[i]) { -CTRL_READ (low, high, msrs, i); -CTRL_SET_ACTIVE (low); -CTRL_WRITE (low, high, msrs, i); +CTRL_READ (msr_content, msrs, i); +CTRL_SET_ACTIVE (msr_content); +CTRL_WRITE (msr_content, msrs, i); } } }
@@ -157,15 +155,15 @@ static void athlon_start(struct op_msrsstatic void athlon_stop (struct op_msrs const *const msrs) { -unsigned int low,high; +uint64_t msr_content;int i;/* Subtle: stop on all counters to avoid race with * setting our pm callback */ for (i =0 ; i <NUM_COUNTERS ; ++i) { -CTRL_READ (low, high, msrs, i); -CTRL_SET_INACTIVE (low); -CTRL_WRITE (low, high, msrs, i); +CTRL_READ (msr_content, msrs, i); +CTRL_SET_INACTIVE (msr_content); +CTRL_WRITE (msr_content, msrs, i); } }
xen/arch/x86/oprofile/op_model_p4.c +68 -66
255 lines not on this page · raw message
xen/arch/x86/oprofile/op_model_ppro.c +33 -33
124 lines not on this page · raw message
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
How many more of these patches do you have up your sleeve?
-- Keir
Quoted 6 lines · Christoph Egger
Hi!
Attached patch makes x86 oprofile code use rdmsrl/wrmsrl
Signed-off-by Christoph Egger <[email protected]>
Quoted 1 line · On Tuesday 29 June 2010 18:02:54 Keir Fraser wrote:
How many more of these patches do you have up your sleeve?
Depends on how small/large the chunks should/may be. My intention is to remove duplicated rdmsr/wrmsr functions and make rdmsr usable as a real C function.
The large part of changes are the bit-operations surrounding the rdmsr/wrmsr code lines.
The sent patches replace rdmsr/wrmsr by rdmsrl/wrmsrl step by step. When this is done, rdmsr/wrmsr can be killed.
Christoph
Quoted 8 lines
-- Keir
Quoted 5 lines · Christoph Egger
Hi!
Attached patch makes x86 oprofile code use rdmsrl/wrmsrl
Signed-off-by Christoph Egger <[email protected]>
Signature · 6 lines
---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632
Quoted 6 lines · Christoph Egger
Quoted 1 line · On Tuesday 29 June 2010 18:02:54 Keir Fraser wrote:
How many more of these patches do you have up your sleeve?
Depends on how small/large the chunks should/may be. My intention is to remove duplicated rdmsr/wrmsr functions and make rdmsr usable as a real C function.
It's a lot of effort for questionable value isn't it. And many of your patches so far have contained bugs, and in some cases decreased code readability by needing to manually merge/split values to be able to make use of rdmsrl/wrmsrl, so it's not like it's even strictly improving the code. Frankly, it smells like busy work.
-- Keir
Quoted 8 lines · On Wednesday 30 June 2010 12:19:09 Keir Fraser wrote:
Quoted 6 lines · Christoph Egger
Quoted 1 line · On Tuesday 29 June 2010 18:02:54 Keir Fraser wrote:
How many more of these patches do you have up your sleeve?
Depends on how small/large the chunks should/may be. My intention is to remove duplicated rdmsr/wrmsr functions and make rdmsr usable as a real C function.
Quoted 5 lines
It's a lot of effort for questionable value isn't it. And many of your patches so far have contained bugs, and in some cases decreased code readability by needing to manually merge/split values to be able to make use of rdmsrl/wrmsrl, so it's not like it's even strictly improving the code. Frankly, it smells like busy work.
Yes, you're right. Work like this is not funny and error-prone (you can look over it many times, test it many times and still bugs sit in there), hence the high number of small patches so that other people look at it and make bisecting easier.
Whoever is doing such work always risks to get the head chopped off for the breakages.
The parts decreasing code readability should actually be rewritten but this is beyond of my intention.
Christoph
Signature · 6 lines
---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632
Quoted 2 lines · Christoph Egger
The parts decreasing code readability should actually be rewritten but this is beyond of my intention.
That's interesting. What is your intention then?
Joerg
Quoted 5 lines · On Wednesday 30 June 2010 16:29:44 Joerg Roedel wrote:
Quoted 2 lines · Christoph Egger
The parts decreasing code readability should actually be rewritten but this is beyond of my intention.
That's interesting. What is your intention then?
As explained in an earlier mail, I want to get rid of the redundant rdmsr/wrmsr functions.
Christoph
Signature · 6 lines
---to satisfy European Law for business letters: Advanced Micro Devices GmbH Einsteinring 24, 85609 Dornach b. Muenchen Geschaeftsfuehrer: Alberto Bozzo, Andrew Bowd Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen Registergericht Muenchen, HRB Nr. 43632
Quoted 2 lines · Christoph Egger
As explained in an earlier mail, I want to get rid of the redundant rdmsr/wrmsr functions.
What's the point of that if you decrease the code readability?
Joerg