lkml.org 
[lkml]   [2022]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v2] bpf/scripts: Generate GCC compatible helpers
    From
    On 12/07/2022 05:40, Andrii Nakryiko wrote:
    > CC Quentin as well
    >
    > On Mon, Jul 11, 2022 at 5:11 PM James Hilliard
    > <james.hilliard1@gmail.com> wrote:
    >>
    >> On Mon, Jul 11, 2022 at 5:36 PM Yonghong Song <yhs@fb.com> wrote:
    >>>
    >>>
    >>>
    >>> On 7/6/22 10:28 AM, James Hilliard wrote:
    >>>> The current bpf_helper_defs.h helpers are llvm specific and don't work
    >>>> correctly with gcc.
    >>>>
    >>>> GCC appears to required kernel helper funcs to have the following
    >>>> attribute set: __attribute__((kernel_helper(NUM)))
    >>>>
    >>>> Generate gcc compatible headers based on the format in bpf-helpers.h.
    >>>>
    >>>> This adds conditional blocks for GCC while leaving clang codepaths
    >>>> unchanged, for example:
    >>>> #if __GNUC__ && !__clang__
    >>>> void *bpf_map_lookup_elem(void *map, const void *key) __attribute__((kernel_helper(1)));
    >>>> #else
    >>>> static void *(*bpf_map_lookup_elem)(void *map, const void *key) = (void *) 1;
    >>>> #endif
    >>>
    >>> It does look like that gcc kernel_helper attribute is better than
    >>> '(void *) 1' style. The original clang uses '(void *) 1' style is
    >>> just for simplicity.
    >>
    >> Isn't the original style going to be needed for backwards compatibility with
    >> older clang versions for a while?
    >
    > I'm curious, is there any added benefit to having this special
    > kernel_helper attribute vs what we did in Clang for a long time? Did
    > GCC do it just to be different and require workarounds like this or
    > there was some technical benefit to this?
    >
    > This duplication of definitions with #if for each one looks really
    > awful, IMO. I'd rather have a macro invocation like below (or
    > something along those lines) for each helper:
    >
    > BPF_HELPER_DEF(2, void *, bpf_map_update_elem, void *map, const void
    > *key, const void *value, __u64 flags);
    >
    > And then define BPF_HELPER_DEF() once based on whether it's Clang or GCC.

    Hi, for what it's worth I agree with Andrii, I would rather avoid the
    #if/else/endif and dual definition for each helper in the header, using
    a macro should keep it more readable indeed. The existing one
    (BPF_HELPER(return_type, name, args, id)) can likely be adapted.

    Also I note that contrarily to clang's helpers, you don't declare GCC's
    as "static" (although I'm not sure of the effect of declaring them
    static in this case).

    Thanks,
    Quentin

    \
     
     \ /
      Last update: 2022-07-12 11:49    [W:5.187 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site