Received-SPF: pass (google.com: domain of linux-kernel+bounces-36975-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Wed, 24 Jan 2024 21:27:38 +0900
From: Masami Hiramatsu (Google) <mhiramat@kernel.org>
To: "yebin (H)" <yebin10@huawei.com>
Cc: <rostedt@goodmis.org>, <mathieu.desnoyers@efficios.com>,
 <linux-trace-kernel@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 3/7] tracing/probes: support '%pd' type for print
 struct dentry's name
Message-Id: <20240124212738.04b1beee70ccf5b5148b2ac8@kernel.org>
In-Reply-To: <65B079F2.4000802@huawei.com>
References: <20240123092139.3698375-1-yebin10@huawei.com>
	<20240123092139.3698375-4-yebin10@huawei.com>
	<20240123234013.52b1834908da7be0272e7a73@kernel.org>
	<65B079F2.4000802@huawei.com>
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

On Wed, 24 Jan 2024 10:46:10 +0800
"yebin (H)" <yebin10@huawei.com> wrote:

> 
> 
> On 2024/1/23 22:40, Masami Hiramatsu (Google) wrote:
> > On Tue, 23 Jan 2024 17:21:35 +0800
> > Ye Bin <yebin10@huawei.com> wrote:
> >
> >> Similar to '%pd' for printk, use '%pd' for print struct dentry's name.
> >>
> >> Signed-off-by: Ye Bin <yebin10@huawei.com>
> >> ---
> >>   kernel/trace/trace_kprobe.c | 6 ++++++
> >>   kernel/trace/trace_probe.h  | 1 +
> >>   2 files changed, 7 insertions(+)
> >>
> >> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
> >> index c4c6e0e0068b..00b74530fbad 100644
> >> --- a/kernel/trace/trace_kprobe.c
> >> +++ b/kernel/trace/trace_kprobe.c
> >> @@ -779,6 +779,7 @@ static int __trace_kprobe_create(int argc, const char *argv[])
> >>   	char buf[MAX_EVENT_NAME_LEN];
> >>   	char gbuf[MAX_EVENT_NAME_LEN];
> >>   	char abuf[MAX_BTF_ARGS_LEN];
> >> +	char dbuf[MAX_DENTRY_ARGS_LEN];
> > Hmm, no, I don't like to expand stack anymore. Please allocate it
> > from heap.
> Do I need to change the other buffers on the stacks to allocate memory 
> from heap?

No, that is not needed for this series, but if you want, you can :)

> >>   	struct traceprobe_parse_context ctx = { .flags = TPARG_FL_KERNEL };
> >>   
> >>   	switch (argv[0][0]) {
> >> @@ -930,6 +931,11 @@ static int __trace_kprobe_create(int argc, const char *argv[])
> >>   		argv = new_argv;
> >>   	}
> >>   
> >> +	ret = traceprobe_expand_dentry_args(argc, argv, dbuf,
> >> +					    MAX_DENTRY_ARGS_LEN);
> >> +	if (ret)
> >> +		goto out;
> > And calling this here will not cover the trace_fprobe.
> >
> > Could you call this from traceprobe_expand_meta_args() instead of
> > calling it directly from trace_kprobe? Then it can be used from
> > fprobe_event too.
> >
> > Thank you,
> At first I wanted to implement the extension logic in 
> traceprobe_expand_meta_args(),
> but I found that the code was difficult to understand when I started 
> writing.

Yeah, I also found that is a bit different usage.

> If fprobe_event
> wants to support this function, is traceprobe_expand_dentry_args() also 
> called?

Yes, it is for expanding '$arg*' into '$arg1 $arg2 ...'

> Or re-encapsulate
> an interface to include the logic of different extensions. In this way, 
> the same buffer is used for
> the entire extension process, and the extension function needs to return 
> the information about
> the length of the buffer.

OK, I confirmed that will be too much complicated. Then can you just call
it from where the traceprobe_expand_meta_args() is called, which is 
__trace_fprobe_create()@trace_fprobe.c ?

But those should be simplified later.

Thank you,

-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>