AMD Family10h+ IBS support for oProfile driver: Setup routines
PATCH 1/3 AMD Family10h+ IBS support for oProfile driver: Setup routines
This patchset supports the new profiling hardware available in the latest AMD CPUs in the oProfile driver.
These new AMD processors CPUs support Instruction Based Sampling (IBS). See
Instruction-Based Sampling: A New Performance Analysis Technique for AMD Family 10h Processors, November 19, 2007
http://developer.amd.com/assets/AMD_IBS_paper_EN.pdf
for more information about IBS.
IBS support requires changes to the oProfile driver to gather this information and initialize the new MSRs associated with these new features.
This patch adds 2 new types of Profiling samples IBS_FETCH and IBS_OP to the per CPU buffers and the event buffers of the oProfile driver.
It also adds new control entries to /dev/oprofile to control IBS sampling.
These changes are backward compatible with the previous PMC only version of the driver, and a separate patch is available to oProfile 0.9.3 to use this new data.
These changes have been extensively tested at AMD on Family10h systems.
Barry Kasindorf [email protected]
--- nmi_int.c | 49 +++++++++++++++++++++++++++++++++++++++++++++---- op_counter.h | 16 +++++++++++++--- 2 files changed, 58 insertions(+), 7 deletions(-)
arch/x86/oprofile/nmi_int.c +45 -4
@@ -218,6 +218,11 @@ static int nmi_setup(void) } } + +/* setup AMD Family10h+ IBS irq if needed */ +if (IBS_avail ()) +setup_ibs_nmi (); +on_each_cpu (nmi_save_registers,NULL ,0 ,1 );on_each_cpu (nmi_cpu_setup,NULL ,0 ,1 ); nmi_enabled =1 ;
@@ -275,6 +280,10 @@ static void nmi_shutdown(void)unregister_die_notifier (&profile_exceptions_nb); model->shutdown (msrs);free_msrs (); + +/* clear AMD Family10h+ IBS irq if needed */ +if (IBS_avail ()) +clear_ibs_nmi (); }static void nmi_cpu_start (void *dummy)
@@ -301,15 +310,14 @@ static void nmi_stop(void) }struct op_counter_config counter_config[OP_MAX_COUNTER ]; +struct op_ibs_config ibs_config;static int nmi_create_files (struct super_block *sb,struct dentry *root) {unsigned int i; - +struct dentry *dir;for (i =0 ; i < model->num_counters; ++i) { -struct dentry *dir;char buf[4 ]; -/* quick little hack to _not_ expose a counter if it is not * available for use. This should protect userspace app. * NOTE: assumes 1:1 mapping here (that counters are organized
@@ -328,6 +336,33 @@ static int nmi_create_files(struct super_block *sb, struct dentry *root)oprofilefs_create_ulong (sb, dir,"user" , &counter_config[i].user); } +/* Setup AMD Family10h+ IBS control if needed */ +if (IBS_avail ()) { +char buf[12 ]; + +/* setup some reasonable defaults */ + ibs_config.max_cnt_fetch =250000 ; + ibs_config.FETCH_enabled =0 ; + ibs_config.max_cnt_op =250000 ; + ibs_config.OP_enabled =0 ; + ibs_config.dispatched_ops =1 ; +snprintf (buf,sizeof (buf),"ibs_fetch" ); + dir =oprofilefs_mkdir (sb, root, buf); +oprofilefs_create_ulong (sb, dir,"ran_enable" , + &ibs_config.rand_en); +oprofilefs_create_ulong (sb, dir,"enable" , + &ibs_config.FETCH_enabled); +oprofilefs_create_ulong (sb, dir,"max_count" , + &ibs_config.max_cnt_fetch); +snprintf (buf,sizeof (buf),"ibs_uops" ); + dir =oprofilefs_mkdir (sb, root, buf); +oprofilefs_create_ulong (sb, dir,"enable" , + &ibs_config.OP_enabled); +oprofilefs_create_ulong (sb, dir,"max_count" , + &ibs_config.max_cnt_op); +oprofilefs_create_ulong (sb, dir,"dispatched_ops" , + &ibs_config.dispatched_ops); + }return 0 ; }
@@ -419,9 +454,15 @@ int __init op_nmi_init(struct oprofile_operations *ops)break ;case 0x10 : model = &op_athlon_spec; - cpu_type ="x86-64/family10" ; + cpu_type ="x86-64/family10h" ; +break ; +case 0x11 : + model = &op_athlon_spec; + cpu_type ="x86-64/family11h" ;break ; } +/* set global if IBS profiling is available */ +check_IBS_avail (family);break ;case X86_VENDOR_INTEL :
arch/x86/oprofile/op_counter.h +13 -3
@@ -6,12 +6,12 @@* * @author John Levon */ - +#ifndef OP_COUNTER_H #define OP_COUNTER_H - +#define OP_MAX_COUNTER 8 - +/* Per-perfctr configuration as set via * oprofilefs. */
@@ -26,4 +26,14 @@ struct op_counter_config {extern struct op_counter_config counter_config[]; +struct op_ibs_config { +unsigned long OP_enabled; +unsigned long FETCH_enabled; +unsigned long max_cnt_fetch; +unsigned long max_cnt_op; +unsigned long rand_en; +unsigned long dispatched_ops; +}; + +extern struct op_ibs_config ibs_config;#endif /* OP_COUNTER_H */
PATCH 2/3 AMD Family10h+ IBS support for oProfile driver: Interrupt routines
--- arch/x86/kernel/apic_32.c | 24 +++ arch/x86/kernel/apic_64.c | 1 arch/x86/oprofile/op_model_athlon.c | 265 +++++++++++++++++++++++++++++++++++- arch/x86/oprofile/op_x86_model.h | 42 +++++ include/asm-x86/apicdef.h | 3 5 files changed, 334 insertions(+), 1 deletion(-)
arch/x86/kernel/apic_32.c +24 -0
@@ -237,6 +237,30 @@ static void __setup_APIC_LVTT(unsigned int clocks, int oneshot, int irqen)if (!oneshot)apic_write_around (APIC_TMICT , clocks/APIC_DIVISOR ); } +#define APIC_EILVT_LVTOFF_MCE 0 +#define APIC_EILVT_LVTOFF_IBS 1 + +static void setup_APIC_eilvt (u8 lvt_off,u8 vector,u8 msg_type,u8 mask) +{ +unsigned long reg = (lvt_off <<4 ) +APIC_EILVT0 ; +unsigned int v = (mask <<16 ) | (msg_type <<8 ) | vector; + +apic_write (reg, v); +} + +u8 setup_APIC_eilvt_mce (u8 vector,u8 msg_type,u8 mask) +{ +setup_APIC_eilvt (APIC_EILVT_LVTOFF_MCE , vector, msg_type, mask); +return APIC_EILVT_LVTOFF_MCE ; +} + +u8 setup_APIC_eilvt_ibs (u8 vector,u8 msg_type,u8 mask) +{ +setup_APIC_eilvt (APIC_EILVT_LVTOFF_IBS , vector, msg_type, mask); +return APIC_EILVT_LVTOFF_IBS ; +} +EXPORT_SYMBOL (setup_APIC_eilvt_ibs); +/* * Program the next event, relative to now
arch/x86/kernel/apic_64.c +1 -0
@@ -229,6 +229,7 @@ u8 setup_APIC_eilvt_ibs(u8 vector, u8 msg_type, u8 mask)setup_APIC_eilvt (APIC_EILVT_LVTOFF_IBS , vector, msg_type, mask);return APIC_EILVT_LVTOFF_IBS ; } +EXPORT_SYMBOL (setup_APIC_eilvt_ibs);/* * Program the next event, relative to now
include/asm-x86/apicdef.h +3 -0
@@ -123,6 +123,9 @@#define APIC_EILVT_MSG_NMI 0x4#define APIC_EILVT_MSG_EXT 0x7#define APIC_EILVT_MASKED (1 << 16) +#define APIC_EILVT_MASK_INT 1 +#define APIC_EILVT_ENA_INT 0 +#define APIC_EILVT1 0x510#define APIC_EILVT2 0x520#define APIC_EILVT3 0x530
arch/x86/oprofile/op_model_athlon.c +264 -1
310 lines not on this page · raw message
arch/x86/oprofile/op_x86_model.h +42 -0
52 lines not on this page · raw message
PATCH 3/3 AMD Family10h+ IBS support for oProfile driver: buffer management
In reply to Barry Kasindorf--- drivers/oprofile/buffer_sync.c | 140 ++++++++++++++++++++++++++++++----------- drivers/oprofile/cpu_buffer.c | 102 +++++++++++++++++++++++++++-- drivers/oprofile/cpu_buffer.h | 3 include/linux/oprofile.h | 13 +++ 4 files changed, 213 insertions(+), 45 deletions(-)
drivers/oprofile/buffer_sync.c +103 -37
@@ -5,6 +5,7 @@* @remark Read the file COPYING * * @author John Levon <[email protected]> +* @author Barry Kasindorf * * This is the core of the buffer management. Each * CPU buffer is processed and entered into the
@@ -33,7 +34,7 @@#include "event_buffer.h" #include "cpu_buffer.h" #include "buffer_sync.h" - +static LIST_HEAD (dying_tasks);static LIST_HEAD (dead_tasks);static cpumask_t marked_cpus =CPU_MASK_NONE ;
@@ -99,7 +100,7 @@ static int munmap_notify(struct notifier_block * self, unsigned long val, void *return 0 ; } - +/* We need to be told about new modules so we don't attribute to a previously * loaded module, or drop the samples on the floor. */
@@ -118,7 +119,7 @@ static int module_load_notify(struct notifier_block * self, unsigned long val, vreturn 0 ; } - +static struct notifier_block task_free_nb = { .notifier_call = task_free_notify, };
@@ -135,7 +136,7 @@ static struct notifier_block module_load_nb = { .notifier_call = module_load_notify, }; - +static void end_sync (void ) {end_cpu_work ();
@@ -212,10 +213,10 @@ static unsigned long get_exec_dcookie(struct mm_struct * mm) {unsigned long cookie =NO_COOKIE ;struct vm_area_struct * vma; - +if (!mm) goto out; - +for (vma = mm->mmap; vma; vma = vma->vm_next) {if (!vma->vm_file)continue ;
@@ -241,7 +242,7 @@ static unsigned long lookup_dcookie(struct mm_struct * mm, unsigned long addr, ostruct vm_area_struct * vma;for (vma =find_vma (mm, addr); vma; vma = vma->vm_next) { - +if (addr < vma->vm_start || addr >= vma->vm_end)continue ;
@@ -263,9 +264,19 @@ static unsigned long lookup_dcookie(struct mm_struct * mm, unsigned long addr, oreturn cookie; } +static void increment_tail (struct oprofile_cpu_buffer *b) +{ +unsigned long new_tail = b->tail_pos +1 ; + +rmb ();/* be sure fifo pointers are synchromized */ +if (new_tail < b->buffer_size) + b->tail_pos = new_tail; +else + b->tail_pos =0 ; +}static unsigned long last_cookie =INVALID_COOKIE ; - +static void add_cpu_switch (int i) {add_event_entry (ESCAPE_CODE );
@@ -278,16 +289,16 @@ static void add_kernel_ctx_switch(unsigned int in_kernel) {add_event_entry (ESCAPE_CODE );if (in_kernel) -add_event_entry (KERNEL_ENTER_SWITCH_CODE ); +add_event_entry (KERNEL_ENTER_SWITCH_CODE );else -add_event_entry (KERNEL_EXIT_SWITCH_CODE ); +add_event_entry (KERNEL_EXIT_SWITCH_CODE ); } - +static void add_user_ctx_switch (struct task_struct const * task,unsigned long cookie) {add_event_entry (ESCAPE_CODE ); -add_event_entry (CTX_SWITCH_CODE ); +add_event_entry (CTX_SWITCH_CODE );add_event_entry (task->pid);add_event_entry (cookie);/* Another code for daemon back-compat */
@@ -296,7 +307,7 @@ add_user_ctx_switch(struct task_struct const * task, unsigned long cookie)add_event_entry (task->tgid); } - +static void add_cookie_switch (unsigned long cookie) {add_event_entry (ESCAPE_CODE );
@@ -304,13 +315,74 @@ static void add_cookie_switch(unsigned long cookie)add_event_entry (cookie); } - +static void add_trace_begin (void ) {add_event_entry (ESCAPE_CODE );add_event_entry (TRACE_BEGIN_CODE ); } +/* +* Add IBS fetch and op entries to event buffer +*/ +static void add_ibs_begin (struct oprofile_cpu_buffer *cpu_buf,int code, +int in_kernel,struct mm_struct *mm) +{ +unsigned long rip; +int i, count; +unsigned long ibs_cookie =0 ; +off_t offset; + +increment_tail (cpu_buf);/* move to RIP entry */ + + rip = ((struct op_sample *)&cpu_buf->buffer[cpu_buf->tail_pos])->eip; + +#ifdef __LP64__ + rip += + ((struct op_sample *)&cpu_buf->buffer[cpu_buf->tail_pos])->event <<32 ; +#endif + +if (mm) { + ibs_cookie =lookup_dcookie (mm, rip, &offset); + +if (ibs_cookie ==NO_COOKIE ) + offset = rip; +if (ibs_cookie ==INVALID_COOKIE ) { +atomic_inc (&oprofile_stats.sample_lost_no_mapping); + offset = rip; + } +if (ibs_cookie != last_cookie) { +add_cookie_switch (ibs_cookie); + last_cookie = ibs_cookie; + } + } + +else + offset = rip; + +add_event_entry (ESCAPE_CODE ); +add_event_entry (code); +add_event_entry (offset);/* Offset from Dcookie */ + +/* we send the Dcookie offset, but send the raw Linear Add also*/ +add_event_entry ( + ((struct op_sample *)&cpu_buf->buffer[cpu_buf->tail_pos])->eip); +add_event_entry ( + ((struct op_sample *)&cpu_buf->buffer[cpu_buf->tail_pos])->event); + +if (code ==IBS_FETCH_CODE ) + count =2 ; +else + count =5 ;/*IBS OP is 5 int64s long */ + +for (i =0 ; i < count; i++) { +increment_tail (cpu_buf); +add_event_entry ( + ((struct op_sample *)&cpu_buf->buffer[cpu_buf->tail_pos])->eip); +add_event_entry ( + ((struct op_sample *)&cpu_buf->buffer[cpu_buf->tail_pos])->event); + } +}static void add_sample_entry (unsigned long offset,unsigned long event) {
@@ -323,9 +395,9 @@ static int add_us_sample(struct mm_struct * mm, struct op_sample * s) {unsigned long cookie;off_t offset; - + cookie =lookup_dcookie (mm, s->eip, &offset); - +if (cookie ==INVALID_COOKIE ) {atomic_inc (&oprofile_stats.sample_lost_no_mapping);return 0 ;
@@ -341,7 +413,7 @@ static int add_us_sample(struct mm_struct * mm, struct op_sample * s)return 1 ; } - +/* Add a sample to the global event buffer. If possible the * sample is converted into a persistent dentry/offset pair * for later lookup from userspace.
@@ -359,7 +431,7 @@ add_sample(struct mm_struct * mm, struct op_sample * s, int in_kernel) }return 0 ; } - +static void release_mm (struct mm_struct * mm) {
@@ -383,7 +455,7 @@ static inline int is_code(unsigned long val) {return val ==ESCAPE_CODE ; } - +/* "acquire" as many cpu buffer slots as we can */ static unsigned long get_slots (struct oprofile_cpu_buffer * b)
@@ -412,19 +484,6 @@ static unsigned long get_slots(struct oprofile_cpu_buffer * b) } -static void increment_tail (struct oprofile_cpu_buffer * b) -{ -unsigned long new_tail = b->tail_pos +1 ; - -rmb (); - -if (new_tail < b->buffer_size) - b->tail_pos = new_tail; -else - b->tail_pos =0 ; -} - -/* Move tasks along towards death. Any tasks on dead_tasks * will definitely have no remaining references in any * CPU buffers at this point, because we use two lists,
@@ -496,21 +555,20 @@ void sync_buffer(int cpu)struct task_struct * new;unsigned long cookie =0 ;int in_kernel =1 ; -unsigned int i;sync_buffer_state state = sb_buffer_start;unsigned long available;mutex_lock (&buffer_mutex); - +add_cpu_switch (cpu);/* Remember, only we can modify tail_pos */ available =get_slots (cpu_buf); -for (i =0 ; i < available; ++i) { -struct op_sample * s = &cpu_buf->buffer[cpu_buf->tail_pos]; - +while (get_slots (cpu_buf)) { +struct op_sample *s = &cpu_buf->buffer[cpu_buf->tail_pos]; +if (is_code (s->eip)) {if (s->event <=CPU_IS_KERNEL ) {/* kernel/userspace switch */
@@ -521,6 +579,14 @@ void sync_buffer(int cpu) }else if (s->event ==CPU_TRACE_BEGIN ) { state = sb_bt_start;add_trace_begin (); + }else if (s->event ==IBS_FETCH_BEGIN ) { + state = sb_bt_start; +add_ibs_begin (cpu_buf, +IBS_FETCH_CODE , in_kernel, mm); + }else if (s->event ==IBS_OP_BEGIN ) { + state = sb_bt_start; +add_ibs_begin (cpu_buf, +IBS_OP_CODE , in_kernel, mm); }else {struct mm_struct * oldmm = mm;
drivers/oprofile/cpu_buffer.c +94 -8
149 lines not on this page · raw message
drivers/oprofile/cpu_buffer.h +3 -0
@@ -53,7 +53,10 @@ DECLARE_PER_CPU(struct oprofile_cpu_buffer, cpu_buffer);void cpu_buffer_reset (struct oprofile_cpu_buffer * cpu_buf);/* transient events for the CPU buffer -> event buffer */ +#define CPU_IS_USER 0#define CPU_IS_KERNEL 1#define CPU_TRACE_BEGIN 2 +#define IBS_FETCH_BEGIN 3 +#define IBS_OP_BEGIN 4#endif /* OPROFILE_CPU_BUFFER_H */
include/linux/oprofile.h +13 -0
@@ -36,6 +36,9 @@#define XEN_ENTER_SWITCH_CODE 10#define SPU_PROFILING_CODE 11#define SPU_CTX_SWITCH_CODE 12 +#define IBS_FETCH_CODE 13 +#define IBS_OP_CODE 14 +struct super_block ;struct dentry ;
@@ -106,6 +109,16 @@ void oprofile_add_sample(struct pt_regs * const regs, unsigned long event);void oprofile_add_ext_sample (unsigned long pc,struct pt_regs *const regs,unsigned long event,int is_kernel); +/** +* Add an AMD IBS sample. This may be called from any context. Pass +* smp_processor_id() as cpu. PAsses IBS registers as a unsigned int[8] +*/ +void oprofile_add_ibs_op_sample (struct pt_regs *const regs, +unsigned int *const ibs_op); + +void oprofile_add_ibs_fetch_sample (struct pt_regs *const regs, +unsigned int *const ibs_fetch); +/* Use this instead when the PC value is not from the regs. Doesn't * backtrace. */ void oprofile_add_pc (unsigned long pc,int is_kernel,unsigned long event);
Re: [PATCH 3/3] AMD Family10h+ IBS support for oProfile driver: buffer management
Quoted 2 lines · Barry Kasindorf
Signed-off-by Barry Kasindorf <[email protected]>
missing changelog, lots of strange whitespace changes.
drivers/oprofile/buffer_sync.c line 40
Quoted 9 lines
drivers/oprofile/buffer_sync.c +1
-1@@ -33,7 +34,7 @@#include "event_buffer.h" #include "cpu_buffer.h" #include "buffer_sync.h" - +static LIST_HEAD (dying_tasks);static LIST_HEAD (dead_tasks);static cpumask_t marked_cpus =CPU_MASK_NONE ;
I actually do not see the difference here. What is going on?
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
RE: [PATCH 3/3] AMD Family10h+ IBS support for oProfile driver: buffer management
Hi,
I can add a changelog, but this is all new and the comments at the head of the first patch detail the nature of the changes.
I don't know why all those white space changes happened, I did a GIT diff and it found them. I suspect that there is some trailing whitespace that got removed, I will check that.
-Barry
-----Original Message-----
From: Pavel Machek [mailto:[email protected]]
Sent: Wednesday, June 11, 2008 6:54 AM
To: Kasindorf, Barry
Cc: [email protected]
Subject: Re: [PATCH 3/3] AMD Family10h+ IBS support for oProfile driver:
buffer management
Quoted 2 lines · Barry Kasindorf
Signed-off-by Barry Kasindorf <[email protected]>
missing changelog, lots of strange whitespace changes.
drivers/oprofile/buffer_sync.c line 40
Quoted 9 lines
drivers/oprofile/buffer_sync.c +1
-1@@ -33,7 +34,7 @@#include "event_buffer.h" #include "cpu_buffer.h" #include "buffer_sync.h" - +static LIST_HEAD (dying_tasks);static LIST_HEAD (dead_tasks);static cpumask_t marked_cpus =CPU_MASK_NONE ;
I actually do not see the difference here. What is going on?
Signature · 3 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
arch/x86/oprofile/nmi_int.c int __init op_nmi_init(struct oprofile_operations *ops) · line 458
Quoted 9 lines
This patchset supports the new profiling hardware available in the latest AMD CPUs in the oProfile driver.
arch/x86/oprofile/nmi_int.c +2
-1@@ -419,9 +454,15 @@ int __init op_nmi_init(struct oprofile_operations *ops)break ;case 0x10 : model = &op_athlon_spec; - cpu_type ="x86-64/family10" ; + cpu_type ="x86-64/family10h" ; +break ;
Is this string visible to userspace?
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 4 lines · Pavel Machek
Hi!
Quoted 1 line
This patchset supports the new profiling hardware available in the latest AMD CPUs in the oProfile driver.
Quoted 9 lines
arch/x86/oprofile/nmi_int.c int __init op_nmi_init(struct oprofile_operations *ops) · line 458
Quoted 7 lines
arch/x86/oprofile/nmi_int.c +2
-1@@ -419,9 +454,15 @@ int __init op_nmi_init(struct oprofile_operations *ops)break ;case 0x10 : model = &op_athlon_spec; - cpu_type ="x86-64/family10" ; + cpu_type ="x86-64/family10h" ; +break ;Is this string visible to userspace?
Yes it is.
-Andi
Yes,
This is visible to userspace. The corresponding changes to the usermode code have been sent to the oProfile list. The confusion is that it is not AMD Family 10 but it is Family 16(10h). This can be confusing so we changed it.
-Barry
-----Original Message-----
From: Andi Kleen [mailto:[email protected]]
Sent: Wednesday, June 11, 2008 8:08 AM
To: Pavel Machek
Cc: Kasindorf, Barry; [email protected]
Subject: Re: [PATCH 1/3] AMD Family10h+ IBS support for oProfile driver:
Setup routines
Quoted 3 lines · Pavel Machek
Hi!
Quoted 1 line
This patchset supports the new profiling hardware available in the
latest AMD CPUs in the oProfile driver.
Quoted 2 lines
Review int __init op_nmi_init(struct
Quoted 2 lines
(quoted diff) +0
-0@@ -419,9 +454,15 @@ int __init op_nmi_init(struct
oprofile_operations *ops)
Quoted 8 lines
arch/x86/oprofile/nmi_int.c int __init op_nmi_init(struct oprofile_operations *ops) · line 458
Quoted 6 lines
arch/x86/oprofile/nmi_int.c +2
-1break ;case 0x10 : model = &op_athlon_spec; - cpu_type ="x86-64/family10" ; + cpu_type ="x86-64/family10h" ; +break ;Is this string visible to userspace?
Yes it is.
-Andi
Quoted 5 lines · Barry
Yes,
This is visible to userspace. The corresponding changes to the usermode code have been sent to the oProfile list. The confusion is that it is not AMD Family 10 but it is Family 16(10h). This can be confusing so we changed it.
...so we require users to upgrade userspace tools at the same moment they upgrade kernel, and they can't go to the old kernel. Bad.
(Please mention userspace change in changelog).
I don't think potential for confusion is that big... and perhaps you can do s/10/10h/ in userspace tools?
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
This is a thorny problem.
A mistake was made in the name almost 2 years ago. The longer we leave it wrong the harder it will be to change. To use IBS, the major feature added by this patch, you HAVE to upgrade the user tools anyway. If there was a way to have the driver send both I would have done that. So far it is a very narrow window of time that would have a problem.
If I get too much pushback I can leave it, but the code that sends Family10 has not been present in the kernel for very long at this point, and it should be fixed. When the patch that added Family10 went in it had the same issue, before it would have reported Family8 and worked with the older userspace tools, adding this would get a cpu type unset error.
-Barry
-----Original Message-----
From: Pavel Machek [mailto:[email protected]]
Sent: Wednesday, June 11, 2008 2:01 PM
To: Kasindorf, Barry
Cc: Andi Kleen; [email protected]
Subject: Re: [PATCH 1/3] AMD Family10h+ IBS support for oProfile driver:
Setup routines
Quoted 2 lines · Barry
Yes,
This is visible to userspace. The corresponding changes to the
usermode
Quoted 2 lines
code have been sent to the oProfile list. The confusion is that it is not AMD Family 10 but it is Family 16(10h). This can be confusing so
we
Quoted 1 line
changed it.
...so we require users to upgrade userspace tools at the same moment they upgrade kernel, and they can't go to the old kernel. Bad.
(Please mention userspace change in changelog).
I don't think potential for confusion is that big... and perhaps you can do s/10/10h/ in userspace tools?
Signature · 3 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Quoted 2 lines · Barry Kasindorf
This patchset supports the new profiling hardware available in the latest AMD CPUs in the oProfile driver.
Looks interesting! There are some patch logistics issues:
1) please rebase your patches ontop of -mm, which carries the oprofile multiplexing cleanups and enhancements already which heavily interact with your patchset.
2) -mm is also based on linux-next which has a few other changes in drivers/oprofile as well: i did a test-merge of your patches and 3/3 interacted heavily with API changes in linux-next. Part of the problem was that you mixed whitespace changes into your patch which randomly iteracted with other stuff. Cleanups are nice and welcome, but try to keep them in a separate patch. (multiple patches if the situation requires it)
3) please Cc: the x86 maintainers too because these patches change quite a few things in the x86 architecture code.
also:
arch/x86/oprofile/nmi_int.c int __init op_nmi_init(struct oprofile_operations *ops) · line 458
Quoted 5 lines
arch/x86/oprofile/nmi_int.c +2
-1case 0x10 : model = &op_athlon_spec; - cpu_type ="x86-64/family10" ; + cpu_type ="x86-64/family10h" ; +break ;
please just keep the ABI string at "x86-64/family10" - we can live with this small quirk forever. The mess to remove that quirk is much larger than the benefit it brings (which is small to non-existent).
[ you really dont want such a small issue block your patches.]
the next one is OK:
arch/x86/oprofile/nmi_int.c int __init op_nmi_init(struct oprofile_operations *ops) · line 461
Quoted 3 lines
arch/x86/oprofile/nmi_int.c +3
-0+case 0x11 : + model = &op_athlon_spec; + cpu_type ="x86-64/family11h" ;
as it's really 0x11.
Ingo
Quoted 3 lines · On Thu, 3 Jul 2008 11:20:52 +0200 Ingo Molnar <[email protected]> wrote:
1) please rebase your patches ontop of -mm, which carries the oprofile multiplexing cleanups and enhancements already which heavily interact with your patchset.
I dropped those. Because it's obvious that significant changes are to be made. So I guess working against linux-next is appropriate for this change.
Quoted 9 lines · Andrew Morton
Quoted 3 lines · On Thu, 3 Jul 2008 11:20:52 +0200 Ingo Molnar <[email protected]> wrote:
1) please rebase your patches ontop of -mm, which carries the oprofile multiplexing cleanups and enhancements already which heavily interact with your patchset.
I dropped those. Because it's obvious that significant changes are to be made. So I guess working against linux-next is appropriate for this change.
hm, okay. All the pending oprofile enhancements are nice in principle and there's like 3-4 oprofile topics that we should really start reviewing, integrating and testing:
- the gist of the perfmon2 changes which make oprofile work across context-switches. Oprofile should have done this from the get go.
- the oprofile syscall enhancements from perfmon2 for lightning-fast profiling feedback.
- oprofile multiplexing: makes hw counter constraints largely invisible. Should have been done years ago.
- new hw support like this IBS patch but it would also be nice to see BTS/PEBS support perhaps.
- [ personally i'd love to see /debug/oprofile/prof.txt that would do user-space-less parsing of oprofile data and would in essence replace readprofile for all practical purposes. (while still being as simple and self-contained as readprofile) Full in-kernel generation of human-readable text output. This would be the thing that would bring oprofile a lot closer to the average kernel developer IMO. (Also, various knobs under /debug/oprofile/* to configure oprofile details on the fly, without any userspace dependencies.) ]
- [ oprofile .config driven in-kernel self-tests. Right now we notice it quite late when it breaks. If we do /debug/oprofile/ that would be a great step towards that. ]
tip/x86/oprofile (which already has some smaller changes) could host them, if there's interest. It would be renamed to tip/oprofile or so, kept separate and exported separately for import into -mm and/or linux-next - not mixed into x86 and other -tip bits.
v2.6.28 stuff at the earliest, obviously.
Ingo
RE: [PATCH 1/3] AMD Family10h+ IBS support for oProfile driver:Setup routines
I have re-worked the IBS patchset and will break it into smaller pieces and re-submit it RSN.
I will rebase it against -mm or Linux-next whichever is best. I need Robert's AMD Family10h EPCI patches for this to build so that has to already be there in whichever tree I use.
This patchset does interact with Jason's multiplexing changes but we can work that out.
-Barry
-----Original Message-----
From: Ingo Molnar [mailto:[email protected]]
Sent: Thursday, July 03, 2008 6:01 AM
To: Andrew Morton
Cc: Kasindorf, Barry; [email protected]; Richter, Robert; the arch/x86 maintainers; Stephane Eranian; Yeh, Jason; Arjan van de Ven; Markus Metzger
Subject: Re: [PATCH 1/3] AMD Family10h+ IBS support for oProfile driver:Setup routines
Quoted 10 lines · Andrew Morton
Quoted 3 lines · On Thu, 3 Jul 2008 11:20:52 +0200 Ingo Molnar <[email protected]> wrote:
1) please rebase your patches ontop of -mm, which carries the oprofile multiplexing cleanups and enhancements already which heavily interact with your patchset.
I dropped those. Because it's obvious that significant changes are to
be made. So I guess working against linux-next is appropriate for this change.
hm, okay. All the pending oprofile enhancements are nice in principle and there's like 3-4 oprofile topics that we should really start reviewing, integrating and testing:
- the gist of the perfmon2 changes which make oprofile work across context-switches. Oprofile should have done this from the get go.
- the oprofile syscall enhancements from perfmon2 for lightning-fast profiling feedback.
- oprofile multiplexing: makes hw counter constraints largely invisible. Should have been done years ago.
- new hw support like this IBS patch but it would also be nice to see BTS/PEBS support perhaps.
- [ personally i'd love to see /debug/oprofile/prof.txt that would do user-space-less parsing of oprofile data and would in essence replace readprofile for all practical purposes. (while still being as simple and self-contained as readprofile) Full in-kernel generation of human-readable text output. This would be the thing that would bring oprofile a lot closer to the average kernel developer IMO. (Also, various knobs under /debug/oprofile/* to configure oprofile details on the fly, without any userspace dependencies.) ]
- [ oprofile .config driven in-kernel self-tests. Right now we notice it
quite late when it breaks. If we do /debug/oprofile/ that would be a
great step towards that. ]
tip/x86/oprofile (which already has some smaller changes) could host them, if there's interest. It would be renamed to tip/oprofile or so, kept separate and exported separately for import into -mm and/or linux-next - not mixed into x86 and other -tip bits.
v2.6.28 stuff at the earliest, obviously.
Ingo