Message-ID: <56499DBF.80903@huawei.com>
Date: Mon, 16 Nov 2015 17:11:27 +0800
From: "Wangnan (F)" <wangnan0@huawei.com>
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0
MIME-Version: 1.0
To: Arnaldo Carvalho de Melo <acme@kernel.org>
CC: <masami.hiramatsu.pt@hitachi.com>, <ast@kernel.org>, <lizefan@huawei.com>,
        <pi3orama@163.com>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 05/12] perf tools: Allow BPF program config probing options
References: <1447417761-156094-1-git-send-email-wangnan0@huawei.com> <1447417761-156094-6-git-send-email-wangnan0@huawei.com> <20151113154629.GL7160@kernel.org>
In-Reply-To: <20151113154629.GL7160@kernel.org>
Content-Type: text/plain; charset="utf-8"; format=flowed
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3193
Lines: 105


On 2015/11/13 23:46, Arnaldo Carvalho de Melo wrote:
> Em Fri, Nov 13, 2015 at 12:29:14PM +0000, Wang Nan escreveu:
>> By extending the syntax of BPF object section names, this patch allows
>> user to config probing options like what they can do in 'perf probe'.
>>
>> Test result:
>>
>> For following BPF file bpf.c:
>>
>>   SEC("inlines=no\n"
>>       "func=SyS_dup?")
>>   int func(void *ctx)
>>   {
>>   	return 1;
>>   }
>>
>>   Cmdline:
>>
>>   # ./perf record  -e ./test_probe_glob.c ls /
>>   ...
>>   [ perf record: Woken up 1 times to write data ]
>>   [ perf record: Captured and wrote 0.013 MB perf.data ]
>>   # ./perf evlist
>>   perf_bpf_probe:func_1
>>   perf_bpf_probe:func
>>
>> Change "inlines=no" to "inlines=yes":
>>
>> Cmdline:
>>
>>   # ./perf record  -e ./test_probe_glob.c ls /
>>   ...
>>   [ perf record: Woken up 2 times to write data ]
>>   [ perf record: Captured and wrote 0.013 MB perf.data ]
>>   # ./perf evlist
>>   perf_bpf_probe:func_3
>>   perf_bpf_probe:func_2
>>   perf_bpf_probe:func_1
>>   perf_bpf_probe:func
>>
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> Cc: Alexei Starovoitov <ast@kernel.org>
>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
>> Cc: Zefan Li <lizefan@huawei.com>
>> Cc: pi3orama@163.com
>> ---

[SNIP]

>> diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
>> index 47b1e36..55785d5 100644
>> --- a/tools/perf/util/util.c
>> +++ b/tools/perf/util/util.c
>> @@ -695,3 +695,21 @@ fetch_kernel_version(unsigned int *puint, char *str,
>>   		*puint = (version << 16) + (patchlevel << 8) + sublevel;
>>   	return 0;
>>   }
>> +
>> +int convert_str_to_bool(const char *str, bool *result)
> strtobool() should be more compact and convey the same idea....
>
> Hey, I googled for that name and guess what, the kernel has exactly this
> function:
>
> lib/string.c
>
> /**
>   * strtobool - convert common user inputs into boolean values
>   * @s: input string
>   * @res: result
>   *
>   * This routine returns 0 iff the first character is one of 'Yy1Nn0'.
>   * Otherwise it will return -EINVAL.  Value pointed to by res is
>   * updated upon finding a match.
>   */
>
> include/linux/string.h
>
> So, please add it to tools/include/linux/string.h and
> tools/lib/util/string.c, this way we use the same code as the kernel,
> with the same function signature, etc.

Good suggestion. One small inconvenience: kernel's strtobool
only support 'yes|no', it doesn't match perf's config, which
requires parsing yes/no/on/off/true/false. In addition, it
accept '0/1' but perf config not.

It is very easy for me to change strtobool() to match them.
However, do you think we can provide a function with same
name with kernel but have different behavior?

However I think we can first clone that function and use it in
BPF without touching perf config, then decide whether we need
to change strtobool() or not.

Thank you.

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