Re: [RFC PATCH 1/3] perf probe: Init symbol as kprobe if any pev is kprobe

[Wangnan (F)]

On 2015/6/27 16:30, Masami Hiramatsu wrote:
> Hi Wang,
>
> On 2015/06/27 16:34, Wangnan (F) wrote:
> > On 2015/6/27 15:29, Masami Hiramatsu wrote:
> > > On 2015/06/25 19:37, Wang Nan wrote:
> > > > Before this patch, add_perf_probe_events() init symbol maps only for
> > > > uprobe if the first pev passed to it is a uprobe event. However, with
> > > > the incoming BPF uprobe support, now it will be possible to pass an
> > > > array with combined kprobe and uprobe events to add_perf_probe_events().
> > > This description is not correct. Actually, add_perf_probe_events already
> > > supports mix of uprobes and kprobes. However, from the command line syntax
> > > constrains the first elements of the probe_event arrays must be kprobes.
> > > So, if the array starts with uprobes, no kprobes should be there.
> > >
> > > > This patch check all pevs instead of the first one, and init kernel
> > > > symbol if any events is not uprobe.
> > > Anyway, I prefer to call init_symbol_maps() with "false" :)
> > I also prefer "false", so I try to find whether all events are uprobe
> > instead
> > of init_symbol_maps(true).
> >
> > So for this patch only commit message needs to be corrected, the code is
> > no problem, right?
> No, I meant you just need to change one line, as below.
>
> -	ret = init_symbol_maps(pevs->uprobes);
> +	ret = init_symbol_maps(false);
>
> That's enough, I don't like to introduce a bool flag and loop.
>
> Thanks,

Then basic structure of kernel map will be build even if all probe 
points are
uprobes. It costs some memory. But sure, we can skip the loop.

Will post an updated version.

Thank you.

> > Thank you.
> >
> > > Thank you,
> > >
> > > > Signed-off-by: Wang Nan <wangnan0@xxxxxxxxxx>
> > > > ---
> > > >    tools/perf/util/probe-event.c | 15 ++++++++++++++-
> > > >    1 file changed, 14 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> > > > index b386d2f..a2b3026 100644
> > > > --- a/tools/perf/util/probe-event.c
> > > > +++ b/tools/perf/util/probe-event.c
> > > > @@ -2802,8 +2802,21 @@ int cleanup_perf_probe_event(struct perf_probe_event *pev)
> > > >    int add_perf_probe_events(struct perf_probe_event *pevs, int npevs, bool cleanup)
> > > >    {
> > > >    	int i, ret;
> > > > +	bool user_only = true;
> > > >    
> > > > -	ret = init_symbol_maps(pevs->uprobes);
> > > > +	/* If any pev is kprobe, init kernel symbols. */
> > > > +	for (i = 0; i < npevs; i++) {
> > > > +		if (!pevs[i].uprobes) {
> > > > +			user_only = false;
> > > > +			break;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	/*
> > > > +	 * Compiler can drop user_only:
> > > > +	 *  ret = init_symbol_maps(i >= npevs);
> > > > +	 */
> > > > +	ret = init_symbol_maps(user_only);
> > > >    	if (ret < 0)
> > > >    		return ret;
> > > >    


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/