netdev - Re: [PATCH v2 bpf-next 4/8] tracepoint: compute num

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2559d7cb-ec60-1200-2362-04fa34fd02bb@fb.com>
Date:   Wed, 21 Mar 2018 15:05:46 -0700
From:   Alexei Starovoitov <ast@...com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
CC:     David Miller <davem@...emloft.net>,
        Daniel Borkmann <daniel@...earbox.net>,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Network Development <netdev@...r.kernel.org>,
        kernel-team <kernel-team@...com>,
        Linux API <linux-api@...r.kernel.org>
Subject: Re: [PATCH v2 bpf-next 4/8] tracepoint: compute num_args at build
 time

On 3/21/18 12:44 PM, Linus Torvalds wrote:
> On Wed, Mar 21, 2018 at 11:54 AM, Alexei Starovoitov <ast@...com> wrote:
>>
>> add fancy macro to compute number of arguments passed into tracepoint
>> at compile time and store it as part of 'struct tracepoint'.
>
> We should probably do this __COUNT() thing in some generic header, we
> just talked last week about another use case entirely.

ok. Not sure which generic header though.
Should I move it to include/linux/kernel.h ?

> And wouldn't it be nice to just have some generic infrastructure like this:
>
>     /*
>      * This counts to ten.
>      *
>      * Any more than that, and we'd need to take off our shoes
>      */
>     #define __GET_COUNT(_0,_1,_2,_3,_4,_5,_6,_7,_8,_9,_10,_n,...) _n
>     #define __COUNT(...) \
>         __GET_COUNT(__VA_ARGS__,10,9,8,7,6,5,4,3,2,1,0)
>     #define COUNT(...) __COUNT(dummy,##__VA_ARGS__)

since it will be a build time error, it's a good time to discuss
how many arguments we want to support in tracepoints and
in general in other places that would want to use this macro.

Like the only reason my patch is counting till 17 is because of
trace_iwlwifi_dev_ucode_error().
The next offenders are using 12 arguments:
trace_mc_event()
trace_mm_vmscan_lru_shrink_inactive()

Clearly not every efficient usage of it:
         trace_mm_vmscan_lru_shrink_inactive(pgdat->node_id,
                         nr_scanned, nr_reclaimed,
                         stat.nr_dirty,  stat.nr_writeback,
                         stat.nr_congested, stat.nr_immediate,
                         stat.nr_activate, stat.nr_ref_keep,
                         stat.nr_unmap_fail,
                         sc->priority, file);
could have passed &stat instead.

I'd like to refactor that trace_iwlwifi_dev_ucode_error()
and from now on set the limit to 12.
Any offenders should be using tracepoints with <= 12 args
instead of extending the macro.
Does it sound reasonable ?

>     #define __CONCAT(a,b) a##b
>     #define __CONCATENATE(a,b) __CONCAT(a,b)
>
> and then you can do things like:
>
>     #define fn(...) __CONCATENATE(fn,COUNT(__VA_ARGS__))(__VA_ARGS__)
>
> which turns "fn(x,y,z..)" into "fn<N>(x,y,z)".
>
> That can be useful for things like "max(a,b,c,d)" expanding to
> "max4()", and then you can just have the trivial
>
>   #define max3(a,b,c) max2(a,max2(b.c))

I can try that. Not sure my macro-fu is up to that level.
__CAST_TO_U64() macro from the next patch was difficult to make
work across compilers and architectures.