From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752325AbdCUExZ (ORCPT ); Tue, 21 Mar 2017 00:53:25 -0400 Received: from mail.kernel.org ([198.145.29.136]:51954 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751212AbdCUExX (ORCPT ); Tue, 21 Mar 2017 00:53:23 -0400 Date: Tue, 21 Mar 2017 13:52:30 +0900 From: Masami Hiramatsu To: Ravi Bangoria , Jiri Olsa Cc: mingo@redhat.com, acme@kernel.org, brendan.d.gregg@gmail.com, peterz@infradead.org, alexander.shishkin@linux.intel.com, wangnan0@huawei.com, ak@linux.intel.com, treeze.taeung@gmail.com, mathieu.poirier@linaro.org, hekuang@huawei.com, sukadev@linux.vnet.ibm.com, ananth@in.ibm.com, naveen.n.rao@linux.vnet.ibm.com, adrian.hunter@intel.com, linux-kernel@vger.kernel.org, hemant@linux.vnet.ibm.com Subject: Re: [PATCH v5 4/7] perf/sdt: Allow recording of existing events Message-Id: <20170321135230.62dd78ce4f2bf5cc812a9e79@kernel.org> In-Reply-To: <58CF9CF6.5040400@linux.vnet.ibm.com> References: <20170314150658.7065-1-ravi.bangoria@linux.vnet.ibm.com> <20170314150658.7065-5-ravi.bangoria@linux.vnet.ibm.com> <20170318081341.612dbec9b99ce5fe373535b4@kernel.org> <58CF9CF6.5040400@linux.vnet.ibm.com> X-Mailer: Sylpheed 3.5.0 (GTK+ 2.24.30; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, 20 Mar 2017 14:42:22 +0530 Ravi Bangoria 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