| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20170321135230.62dd78ce4f2bf5cc812a9e79@kernel.org>
Date: Tue, 21 Mar 2017 13:52:30 +0900
From: Masami Hiramatsu <mhiramat@...nel.org>
To: Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>,
Jiri Olsa <jolsa@...nel.org>
Cc: mingo@...hat.com, acme@...nel.org, brendan.d.gregg@...il.com,
peterz@...radead.org, alexander.shishkin@...ux.intel.com,
wangnan0@...wei.com, ak@...ux.intel.com, treeze.taeung@...il.com,
mathieu.poirier@...aro.org, hekuang@...wei.com,
sukadev@...ux.vnet.ibm.com, ananth@...ibm.com,
naveen.n.rao@...ux.vnet.ibm.com, adrian.hunter@...el.com,
linux-kernel@...r.kernel.org, hemant@...ux.vnet.ibm.com
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@...ux.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@...e'.
> 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@...nel.org>
Powered by blists - more mailing lists