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 -- To unsubscribe from this list: send the line "unsubscribe linux-arch" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html