> On Jul 18, 2022, at 6:19 AM, Petr Mladek <pmladek@xxxxxxxx> wrote: > > 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@xxxxxxxxx/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@xxxxxxxxx> >> >> 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; Yeah, we can actually do something like this. We can also move the ops->flags & FTRACE_OPS_FL_IPMODIFY check to register_ftrace_function(), so we only lock direct_mutex when when it is necessary. > > >> 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? Unfortunately, we cannot do this. ftrace code cannot look up bpf trampolines without locking direct_mutex. Thanks, Song
