From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752790AbdCTDwT (ORCPT ); Sun, 19 Mar 2017 23:52:19 -0400 Received: from mx0b-001b2d01.pphosted.com ([148.163.158.5]:47444 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751988AbdCTDwP (ORCPT ); Sun, 19 Mar 2017 23:52:15 -0400 Subject: Re: [PATCH v5 3/7] perf/sdt: Directly record SDT events with 'perf record' To: Masami Hiramatsu References: <20170314150658.7065-1-ravi.bangoria@linux.vnet.ibm.com> <20170314150658.7065-4-ravi.bangoria@linux.vnet.ibm.com> <20170317180554.c6b69de1e655eee60e723dbd@kernel.org> Cc: mingo@redhat.com, acme@kernel.org, brendan.d.gregg@gmail.com, peterz@infradead.org, alexander.shishkin@linux.intel.com, wangnan0@huawei.com, jolsa@kernel.org, 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, Ravi Bangoria From: Ravi Bangoria Date: Mon, 20 Mar 2017 09:21:58 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: <20170317180554.c6b69de1e655eee60e723dbd@kernel.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-TM-AS-GCONF: 00 x-cbid: 17032003-0008-0000-0000-0000077553DD X-IBM-SpamModules-Scores: X-IBM-SpamModules-Versions: BY=3.00006814; HX=3.00000240; KW=3.00000007; PH=3.00000004; SC=3.00000206; SDB=6.00836168; UDB=6.00410930; IPR=6.00613937; BA=6.00005218; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000; ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00014717; XFM=3.00000013; UTC=2017-03-20 03:52:11 X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused x-cbparentid: 17032003-0009-0000-0000-000040CC4145 Message-Id: <58CF51DE.5070300@linux.vnet.ibm.com> X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-03-20_02:,, signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=2 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1702020001 definitions=main-1703200033 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Thanks Masami for detailed review. Please see my comments below. On Friday 17 March 2017 02:35 PM, Masami Hiramatsu wrote: > Hi Ravi, > > (I avoided to review parser part since it may go to yacc in next version) > > On Tue, 14 Mar 2017 20:36:54 +0530 > Ravi Bangoria wrote: > > [SNIP] >> @@ -1516,9 +1534,10 @@ static bool dry_run; >> * using pipes, etc. >> */ >> static struct option __record_options[] = { >> - OPT_CALLBACK('e', "event", &record.evlist, "event", >> - "event selector. use 'perf list' to list available events", >> - parse_events_option), >> + OPT_CALLBACK_ARG('e', "event", &record.evlist, >> + &record.sdt_event_list, "event", >> + "event selector. use 'perf list' to list available events", >> + record__parse_events_option), > Does --event option NOT requires argument without this patch? > If it should be changed to use OPT_CALLBACK_ARG(), would it be > better merge this part to previous patch? Ok. Yes, it does. I think macro name is confusing. This new macro allows passing of extra data from builtin-*.c to libelf. One such macro already exists (OPT_CALLBACK_OPTARG), but the argument is optional for it and thus it ignores the argument. I need a macro in which argument is necessary and it also allows to pass extra data. Hence, I introduced this macro. Will change macro to OPT_CALLBACK_ARGDATA. Please suggest if you have better name. > [SNIP] >> +/* >> + * Delete the SDT events from uprobe_events file that >> + * were created initially. >> + */ >> +void remove_sdt_event_list(struct list_head *sdt_events) >> +{ >> + struct sdt_event_list *sdt_event; >> + struct strfilter *filter = NULL; >> + const char *err = NULL; >> + >> + if (list_empty(sdt_events)) >> + return; >> + >> + list_for_each_entry(sdt_event, sdt_events, list) { >> + if (!filter) { >> + filter = strfilter__new(sdt_event->name, &err); >> + if (!filter) >> + goto free_list; > Don't we need to return error code for this case? > >> + } else { >> + strfilter__or(filter, sdt_event->name, &err); > strfilter__or() can fail here. > >> + } >> + } >> + >> + del_perf_probe_events(filter); > Here too, if it is ignored silently by design, please comment it here. Sure. Will think about handling errors in this function. > >> + >> +free_list: >> + free_sdt_list(sdt_events); >> +} >> + >> +static int get_sdt_events_from_cache(struct perf_probe_event *pev) >> +{ >> + int ret = 0; >> + >> + pev->ntevs = find_cached_events_all(pev, &pev->tevs); >> + >> + if (pev->ntevs < 0) { >> + pr_err("Error: Cache lookup failed (code: %d)\n", pev->ntevs); >> + ret = pev->ntevs; >> + } else if (!pev->ntevs) { >> + pr_err("Error: %s:%s not found in the cache\n", >> + pev->group, pev->event); >> + ret = -EINVAL; >> + } else if (pev->ntevs > 1) { >> + pr_warning("Warning : Recording on %d occurences of %s:%s\n", >> + pev->ntevs, pev->group, pev->event); >> + } >> + >> + return ret; >> +} >> + >> +static int add_event_to_sdt_evlist(struct probe_trace_event *tev, >> + struct list_head *sdt_evlist) >> +{ >> + struct sdt_event_list *tmp; > Well, strbuf can make this simpler as below ;-) > > struct strbuf buf = STRBUF_INIT; Sure, will do it. Thanks :) Ravi >> + >> + tmp = zalloc(sizeof(*tmp)); >> + if (!tmp) >> + return -ENOMEM; >> + >> + INIT_LIST_HEAD(&tmp->list); > if (strbuf_addf(&buf, "%s:%s", tev->group, tev->event)) > goto error; > > tmp->name = strbuf_detach(&buf); > >> + list_add(&tmp->list, sdt_evlist); >> + >> + return 0; > error: > free(tmp); > > return -ENOMEM; >> +} >> + >> +static int add_events_to_sdt_evlist(struct perf_probe_event *pev, >> + struct list_head *sdt_evlist) >> +{ >> + int i, ret; >> + >> + for (i = 0; i < pev->ntevs; i++) { >> + ret = add_event_to_sdt_evlist(&pev->tevs[i], sdt_evlist); >> + >> + if (ret < 0) >> + return ret; >> + } >> + return 0; >> +} > > Thanks, >