Frederic Weisbecker wrote:
>> diff --git a/kernel/trace/trace.h b/kernel/trace/trace.h
>> index 206cb7d..65945eb 100644
>> --- a/kernel/trace/trace.h
>> +++ b/kernel/trace/trace.h
>> @@ -45,6 +45,8 @@ enum trace_type {
>>      TRACE_POWER,
>>      TRACE_BLK,
>>      TRACE_KSYM,
>> +    TRACE_KPROBE,
>> +    TRACE_KRETPROBE,
>>  
>>      __TRACE_LAST_TYPE,
>>  };
>> @@ -227,6 +229,22 @@ struct trace_ksym {
>>      char                    ksym_name[KSYM_NAME_LEN];
>>      char                    p_name[TASK_COMM_LEN];
>>  };
>> +#define TRACE_KPROBE_ARGS 6
>> +
>> +struct kprobe_trace_entry {
>> +    struct trace_entry      ent;
>> +    unsigned long           ip;
>> +    int                     nargs;
>> +    unsigned long           args[TRACE_KPROBE_ARGS];
> 
> 
> 
> I see that you actually make use of arg as a dynamic sizeable
> array.
> For clarity, args[TRACE_KPROBE_ARGS] could be args[0].
> 
> It's just a neat and wouldn't affect the code nor the data
> but would be clearer for readers of that code.

Hmm. In that case, I think we'll need a new macro for field
definition, like TRACE_FIELD_ZERO(type, item).

>> +};
>> +
>> +struct kretprobe_trace_entry {
>> +    struct trace_entry      ent;
>> +    unsigned long           func;
>> +    unsigned long           ret_ip;
>> +    int                     nargs;
>> +    unsigned long           args[TRACE_KPROBE_ARGS];
>> +};
> 
> 
> ditto
> 
> 
>   
>>  /*
>>   * trace_flag_type is an enumeration that holds different
>> @@ -344,6 +362,10 @@ extern void __ftrace_bad_type(void);
>>              IF_ASSIGN(var, ent, struct syscall_trace_exit,          \
>>                        TRACE_SYSCALL_EXIT);                          \
>>              IF_ASSIGN(var, ent, struct trace_ksym, TRACE_KSYM);     \
>> +            IF_ASSIGN(var, ent, struct kprobe_trace_entry,          \
>> +                      TRACE_KPROBE);                                \
>> +            IF_ASSIGN(var, ent, struct kretprobe_trace_entry,       \
>> +                      TRACE_KRETPROBE);                             \
>>              __ftrace_bad_type();                                    \
>>      } while (0)
>>  
>> diff --git a/kernel/trace/trace_event_types.h 
>> b/kernel/trace/trace_event_types.h
>> index 6db005e..ec2e6f3 100644
>> --- a/kernel/trace/trace_event_types.h
>> +++ b/kernel/trace/trace_event_types.h
>> @@ -175,4 +175,24 @@ TRACE_EVENT_FORMAT(kmem_free, TRACE_KMEM_FREE, 
>> kmemtrace_free_entry, ignore,
>>      TP_RAW_FMT("type:%u call_site:%lx ptr:%p")
>>  );
>>  
>> +TRACE_EVENT_FORMAT(kprobe, TRACE_KPROBE, kprobe_trace_entry, ignore,
>> +    TRACE_STRUCT(
>> +            TRACE_FIELD(unsigned long, ip, ip)
>> +            TRACE_FIELD(int, nargs, nargs)
>> +            TRACE_FIELD_SPECIAL(unsigned long args[TRACE_KPROBE_ARGS],
>> +                                args, TRACE_KPROBE_ARGS, args)
>> +    ),
>> +    TP_RAW_FMT("%08lx: args:0x%lx ...")
>> +);
>> +
>> +TRACE_EVENT_FORMAT(kretprobe, TRACE_KRETPROBE, kretprobe_trace_entry, 
>> ignore,
>> +    TRACE_STRUCT(
>> +            TRACE_FIELD(unsigned long, func, func)
>> +            TRACE_FIELD(unsigned long, ret_ip, ret_ip)
>> +            TRACE_FIELD(int, nargs, nargs)
>> +            TRACE_FIELD_SPECIAL(unsigned long args[TRACE_KPROBE_ARGS],
>> +                                args, TRACE_KPROBE_ARGS, args)
>> +    ),
>> +    TP_RAW_FMT("%08lx <- %08lx: args:0x%lx ...")
>> +);
>>  #undef TRACE_SYSTEM
>> diff --git a/kernel/trace/trace_kprobe.c b/kernel/trace/trace_kprobe.c
>> new file mode 100644
>> index 0000000..0951512
>> --- /dev/null
>> +++ b/kernel/trace/trace_kprobe.c
>> @@ -0,0 +1,1183 @@
>> +/*
>> + * kprobe based kernel tracer
>> + *
>> + * Created by Masami Hiramatsu <[email protected]>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/kprobes.h>
>> +#include <linux/seq_file.h>
>> +#include <linux/slab.h>
>> +#include <linux/smp.h>
>> +#include <linux/debugfs.h>
>> +#include <linux/types.h>
>> +#include <linux/string.h>
>> +#include <linux/ctype.h>
>> +#include <linux/ptrace.h>
>> +
>> +#include "trace.h"
>> +#include "trace_output.h"
>> +
>> +#define MAX_ARGSTR_LEN 63
>> +
>> +/* currently, trace_kprobe only supports X86. */
>> +
>> +struct fetch_func {
>> +    unsigned long (*func)(struct pt_regs *, void *);
>> +    void *data;
>> +};
>> +
>> +static __kprobes unsigned long call_fetch(struct fetch_func *f,
>> +                                      struct pt_regs *regs)
>> +{
>> +    return f->func(regs, f->data);
>> +}
>> +
>> +/* fetch handlers */
>> +static __kprobes unsigned long fetch_register(struct pt_regs *regs,
>> +                                          void *offset)
>> +{
>> +    return regs_get_register(regs, (unsigned)((unsigned long)offset));
>> +}
>> +
>> +static __kprobes unsigned long fetch_stack(struct pt_regs *regs,
>> +                                       void *num)
>> +{
>> +    return regs_get_kernel_stack_nth(regs, (unsigned)((unsigned long)num));
> 
> 
> You seem to often use unsigned as an implicit type.
> Would be better to explicitly use unsigned int.

Agreed.

> Anyway, I guess we can merge it in -tip and give it a try.

Thank you!


-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America), Inc.
Software Solutions Division

e-mail: [email protected]

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to