Message-ID: <0858c40c-a750-43f4-b7d6-f3e654d093b2@netronome.com>
Date:   Sat, 15 Dec 2018 03:31:14 +0000
From:   Quentin Monnet <quentin.monnet@...ronome.com>
To:     Stanislav Fomichev <sdf@...ichev.me>
Cc:     Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
        oss-drivers@...ronome.com,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Jesper Dangaard Brouer <brouer@...hat.com>,
        Stanislav Fomichev <sdf@...gle.com>
Subject: Re: [PATCH bpf-next 1/8] tools: bpftool: add basic probe capability,
 probe syscall and kversion

2018-12-14 10:45 UTC-0800 ~ Stanislav Fomichev <sdf@...ichev.me>
> On 12/14, Quentin Monnet wrote:
>> 2018-12-13 18:50 UTC-0800 ~ Stanislav Fomichev <sdf@...ichev.me>
>>> On 12/13, Quentin Monnet wrote:
>>>> Add a new component and command for bpftool, in order to probe the
>>>> system to dump a set of eBPF-related parameters so that users can know
>>>> what features are available on the system.
>>>>
>>>> Parameters are dumped in plain or JSON output (with -j/-p options).
>>>> Additionally, a specific keyword can be used to provide a third possible
>>>> output so that the parameters are dumped as #define-d macros, ready to
>>>> be saved to a header file and included in an eBPF-based project.
>>>>
>>>> The current patch introduces probing of two simple parameters:
>>>> availability of the bpf() system call, and kernel version. Later commits
>>>> will add other probes.
>>>>
>>>> Sample output:
>>>>
>>>>     # bpftool feature probe kernel
>>>>     Scanning system call and kernel version...
>>>>     Kernel release is 4.19.0
>>>>     bpf() syscall is available
>>>>
>>>>     # bpftool --json --pretty feature probe kernel
>>>>     {
>>>>         "syscall_config": {
>>>>             "kernel_version_code": 267008,
>>>>             "have_bpf_syscall": true
>>>>         }
>>>>     }
>>>>
>>>>     # bpftool feature probe kernel macros prefix BPFTOOL_
>>>>     /*** System call and kernel version ***/
>>>>     #define BPFTOOL_LINUX_VERSION_CODE 267008
>>>>     #define BPFTOOL_BPF_SYSCALL
>>>>
>>>> The optional "kernel" keyword enforces probing of the current system,
>>>> which is the only possible behaviour at this stage. It can be safely
>>>> omitted.
>>>>
>>>> The feature comes with the relevant man page, but bash completion will
>>>> come in a dedicated commit.
>>>>
>>>> Signed-off-by: Quentin Monnet <quentin.monnet@...ronome.com>
>>>> Reviewed-by: Jakub Kicinski <jakub.kicinski@...ronome.com>
>>>> ---
>>
>>>
>>> [..]
>>>
>>>> +		printf("#define %s%s%s\n", define_prefix,
>>>> +		       res ? "" : "NO_", define_name);
>>>
>>> Should we keep it autoconf style and do:
>>> #define XYZ 1 - in case of supported feature
>>> /* #undef XYZ */ - in case of unsupported feature
>>>
>>> ?
>>
>> But then if you include this as a header, you have no way to distinguish
>> the case when the feature is not supported from when bpftool did not
>> attempt to run the probe at all?
> How do you expect to exercise that knowledge? Something like the following?
> 
> #ifdef FEAT_X
> /* we know X is present, use it */
> #else
> # ifdef NO_FEAT_X
> /* we know X is not there, fall back to something else or let the user
>  * know we depend on it
>  */
> # else
> /* we don't know whether the feature is there or not,
>  * what are we supposed to do?
>  *
>  * isn't it essentially the same as 'ifdef FEAT_X'?
>  * we try to use the feature anyway here, I suppose?
>  */
> # endif
> #endif
> 
> My thinking of using that was something like the following (in a simple
> autoconf like fashion):
> #ifndef FEAT_X
>   /* error or fallback to something else */
> #endif
> /* use feature (or whatever fallback we've set up in the previous ifdef)
>  */

But then with autoconf you're supposed to probe everything that you need
to know to compile your program, right? I don't believe there would be a
case where some random features are not tested at all by the script...

In your above example, the "#ifndef FEAT_X /* error */ #endif", you do
return an error if the feature has not been probed instead of trying
anyway to see if it's there, as you mentioned above that... Did I miss
something? Did you mean something like "#ifdef FEAT_NO_X /* error */
#endif"? I believe the fact we reliably found that the feature is not
present _is_ an important information.

> 
> My worry is that we just export too much and it's hard to use.

I understand and I'm not opposed to changing my output. I'd like to
avoid loosing information (and keep the same amount of info in JSON and
#define outputs).

> 
>>
>>>> +	else
>>>> +		printf("%s is %savailable\n", plain_name, res ? "" : "NOT ");
>>>
>>> Why not do printf("%s %s\n", feat_name, res ? "yes" : "no") instead?
>>> And not complicate (drop) the output with human readability. One
>>> possible (dis)advantage - scripts can use this.
>>
>> I've been pondering about the interest of keeping human-readable output.
>> I think it helps users understand the output, especially for the procfs
>> parameters for example.
>>
>> As for scripts, they can and should stick to JSON. Plain output from
>> bpftool is not meant to be reliable for scripting.
> Makes sense, if you think that it provides more info than just rephrased
> json field name, then go for it :-)

Currently, a bit more descriptive info for the procfs parameters. And
the kernel version is in human-readable form (ok we don't care much).
But it's true it does not bring much. Mostly I thought of it as a nicer
output for someone who wants to probe the system just to have a quick
look at what's supported, it feels easier to interpret than JSON or
#defines.

Powered by blists - more mailing lists