Message-Id: <58CF9CF6.5040400@linux.vnet.ibm.com>
Date:   Mon, 20 Mar 2017 14:42:22 +0530
From:   Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
To:     Masami Hiramatsu <mhiramat@...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, jolsa@...nel.org, 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,
        Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com>
Subject: Re: [PATCH v5 4/7] perf/sdt: Allow recording of existing events

Thanks Masami for detailed review.

Please see my comments below.

On Saturday 18 March 2017 04:43 AM, Masami Hiramatsu wrote:
> On Tue, 14 Mar 2017 20:36:55 +0530
> Ravi Bangoria <ravi.bangoria@...ux.vnet.ibm.com> wrote:
>
>>  		}
>>  	}
>>  
>> -	del_perf_probe_events(filter);
>> +	if (filter)
>> +		del_perf_probe_events(filter);
> Hmm, I found strfilter__string(NULL) (which will happen if 
> del_perf_probe_events(NULL) invoked) causes SEGV. It should be safe
> for NULL instead of checking here.

Sure will change accordingly.

>>  }
>>  
>> -/*
>> - * 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.

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.

>> +}
>> +
>> +static int sdt_event_probepoint_exists(struct perf_probe_event *pev,
>> +				       struct probe_trace_event *tevs,
>> +				       int ntevs,
>> +				       struct list_head *sdt_evlist)
>> +{
>> +	int i = 0, ret = 0, ctr = 0;
>> +
>> +	for (i = 0; i < ntevs; i++) {
>> +		if (sdt_name_match(pev, &tevs[i])) {
>> +			ret = add_event_to_sdt_evlist(&tevs[i],
>> +						sdt_evlist, true);
>> +			if (ret < 0)
>> +				return ret;
>> +
>> +			ctr++;
>> +		}
>> +	}
>> +
>> +	if (ctr > 1)
>> +		sdt_warn_multi_events(ctr, pev);
>> +
>> +	return ctr;
>> +}
>> +
>> +static bool sdt_file_addr_match(struct probe_trace_event *tev1,
>> +				struct probe_trace_event *tev2)
>> +{
>> +	return (tev1->point.address == tev2->point.address &&
>> +		!(strcmp(tev1->point.module, tev2->point.module)));
>> +}
>> +
>> +static void shift_sdt_events(struct perf_probe_event *pev, int i)
>> +{
>> +	int j = 0;
>> +
>> +	clear_probe_trace_event(&pev->tevs[i]);
>> +
>> +	if (i == pev->ntevs - 1)
>> +		goto out;
>> +
>> +	for (j = i; j < pev->ntevs - 1; j++)
>> +		memcpy(&pev->tevs[j], &pev->tevs[j + 1],
>> +		       sizeof(struct probe_trace_event));
>> +
>> +out:
>> +	pev->ntevs--;
>> +}
>> +
>> +static int sdt_merge_events(struct perf_probe_event *pev,
>> +			    struct probe_trace_event *exst_tevs,
>> +			    int exst_ntevs,
>> +			    struct list_head *sdt_evlist)
>> +{
>> +	int i, j, ret = 0, ctr = 0;
>> +	bool ptrn_used = sdt_is_ptrn_used(pev);
>> +
>> +	for (i = 0; i < pev->ntevs; i++) {
>> +		for (j = 0; j < exst_ntevs; j++) {
>> +			if (sdt_file_addr_match(&pev->tevs[i],
>> +						&exst_tevs[j])) {
>> +				ret = add_event_to_sdt_evlist(&exst_tevs[j],
>> +							  sdt_evlist, true);
>> +				if (ret < 0)
>> +					return ret;
>> +
>> +				if (!ptrn_used)
>> +					shift_sdt_events(pev, i);
>> +				ctr++;
>> +			}
>> +		}
>> +	}
>> +
>> +	if (!ptrn_used || ctr == 0) {
>> +		/*
>> +		 * Create probe point for all probe-cached events by
>> +		 * adding them in uprobe_events file.
>> +		 */
>> +		ret = apply_perf_probe_events(pev, 1);
>> +		if (ret < 0) {
>> +			pr_err("Error in adding SDT event: %s:%s\n",
>> +				pev->group, pev->event);
>> +			goto out;
>> +		}
>> +
>> +		/* Add events to sdt_evlist. */
>> +		ret = add_events_to_sdt_evlist(pev, sdt_evlist);
>> +		if (ret < 0) {
>> +			pr_err("Error while updating event list\n");
>> +			goto out;
>> +		}
>> +
>> +		ctr += pev->ntevs;
>> +		if (ctr > 1)
>> +			sdt_warn_multi_events(ctr, pev);
>> +	}
>> +
>> +out:
>> +	return ret;
>> +}
>> +
>>  int add_sdt_event(char *event, struct list_head *sdt_evlist)
>>  {
>>  	struct perf_probe_event *pev;
>> -	int ret;
>> +	int ret, exst_ntevs;
>> +	struct probe_trace_event *exst_tevs = NULL;
>>  
>>  	pev = zalloc(sizeof(*pev));
>>  	if (!pev)
>> @@ -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.
    */

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

Powered by blists - more mailing lists