lkml.org 
[lkml]   [2022]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function
    On Mon 2022-07-18 15:42:25, kernel test robot wrote:
    > Hi Song,
    >
    > I love your patch! Perhaps something to improve:
    >
    > [auto build test WARNING on bpf-next/master]
    >
    > url: https://github.com/intel-lab-lkp/linux/commits/Song-Liu/ftrace-host-klp-and-bpf-trampoline-together/20220718-135652
    > base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
    > config: x86_64-randconfig-a004 (https://download.01.org/0day-ci/archive/20220718/202207181552.VuKfz9zg-lkp@intel.com/config)
    > compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
    > reproduce (this is a W=1 build):
    > # https://github.com/intel-lab-lkp/linux/commit/9ef1ec8cb818d8ca70887c8c123f2d579384a6c6
    > git remote add linux-review https://github.com/intel-lab-lkp/linux
    > git fetch --no-tags linux-review Song-Liu/ftrace-host-klp-and-bpf-trampoline-together/20220718-135652
    > git checkout 9ef1ec8cb818d8ca70887c8c123f2d579384a6c6
    > # save the config file
    > mkdir build_dir && cp config build_dir/.config
    > make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash kernel/trace/
    >
    > If you fix the issue, kindly add following tag where applicable
    > Reported-by: kernel test robot <lkp@intel.com>
    >
    > All warnings (new ones prefixed by >>):
    >
    > kernel/trace/ftrace.c: In function 'register_ftrace_function':
    > >> kernel/trace/ftrace.c:8197:14: warning: variable 'direct_mutex_locked' set but not used [-Wunused-but-set-variable]
    > 8197 | bool direct_mutex_locked = false;
    > | ^~~~~~~~~~~~~~~~~~~
    >
    >
    > vim +/direct_mutex_locked +8197 kernel/trace/ftrace.c
    >
    > 8182
    > 8183 /**
    > 8184 * register_ftrace_function - register a function for profiling
    > 8185 * @ops: ops structure that holds the function for profiling.
    > 8186 *
    > 8187 * Register a function to be called by all functions in the
    > 8188 * kernel.
    > 8189 *
    > 8190 * Note: @ops->func and all the functions it calls must be labeled
    > 8191 * with "notrace", otherwise it will go into a
    > 8192 * recursive loop.
    > 8193 */
    > 8194 int register_ftrace_function(struct ftrace_ops *ops)
    > 8195 __releases(&direct_mutex)
    > 8196 {
    > > 8197 bool direct_mutex_locked = false;
    > 8198 int ret;
    > 8199
    > 8200 ftrace_ops_init(ops);
    > 8201
    > 8202 ret = prepare_direct_functions_for_ipmodify(ops);
    > 8203 if (ret < 0)
    > 8204 return ret;
    > 8205 else if (ret == 1)
    > 8206 direct_mutex_locked = true;

    Honestly, this is another horrible trick. Would it be possible to
    call prepare_direct_functions_for_ipmodify() with direct_mutex
    already taken?

    I mean something like:

    mutex_lock(&direct_mutex);

    ret = prepare_direct_functions_for_ipmodify(ops);
    if (ret)
    goto out:

    mutex_lock(&ftrace_lock);
    ret = ftrace_startup(ops, 0);
    mutex_unlock(&ftrace_lock);

    out:
    mutex_unlock(&direct_mutex);
    return ret;


    > 8208 mutex_lock(&ftrace_lock);
    > 8209
    > 8210 ret = ftrace_startup(ops, 0);
    > 8211
    > 8212 mutex_unlock(&ftrace_lock);
    > 8213

    Would be possible to handle tr->mutex the same way to avoid
    the trylock? I mean to take it in advance before direct_mutex?

    Best Regards,
    Petr

    \
     
     \ /
      Last update: 2022-07-18 15:20    [W:4.178 / U:0.072 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site