Messages in this thread | | | Date | Tue, 21 Mar 2017 13:52:30 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH v5 4/7] perf/sdt: Allow recording of existing events |
| |
On Mon, 20 Mar 2017 14:42:22 +0530 Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:
[SNIP] > >> } > >> > >> -/* > >> - * Find the SDT event from the cache and if found add it/them > >> - * to the uprobe_events file > >> - */ > >> +static bool sdt_is_ptrn_used(struct perf_probe_event *pev) > > This might be sdt_name_is_glob(). > > Hmm looks good. Will change it. > > >> +{ > >> + return !is_c_func_name(pev->group) || !is_c_func_name(pev->event); > > Would you mean strisglob()@util.h ? :) > > Yes, BUT this needs to be changed now. > > I found perf probe accepts glob wildcards(*, ?) and character classes like > [a-z]. But perf record only allow wildcards. So I can't use strisglob() > function.
Oops, right. Hmm, Jiri, can we support full glob matching for events? (or what happen if we use it?)
> > Please let me know if that is wrong. > > >> +} > >> + > >> +static bool sdt_name_match(struct perf_probe_event *pev, > >> + struct probe_trace_event *tev) > >> +{ > >> + if (sdt_is_ptrn_used(pev)) > >> + return strglobmatch(tev->group, pev->group) && > >> + strglobmatch(tev->event, pev->event); > >> + > >> + return !strcmp(tev->group, pev->group) && > >> + !strcmp(tev->event, pev->event); > > Would we really need these two? Since strglobmatch() accepts a string > > without wildcard, you don't need strcmp() version... > > Hmm. yes, second part looks unnecessary. > > >> +} > >> + > >> +static void sdt_warn_multi_events(int ctr, struct perf_probe_event *pev) > >> +{ > >> + pr_warning("Warning: Recording on %d occurrences of %s:%s\n", > >> + ctr, pev->group, pev->event); > > Could you show what event will be recorded instead of just showing > > the number of events? > > Sure. I'll remove this warning and show all events as 'event addr@file'. > Please let me know if you want to list it differently. > > Actually, I do that, but partially. Please see patch 6/7. It list all those > *existing* events which are being recorded.
OK, that will be enough for users to warn.
[SNIP] > >> @@ -3606,23 +3727,37 @@ int add_sdt_event(char *event, struct list_head *sdt_evlist) > >> probe_conf.max_probes = MAX_PROBES; > >> probe_conf.force_add = 1; > >> > >> + /* Fetch all sdt events from uprobe_events */ > >> + exst_ntevs = probe_file__get_sdt_events(&exst_tevs); > >> + if (exst_ntevs < 0) { > >> + ret = exst_ntevs; > >> + goto free_pev; > >> + } > >> + > >> + /* Check if events with same name already exists in uprobe_events. */ > >> + ret = sdt_event_probepoint_exists(pev, exst_tevs, > >> + exst_ntevs, sdt_evlist); > >> + if (ret) { > >> + ret = ret > 0 ? 0 : ret; > >> + goto free_pev; > >> + } > >> + > >> /* Fetch all matching events from cache. */ > >> ret = get_sdt_events_from_cache(pev); > >> if (ret < 0) > >> goto free_pev; > > Hmm, IMHO, you'd better call get_sdt_events_from_cache() first, and > > check the existence of those events afterwards. Then you may not > > need the "shift" function. > > "shift" function is needed. let me explain. > > As we are allowing both, 'perf probe' as well as 'perf record' on SDT > events, there are two sources of events for 'perf record' > 1. uprobe_events file > 2. probe-cache > > When perf fetches events from cache, it fetch all matching events, > no matter if any event already exists in uprobe_events or not. > > Consider an example, There are 3 events with same name sdt_a:b > exists in 'test' binary. > > $ readelf -n test > sdt_a:b @ addr 0x123 > sdt_a:b @ addr 0x456 > sdt_a:b @ addr 0x789 > > $ perf probe -x test sdt_a:b > /* Add all 3 events sdt_a:b sdt_a:b_1 and sdt_a:b_2 */ > $ perf probe -d sdt_a:b > $ perf probe -d sdt_a:b_2 > > $ perf record sdt_a:b > /* > arr1 = probe_file__get_sdt_events(); > {sdt_a:b_1 addr 0x456} > > arr2 = get_sdt_events_from_cache(); > {sdt_a:b addr 0x123, sdt_a:b addr 0x456, sdt_a:b addr 0x789} > > Now, as sdt event of addr 0x456 already exists in arr1, it needs to > be reused and thus it needs to be removed from arr2. Removing > 2nd element needs shifting of third element to 2nd position. > */
Ah, I see. In that case, you can just set the tev->point.symbol = NULL, then it is just skipped to apply in __add_probe_trace_events().
So what you need to do is, 1) get_sdt_events_from_cache(pev); and 2) set tev[x].point.symbol = NULL in sdt_event_probepoint_exists(). (I also recommend to release tev[x].event and group, then you can easilly find what events should be removed afterwards)
Thank you,
> > I don't think it's easy to remove "shift" function. I can remove it but > that needs changes in existing infrastructure like fetch only those > events from cache which are not present in uprobe_events. But, > IMHO, it's not a good way to go. > > Let me know if I misunderstood your point. > > Thanks, > Ravi >
-- Masami Hiramatsu <mhiramat@kernel.org>
| |