From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1753039AbbCSPdb (ORCPT <rfc822;w@1wt.eu>);
	Thu, 19 Mar 2015 11:33:31 -0400
Received: from mail-ie0-f172.google.com ([209.85.223.172]:35342 "EHLO
	mail-ie0-f172.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751144AbbCSPdZ (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Thu, 19 Mar 2015 11:33:25 -0400
Message-ID: <550AEC44.3060107@plumgrid.com>
Date: Thu, 19 Mar 2015 08:33:24 -0700
From: Alexei Starovoitov <ast@plumgrid.com>
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:31.0) Gecko/20100101 Thunderbird/31.5.0
MIME-Version: 1.0
To: Steven Rostedt <rostedt@goodmis.org>
CC: Ingo Molnar <mingo@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
        Arnaldo Carvalho de Melo <acme@infradead.org>,
        Jiri Olsa <jolsa@redhat.com>,
        Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>,
        "David S. Miller" <davem@davemloft.net>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Peter Zijlstra <a.p.zijlstra@chello.nl>, linux-api@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v7 tip 2/8] tracing: attach BPF programs to kprobes
References: <1426542584-9406-1-git-send-email-ast@plumgrid.com>	<1426542584-9406-3-git-send-email-ast@plumgrid.com> <20150319110742.7dc9473d@gandalf.local.home>
In-Reply-To: <20150319110742.7dc9473d@gandalf.local.home>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 3/19/15 8:07 AM, Steven Rostedt wrote:
>>   struct trace_print_flags {
>>   	unsigned long		mask;
>> @@ -252,6 +253,7 @@ enum {
>>   	TRACE_EVENT_FL_WAS_ENABLED_BIT,
>>   	TRACE_EVENT_FL_USE_CALL_FILTER_BIT,
>>   	TRACE_EVENT_FL_TRACEPOINT_BIT,
>> +	TRACE_EVENT_FL_KPROBE_BIT,
>
> I think this should be broken up into two patches. One that adds the
> KPROBE_BIT flag to kprobe events, and the other that adds the bpf
> program.

sure. will do.

>
> There should be kerneldoc comments above this function.
>
>> +unsigned int trace_call_bpf(struct bpf_prog *prog, void *ctx)

ok.

>> +	per_cpu(bpf_prog_active, cpu)--;
>
> Hmm, as cpu == smp_processor_id(), you should be using
> __this_cpu_inc_return(), and __this_cpu_dec().

yeah. picked a wrong place to copy paste from.
will do. good point.

>
> Why not just make kprobe_prog_funcs[] = {
> 	[BPF_FUNC_map_lookup_elem] = &bpf_map_lookup_elem_proto,
> 	[BPF_FUNC_map_update_elem] = &bpf_map_update_elem_proto,
> 	[BPF_FUNC_map_delete_elem] = &bpf_map_delete_elem_proto,
> 	[BPF_FUNC_probe_read] = &kprobe_prog_proto,
>
> And define kprobe_prog_proto separately.
>
> And then you don't need the switch statement, you could just use the
> if (func_id < 0 || func_id >= ARRAY_SIZE(kprobe_prog_funcs))
> 	return kprobe_prog_funcs[func_id];
>
> I think there's several advantages to my approach. One, is that you are
> not wasting memory with empty objects in the array. Also, if the array
> gets out of sync with the enum, it would be possible to return an empty
> object. That is, &kprobe_prog_funcs[out_of_sync_func_id], would not be
> NULL if in the future someone added an enum before BPF_FUNC_probe_read,
> and the func_id passed in is that enum.

yes! There will be gaps in func_ids, so that would have
been a bug. Thanks for catching it!