Messages in this thread | | | From | Namhyung Kim <> | Date | Tue, 23 Apr 2024 16:05:48 -0700 | Subject | Re: [RFC PATCH v6 2/5] perf stat: Fork and launch perf record when perf stat needs to get retire latency value for a metric. |
| |
On Tue, Apr 23, 2024 at 3:16 PM Wang, Weilin <weilin.wang@intel.com> wrote: > > > > > -static int __run_perf_record(void) > > > > > +static int __run_perf_record(const char **record_argv) > > > > > { > > > > > + int i = 0; > > > > > + struct tpebs_event *e; > > > > > + > > > > > pr_debug("Prepare perf record for retire_latency\n"); > > > > > + > > > > > + record_argv[i++] = "perf"; > > > > > + record_argv[i++] = "record"; > > > > > + record_argv[i++] = "-W"; > > > > > + record_argv[i++] = "--synth=no"; > > > > > + > > > > > + if (stat_config.user_requested_cpu_list) { > > > > > + record_argv[i++] = "-C"; > > > > > + record_argv[i++] = stat_config.user_requested_cpu_list; > > > > > + } > > > > > + > > > > > + if (stat_config.system_wide) > > > > > + record_argv[i++] = "-a"; > > > > > + > > > > > + if (!stat_config.system_wide > > && !stat_config.user_requested_cpu_list) > > > > { > > > > > + pr_err("Require -a or -C option to run sampling.\n"); > > > > > + return -ECANCELED; > > > > > + } > > > > > + > > > > > + list_for_each_entry(e, &stat_config.tpebs_events, nd) { > > > > > + record_argv[i++] = "-e"; > > > > > + record_argv[i++] = e->name; > > > > > + } > > > > > + > > > > > + record_argv[i++] = "-o"; > > > > > + record_argv[i++] = PERF_DATA; > > > > > + > > > > > return 0; > > > > > } > > > > > > > > Still I think it's weird it has 'perf record' in perf stat (despite the > > > > 'perf stat record'). If it's only Intel thing and we don't have a plan > > > > to do the same on other arches, we can move it to the arch > > > > directory and keep the perf stat code simple. > > > > > > I'm not sure what is the proper way to solve this. And Ian mentioned > > > that put code in arch directory could potentially cause other bugs. > > > So I'm wondering if we could keep this code here for now. I could work > > > on it later if we found it's better to be in arch directory. > > > > Maybe somewhere in the util/ and keep the main code minimal. > > IIUC it's only for very recent (or upcoming?) Intel CPUs and we > > don't have tests (hopefully can run on other arch/CPUs). > > > > So I don't think having it here would help fixing potential bugs. > > Do you mean by creating a new file in util/ to hold this code?
Yeah, maybe util/intel-tpebs.c (if it's better than arch/x86/...) ?
> > Yes, this feature is for very recent Intel CPUs. It should only be triggered if > a metric uses event(s) that has the R modifier in the formula.
Can we have a test with a fake metric so that we can test the code on non-(or old-)Intel machines?
Thanks, Namhyung
| |