Re: [PATCH V2 4/4] sh: Use the perf-events backend for oprofile

[Robert Richter <robert.richter@amd.com>]

On 31.08.10 08:23:43, Matt Fleming wrote:
> > > > > -static int op_sh_start(void)
> > > > > +static char *op_name_from_perf_name(const char *name)
> > > > >  {
> > > > > -	/* Enable performance monitoring for all counters.  */
> > > > > -	on_each_cpu(model->cpu_start, NULL, 1);
> > > > > +	if (!strcmp(name, "SH-4A"))
> > > > > +		return "sh/sh4a";
> > > > > +	if (!strcmp(name, "SH7750"))
> > > > > +		return "sh/sh7750";
> > > > 
> > > > With that implementation we always have to touch the code for new
> > > > cpus. Maybe we derive it from the perf name, e.g. making all lowercase
> > > > and removing dashes?
> > > 
> > > Is this code really that bad that we need to start playing string
> > > manipulation games?
> > 
> > No, but with that implementation we always have to update the cpu
> > string with each new cpu though nothing else changes. We may keep this
> > code. But, shouldn't we return a default string "sh/<name>" for all
> > other cases? We will then need to update only the oprofile userland
> > with new cpus.
> 
> These names are actually the names of types of performance counters,
> not a specific cpu. All SH-4 cpus that have performance counters have
> 7750-style performance counters and all SH-4A cpus have SH-4A-style
> counters.
> 
> It's unlikely we'd have to update this code in the near future. Paul,
> correct me if I'm wrong here.

Ok, this shouldn't block this patch series, we still can make a patch
if there is a use case.

> > > > > +	ops->setup		= oprofile_perf_setup;
> > > > > +	ops->create_files	= oprofile_perf_create_files;
> > > > > +	ops->start		= oprofile_perf_start;
> > > > > +	ops->stop		= oprofile_perf_stop;
> > > > > +	ops->cpu_type		= op_name_from_perf_name(sh_pmu_name());
> > > > >  
> > > > > -	model = lmodel;
> > > > > +	oprofile_perf_set_num_counters(sh_pmu_num_events());
> > > > >  
> > > > > -	ops->setup		= op_sh_setup;
> > > > > -	ops->create_files	= op_sh_create_files;
> > > > > -	ops->start		= op_sh_start;
> > > > > -	ops->stop		= op_sh_stop;
> > > > > -	ops->cpu_type		= lmodel->cpu_type;
> > > > > +	ret = oprofile_perf_init();
> > > > 
> > > > Instead of exporting all the functions above implement something like:
> > > > 
> > > > 	name = op_name_from_perf_name(sh_pmu_name());
> > > > 	num_events = sh_pmu_num_events();
> > > > 	ret = oprofile_perf_init(ops, name, num_events);
> > > > 
> > > > We will then have only oprofile_perf_init() and oprofile_perf_exit()
> > > > as interface which is much cleaner.
> > > 
> > > Well, the reason that I left it this way is so that architectures can
> > > choose to implement wrappers around the oprofile_perf_* functions. I
> > > don't think ARM or SH actually need wrappers (the only extra thing that
> > > ARM does is locking which SH should probably do too) but I assumed there
> > > was a reason that these functions pointers were exposed originally. I
> > > haven't look at what other architectures would do. I'll take a look at
> > > that.
> > 
> > I am not sure if we need such wrappers, and if so we could implement
> > it anyway, e.g.:
> > 
> >  oprofile_perf_init(perf_ops, name, num_events);
> > 
> >  op_sh_setup():
> > 
> > 	/* setup something */
> > 	...
> > 
> > 	perf_ops->setup();
> > 
> > 	/* setup more */
> > 	...
> > 
> > But I don't think we need this. And the above makes the interface much
> > cleaner.
> 
> OK, seeing as the two architectures that will use this initially don't
> require wrappers I've no problem doing it your way. It can always be
> extended later if necessary. And more importantly, with a proper
> usecase we'll be able to see exactly _how_ it needs to be extended.

Yes, right. So I am looking forward to your new version.

Thanks,

-Robert

-- 
Advanced Micro Devices, Inc.
Operating System Research Center