Re: [PATCH 6/6] sh: oprofile: Use perf-events oprofile backend

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

On 27.09.10 16:01:38, Matt Fleming wrote:
> On Thu, Sep 16, 2010 at 04:32:54PM +0200, Robert Richter wrote:
> > > diff --git a/arch/sh/kernel/perf_event.c b/arch/sh/kernel/perf_event.c
> > > index 2cb9ad5..3c3fc9a 100644
> > > --- a/arch/sh/kernel/perf_event.c
> > > +++ b/arch/sh/kernel/perf_event.c
> > > @@ -59,6 +59,14 @@ static inline int sh_pmu_initialized(void)
> > >  	return !!sh_pmu;
> > >  }
> > >  
> > > +const char *sh_pmu_name(void)
> > > +{
> > > +	if (!sh_pmu)
> > > +		return NULL;
> > > +
> > > +	return sh_pmu->name;
> > > +}
> > 
> > Couldn't we make this a generic function like perf_num_counters()?
> 
> Well, ARM doesn't have names as strings for its pmus currently. What's
> more, ARM wouldn't use it; SH would be the only user of this function. I
> don't think this one makes sense to be a generic function.

I didn't catch this with my first review, the function will need an
EXPORT_SYMBOL_GPL() to allow building modules. This will mean an
interface extension what should be non-arch. So, for architectures we
need the pmu name like SH we just implement the generic function. For
ARM we don't need to provide this function.

Most of the interface is defined in linux/perf_event.h. We shouldn't
move this to asm/perf_event.h, so this is one more argument for the
non-arch implementation.

As the implementation of the function would be optional, why should we
make it architectural?

-Robert

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