Re: [PATCH v4 bpf-next 2/4] ftrace: allow IPMODIFY and DIRECT ops on the same function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, 19 Jul 2022 22:57:52 +0000
Song Liu <songliubraving@xxxxxx> wrote:

> >> +	hash = ops->func_hash->filter_hash;
> >> +	size = 1 << hash->size_bits;
> >> +	for (i = 0; i < size; i++) {
> >> +		hlist_for_each_entry(entry, &hash->buckets[i], hlist) {
> >> +			unsigned long ip = entry->ip;
> >> +			bool found_op = false;
> >> +
> >> +			mutex_lock(&ftrace_lock);
> >> +			do_for_each_ftrace_op(op, ftrace_ops_list) {
> >> +				if (!(op->flags & FTRACE_OPS_FL_DIRECT))
> >> +					continue;
> >> +				if (ops_references_ip(op, ip)) {
> >> +					found_op = true;
> >> +					break;  
> > 
> > I think you want a goto here. The macros "do_for_each_ftrace_op() { .. }
> > while_for_each_ftrace_op()" is a double loop. The break just moves to the
> > next set of pages and does not break out of the outer loop.  
> 
> Hmmm... really? I didn't see it ...
> 
> 
> #define do_for_each_ftrace_op(op, list)                 \
>         op = rcu_dereference_raw_check(list);                   \
>         do
> 
> #define while_for_each_ftrace_op(op)                            \
>         while (likely(op = rcu_dereference_raw_check((op)->next)) &&    \
>                unlikely((op) != &ftrace_list_end))
> 
> Did I miss something...?

Bah, you're right. I was confusing it with do_for_each_ftrace_rec(), which
*is* a double loop.

Never mind ;-)

> 
> > 
> > 					goto out_loop;
> >   
> >> +				}
> >> +			} while_for_each_ftrace_op(op);  
> 
> [...]
> 
> >   
> >> 	mutex_lock(&ftrace_lock);
> >> 
> >> 	ret = ftrace_startup(ops, 0);
> >> 
> >> 	mutex_unlock(&ftrace_lock);
> >> 
> >> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
> >> +	if (direct_mutex_locked)
> >> +		mutex_unlock(&direct_mutex);
> >> +#endif  
> > 
> > Change this to:
> > 
> > out_unlock:
> > 	mutex_unlock(&direct_mutex);
> >   
> 
> We still need #ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS, as 
> direct_mutex is not defined without that config. 

Ah, right. I just meant to get rid of the if statement.

To keep the code clean, perhaps we should have:

#ifdef CONFIG_DYNAMIC_FTRACE_WITH_DIRECT_CALLS
# define lock_direct_mutex()	mutex_lock(&direct_mutex)
# define unlock_direct_mutex()	mutex_unlock(&direct_mutex)
#else
# define lock_direct_mutex()	do { } while (0)
# define unlock_direct_mutex()	do { } while (0)
#endif

And use that here.

-- Steve



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux