lkml.org 
[lkml]   [2019]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Date
    SubjectRe: [PATCH 02/25] tracing: Improve "if" macro code generation
    On Wed, Mar 20, 2019 at 4:17 AM David Laight <David.Laight@aculab.com> wrote:
    >
    > > ______r = !!(cond); \
    >
    > Is that (or maybe just the !!) needed any more??

    It is, because the 'cond' expression might not be an int, it could be
    a test for a pointer being non-NULL, or an u64 being non-zero, and not
    having the "!!" would mean that you'd get a warning or drop bits when
    assigning to 'int'.

    And you do need the new temporary variable to avoid double evaluation
    the way that code is written.

    That said, I do think the code is really ugly. We could:

    - avoid the temporary by just simplifying things.

    - do the '!!' just once in the parent macro.

    - Steven has this crazy model of "more underscores are better". They
    aren't. They don't help if things nest anyway, but what does help is
    meaningful names. Both when things don't nest, and when looking at
    generated asm files.

    - ,, and finally, what _is_ better is to chop things up so that they
    are smaller and make each macro do only one thing

    So maybe do the patch something like the attached instead? Completely
    untested, but it looks sane to me.

    Linus
    include/linux/compiler.h | 35 ++++++++++++++++++-----------------
    1 file changed, 18 insertions(+), 17 deletions(-)

    diff --git a/include/linux/compiler.h b/include/linux/compiler.h
    index 445348facea9..8aaf7cd026b0 100644
    --- a/include/linux/compiler.h
    +++ b/include/linux/compiler.h
    @@ -53,23 +53,24 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val,
    * "Define 'is'", Bill Clinton
    * "Define 'if'", Steven Rostedt
    */
    -#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
    -#define __trace_if(cond) \
    - if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
    - ({ \
    - int ______r; \
    - static struct ftrace_branch_data \
    - __aligned(4) \
    - __section("_ftrace_branch") \
    - ______f = { \
    - .func = __func__, \
    - .file = __FILE__, \
    - .line = __LINE__, \
    - }; \
    - ______r = !!(cond); \
    - ______f.miss_hit[______r]++; \
    - ______r; \
    - }))
    +#define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) )
    +
    +#define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond))
    +
    +#define __trace_if_value(cond) ({ \
    + static struct ftrace_branch_data \
    + __aligned(4) \
    + __section("_ftrace_branch") \
    + __if_trace = { \
    + .func = __func__, \
    + .file = __FILE__, \
    + .line = __LINE__, \
    + }; \
    + (cond) ? \
    + (__if_trace.miss_hit[1]++,1) : \
    + (__if_trace.miss_hit[0]++,0); \
    +})
    +
    #endif /* CONFIG_PROFILE_ALL_BRANCHES */

    #else
    \
     
     \ /
      Last update: 2019-03-20 18:34    [W:4.783 / U:0.568 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site