Re: [PATCH] trace: Add trace any kernel object

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 15 Oct 2021 01:35:05 +0800
Huan Xie <xiehuan09@gmail.com> wrote:

> > That said, we are going to have to work with you to come up with a
> > better and more flexible interface, and make sure locking and
> > synchronization works.  
> 
> Thank you very much for your help .I am not currently a full-time
> Linux kernel developer, and hope to work hard to submit
> more quality kernel patches.I view the kernel code almost every day.
> However, few patches have been submitted so far:).
> I have viewed the ftrace source code for about half a year and am very
> interested in and watched all the videos about ftrace
> you published on the internet.

It's good to hear people are still working on the kernel as a "hobbyist".
That's basically how I started.


> > Honestly, I don't think this should be implemented by a file. What
> > could work, and Masami let me know your thoughts, is to add something
> > to the kprobe/uprobe/eprobe interface. That is:
> >
> >  # echo 'p bio_add_page arg1=$arg1:trace' > kprobe_events  
> 
> Great, this one looks more advanced.
> 
> # echo 'p bio_add_page arg1=$arg1:trace' > kprobe_events

Right, I think this is the way I would like to go. But as Masami is the
maintainer of kprobes, he has final say.

> >
> > Or something, that we explicitly set on the kprobe itself, and then we
> > can pick what we want to trace, especially if we only want to trace
> > one item in the list.  
> 
> If we only want to trace one function or pick what we want to trace, we can
> set the file set_ftrace_filter in my understanding.

Sorta ;-)

The way you have the code right now, no that is not the case. But it can be
fixed by attaching it to the hash of the trace array. I'll have to help you
doing that because it is not very trivial.

I should add helpers to make it easier.


> > > +void set_trace_object(void *obj)
> > > +{
> > > +     int i;
> > > +
> > > +     if (!obj || global_trace_count == MAX_TRACE_OBJ_NUM)
> > > +             goto out;
> > > +
> > > +     for (i = 0; i < global_trace_count; i++) {
> > > +             if (global_trace_obj[i] == (unsigned long)obj)
> > > +                     goto out;
> > > +     }
> > > +     mutex_lock(&object_mutex_lock);  
> >
> > As stated above, this can be called from atomic context, and you can't
> > have a mutex here.  
> 
> I'm so sorry, I ignored it.

No problem. This is a learning process. You will likely still need a lock,
but it just can not be a mutex. A raw_spin_lock() (or RCU) will be more
appropriate. Again, I can help you with this.

> 
> >  
> > > +
> > > +     global_trace_obj[global_trace_count++] = (unsigned long)obj;
> > > +
> > > +     mutex_unlock(&object_mutex_lock);
> > > +out:
> > > +     return;
> > > +}
> > > +

-- Steve