Messages in this thread | | | Date | Wed, 23 Jul 2008 21:46:08 +0200 | From | Robert Richter <> | Subject | Re: [PATCH 10/24] x86/oprofile: Add IBS support for AMD CPUs, IBS buffer handling routines |
| |
On 23.07.08 14:20:37, Maynard Johnson wrote: > Robert Richter wrote:
[...]
> > @@ -252,6 +253,71 @@ void oprofile_add_sample(struct pt_regs * const regs, unsigned long event) > > oprofile_add_ext_sample(pc, regs, event, is_kernel); > > } > > > > +#define MAX_IBS_SAMPLE_SIZE 14 > > +static int log_ibs_sample(struct oprofile_cpu_buffer *cpu_buf, > > + unsigned long pc, int is_kernel, unsigned int *ibs, int ibs_code) > > +{ > > + struct task_struct *task; > > + > > + cpu_buf->sample_received++; > > + > > + if (nr_available_slots(cpu_buf) < MAX_IBS_SAMPLE_SIZE) { > > + cpu_buf->sample_lost_overflow++; > > + return 0; > > + } > > + > > + is_kernel = !!is_kernel; > > + > > + /* notice a switch from user->kernel or vice versa */ > > + if (cpu_buf->last_is_kernel != is_kernel) { > > + cpu_buf->last_is_kernel = is_kernel; > > + add_code(cpu_buf, is_kernel); > > + } > > + > > + /* notice a task switch */ > > + if (!is_kernel) { > > + task = current; > > + > > + if (cpu_buf->last_task != task) { > > + cpu_buf->last_task = task; > > + add_code(cpu_buf, (unsigned long)task); > > + } > > + } > > + > > + add_code(cpu_buf, ibs_code); > > + add_sample(cpu_buf, ibs[0], ibs[1]); > > + add_sample(cpu_buf, ibs[2], ibs[3]); > > + add_sample(cpu_buf, ibs[4], ibs[5]); > > + > > + if (ibs_code == IBS_OP_BEGIN) { > > + add_sample(cpu_buf, ibs[6], ibs[7]); > > + add_sample(cpu_buf, ibs[8], ibs[9]); > > + add_sample(cpu_buf, ibs[10], ibs[11]); > > + } > > + > > + return 1; > > +} > > + > > > I'm concerned about the arch-specific nature of the "ibs" functions > above being placed here in the generic portion of the oprofile driver. > Better to generalize the external function defined below (and rename it) > by invoking arch-specific handlers via function pointers. Hopefully > find a way to move the arch-specific code back to where it belongs, > under the appropriate arch/ directory.
Yes, this is true. The plan is to rework the code so that IBS is only in op_model_amd.c. The only thing visible outside will then be the code macros introduced in patch #9. The code will change into generic with something like add_u64(). This function will then be called from the IBS handler.
This is also the reason I did not add op_amd_handle_ibs() to the API definition in linux/oprofile.h.
-Robert
-- Advanced Micro Devices, Inc. Operating System Research Center email: robert.richter@amd.com
| |