From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id CD0FDC433EF for ; Tue, 22 Feb 2022 12:19:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231826AbiBVMUE (ORCPT ); Tue, 22 Feb 2022 07:20:04 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58296 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231174AbiBVMUD (ORCPT ); Tue, 22 Feb 2022 07:20:03 -0500 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7FEC59ADB0 for ; Tue, 22 Feb 2022 04:19:37 -0800 (PST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 361B4B819B4 for ; Tue, 22 Feb 2022 12:19:36 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 41E1FC340E8; Tue, 22 Feb 2022 12:19:33 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1645532374; bh=K05Gw+mOTnjht4gvZE6R/Wicq4llHCpuYapSQpPm3f8=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=rcFeMsq5cWiu+I8s9r6GsQz/jgiEuLJIOg4GF6K+1yDzxCgFPolQ7tF+WstwK6ZnC 1svpj4WfkYFo5OdHrAn+yLllbJfzfW4ucEbMVJmeXsGFZFCgRwEoJo6A97pGu32cvt PfkZQd9j40H4YWC9Yoqux3Uknwcezc7pCWy2xo4rK24p4rw4ekM2i7hdvJC+Ms3Wds Xa9oNVGxNZPFSDnTd/5Rd0Q6+OFHZ5sjG5uKW1iO1paQa6ollPqwVnyCUJ3fxRCFHe 3DIS37NKGNFlac42LbmaVeUfDTy4njEWBi5Dbkr4RWw9w+Y5K8/IsI6rj0uBlEiD/D GcboOC5+cqMFQ== Date: Tue, 22 Feb 2022 21:19:30 +0900 From: Masami Hiramatsu To: Steven Rostedt Cc: LKML , Ingo Molnar , Andrew Morton , Joel Fernandes , Tzvetomir Stoyanov , Masami Hiramatsu Subject: Re: [PATCH] eprobes: Remove redundant event type information Message-Id: <20220222211930.458224fe0499e036f3c5a06d@kernel.org> In-Reply-To: <20220218190057.2f5a19a8@gandalf.local.home> References: <20220218190057.2f5a19a8@gandalf.local.home> X-Mailer: Sylpheed 3.7.0 (GTK+ 2.24.32; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 18 Feb 2022 19:00:57 -0500 Steven Rostedt wrote: > From 3163c1db8bbde856367b9d4e132d1bac9ec26704 Mon Sep 17 00:00:00 2001 > From: "Steven Rostedt (Google)" > Date: Fri, 18 Feb 2022 18:52:38 -0500 > Subject: [PATCH] eprobes: Remove redundant event type information > > Currently, the event probes save the type of the event they are attached > to when recording the event. For example: > > # echo 'e:switch sched/sched_switch prev_state=$prev_state prev_prio=$prev_prio next_pid=$next_pid next_prio=$next_prio' > dynamic_events > # cat events/eprobes/switch/format > > name: switch > ID: 1717 > format: > field:unsigned short common_type; offset:0; size:2; signed:0; > field:unsigned char common_flags; offset:2; size:1; signed:0; > field:unsigned char common_preempt_count; offset:3; size:1; signed:0; > field:int common_pid; offset:4; size:4; signed:1; > > field:unsigned int __probe_type; offset:8; size:4; signed:0; > field:u64 prev_state; offset:12; size:8; signed:0; > field:u64 prev_prio; offset:20; size:8; signed:0; > field:u64 next_pid; offset:28; size:8; signed:0; > field:u64 next_prio; offset:36; size:8; signed:0; > > print fmt: "(%u) prev_state=0x%Lx prev_prio=0x%Lx next_pid=0x%Lx next_prio=0x%Lx", REC->__probe_type, REC->prev_state, REC->prev_prio, REC->next_pid, REC->next_prio > > The __probe_type adds 4 bytes to every event. > > One of the reasons for creating eprobes is to limit what is traced in an > event to be able to limit what is written into the ring buffer. Having > this redundant 4 bytes to every event takes away from this. > > The event that is recorded can be retrieved from the event probe itself, > that is available when the trace is happening. For user space tools, it > could simply read the dynamic_event file to find the event they are for. > So there is really no reason to write this information into the ring > buffer for every event. OK, This looks good to me. Acked-by: Masami Hiramatsu Thank you, > > Signed-off-by: Steven Rostedt (Google) > --- > kernel/trace/trace.h | 1 - > kernel/trace/trace_eprobe.c | 15 +++++++-------- > kernel/trace/trace_probe.c | 10 +++++----- > kernel/trace/trace_probe.h | 1 - > 4 files changed, 12 insertions(+), 15 deletions(-) > > diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h > index 0f5e22238cd2..07d990270e2a 100644 > --- a/kernel/trace/trace.h > +++ b/kernel/trace/trace.h > @@ -136,7 +136,6 @@ struct kprobe_trace_entry_head { > > struct eprobe_trace_entry_head { > struct trace_entry ent; > - unsigned int type; > }; > > struct kretprobe_trace_entry_head { > diff --git a/kernel/trace/trace_eprobe.c b/kernel/trace/trace_eprobe.c > index 191db32dec46..02838d47129f 100644 > --- a/kernel/trace/trace_eprobe.c > +++ b/kernel/trace/trace_eprobe.c > @@ -250,8 +250,6 @@ static int eprobe_event_define_fields(struct trace_event_call *event_call) > if (WARN_ON_ONCE(!tp)) > return -ENOENT; > > - DEFINE_FIELD(unsigned int, type, FIELD_STRING_TYPE, 0); > - > return traceprobe_define_arg_fields(event_call, sizeof(field), tp); > } > > @@ -270,7 +268,9 @@ print_eprobe_event(struct trace_iterator *iter, int flags, > struct trace_event_call *pevent; > struct trace_event *probed_event; > struct trace_seq *s = &iter->seq; > + struct trace_eprobe *ep; > struct trace_probe *tp; > + unsigned int type; > > field = (struct eprobe_trace_entry_head *)iter->ent; > tp = trace_probe_primary_from_call( > @@ -278,15 +278,18 @@ print_eprobe_event(struct trace_iterator *iter, int flags, > if (WARN_ON_ONCE(!tp)) > goto out; > > + ep = container_of(tp, struct trace_eprobe, tp); > + type = ep->event->event.type; > + > trace_seq_printf(s, "%s: (", trace_probe_name(tp)); > > - probed_event = ftrace_find_event(field->type); > + probed_event = ftrace_find_event(type); > if (probed_event) { > pevent = container_of(probed_event, struct trace_event_call, event); > trace_seq_printf(s, "%s.%s", pevent->class->system, > trace_event_name(pevent)); > } else { > - trace_seq_printf(s, "%u", field->type); > + trace_seq_printf(s, "%u", type); > } > > trace_seq_putc(s, ')'); > @@ -498,10 +501,6 @@ __eprobe_trace_func(struct eprobe_data *edata, void *rec) > return; > > entry = fbuffer.entry = ring_buffer_event_data(fbuffer.event); > - if (edata->ep->event) > - entry->type = edata->ep->event->event.type; > - else > - entry->type = 0; > store_trace_args(&entry[1], &edata->ep->tp, rec, sizeof(*entry), dsize); > > trace_event_buffer_commit(&fbuffer); > diff --git a/kernel/trace/trace_probe.c b/kernel/trace/trace_probe.c > index 73d90179b51b..80863c6508e5 100644 > --- a/kernel/trace/trace_probe.c > +++ b/kernel/trace/trace_probe.c > @@ -871,15 +871,15 @@ static int __set_print_fmt(struct trace_probe *tp, char *buf, int len, > switch (ptype) { > case PROBE_PRINT_NORMAL: > fmt = "(%lx)"; > - arg = "REC->" FIELD_STRING_IP; > + arg = ", REC->" FIELD_STRING_IP; > break; > case PROBE_PRINT_RETURN: > fmt = "(%lx <- %lx)"; > - arg = "REC->" FIELD_STRING_FUNC ", REC->" FIELD_STRING_RETIP; > + arg = ", REC->" FIELD_STRING_FUNC ", REC->" FIELD_STRING_RETIP; > break; > case PROBE_PRINT_EVENT: > - fmt = "(%u)"; > - arg = "REC->" FIELD_STRING_TYPE; > + fmt = ""; > + arg = ""; > break; > default: > WARN_ON_ONCE(1); > @@ -903,7 +903,7 @@ static int __set_print_fmt(struct trace_probe *tp, char *buf, int len, > parg->type->fmt); > } > > - pos += snprintf(buf + pos, LEN_OR_ZERO, "\", %s", arg); > + pos += snprintf(buf + pos, LEN_OR_ZERO, "\"%s", arg); > > for (i = 0; i < tp->nr_args; i++) { > parg = tp->args + i; > diff --git a/kernel/trace/trace_probe.h b/kernel/trace/trace_probe.h > index 99e7a5df025e..92cc149af0fd 100644 > --- a/kernel/trace/trace_probe.h > +++ b/kernel/trace/trace_probe.h > @@ -38,7 +38,6 @@ > #define FIELD_STRING_IP "__probe_ip" > #define FIELD_STRING_RETIP "__probe_ret_ip" > #define FIELD_STRING_FUNC "__probe_func" > -#define FIELD_STRING_TYPE "__probe_type" > > #undef DEFINE_FIELD > #define DEFINE_FIELD(type, item, name, is_signed) \ > -- > 2.34.1 > -- Masami Hiramatsu