Re: [PATCH v5 4/7] perf/sdt: Allow recording of existing events

[Masami Hiramatsu <mhiramat@kernel.org>]

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>