[v2,bpf-next,4/8] tracepoint: compute num_args at build time

From: Alexei Starovoitov <ast@kernel.org>

add fancy macro to compute number of arguments passed into tracepoint
at compile time and store it as part of 'struct tracepoint'.
The number is necessary to check safety of bpf program access that
is coming in subsequent patch.

for_each_tracepoint_range() api has no users inside the kernel.
Make it more useful with ability to stop for_each() loop depending
via callback return value.
In such form it's used in subsequent patch.

Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/tracepoint-defs.h |  1 +
 include/linux/tracepoint.h      | 32 +++++++++++++++++++++++---------
 include/trace/define_trace.h    | 14 +++++++-------
 kernel/tracepoint.c             | 27 ++++++++++++++++-----------
 4 files changed, 47 insertions(+), 27 deletions(-)

On Wed, Mar 21, 2018 at 11:54 AM, Alexei Starovoitov <ast@fb.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.

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__)

    #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))

etc (with proper parentheses, of course).

And I'd rather not have that function name concatenation be part of
the counting logic, because we actually may have different ways of
counting, so the concatenation is separate.

In particular, the other situation this came up for, the counting was
in arguments _pairs_, so you'd use a "COUNT_PAIR()" instead of
"COUNT()".

NOTE NOTE NOTE! The above was slightly tested and then cut-and-pasted.
I might have screwed up at any point. Think of it as pseudo-code.

         Linus

On 3/21/18 12:44 PM, Linus Torvalds wrote:
> On Wed, Mar 21, 2018 at 11:54 AM, Alexei Starovoitov <ast@fb.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.

On Wed, 21 Mar 2018 15:05:46 -0700
Alexei Starovoitov <ast@fb.com> wrote:

> 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.

Yes they should have, and if I was on the Cc for that patch, I would
have yelled at them and told them that's exactly what they needed to do.

Perhaps I should add something to keep any tracepoint from having more
than 6 arguments. That should force a clean up quickly.

I think I may start doing that.

-- Steve

On 3/22/18 6:36 AM, Steven Rostedt wrote:
> On Wed, 21 Mar 2018 15:05:46 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
>
>> 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.
>
> Yes they should have, and if I was on the Cc for that patch, I would
> have yelled at them and told them that's exactly what they needed to do.
>
> Perhaps I should add something to keep any tracepoint from having more
> than 6 arguments. That should force a clean up quickly.

I was hesitant to do anything about iwlwifi_dev_ucode_error's 17 args,
because when the code is in such shape there are likely more
skeletons in the closet.
Turned out 'struct iwl_error_event_table' is defined twice with subtle
different field names and layout. While the same
trace_iwlwifi_dev_ucode_error() is used in what looks like two
different cases. I think I managed to refactor it from 17 args to 4
while keeping all bugs in place, but it really should be a job of
the author of the code to deal with such oddities.
Will send the 'fix' in the next respin.
So I definitely support the idea of build time warn for large
number of args.

On Thu, 22 Mar 2018 08:51:16 -0700
Alexei Starovoitov <ast@fb.com> wrote:

> So I definitely support the idea of build time warn for large
> number of args.

I'm more for a build time error for large number of args.

-- Steve