This is the mail archive of the systemtap@sourceware.org mailing list for the systemtap project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [Patch 2/5][Djprobe]Djprobe Coexist with Kprobes


Hi, Anil

Keshavamurthy Anil S wrote:
> On Thu, Sep 29, 2005 at 05:59:31AM -0700, Masami Hiramatsu wrote:
>
> 	I see your patch has lots of line wrap and hard to review.

Would you say that my patch has wrong indentation? Or would the
way to separate the patch be wrong?
I would like to correct wrong points.

> With great difficulty, I have reviewed your code,
> please see my comments below.

Thanks a lot!

> Main highlights:
> 1) You code does not work if a new CPU comes online or existing cpu goes offline

I have no machine which can plug/unplug CPUs. So I can not test
that feature. But I will try to develop a patch that djprobe can
work with CPU-Hotplug.
Would you have that machine? If I develop a patch, would you help
me to test?

> 2) suggestion: Create djprobe.c and djprobe.h files instead of pushing them into
existing files

Djprobe uses kprobes' internal function (__get_insn_slot()) to
allocate stub code buffers. And Kprobe has to check whether the
code at the insertion address was already modified by djprobe.
So we need to apply some patches to kprobes.c.
Additionally, I think the djprobe is one of the probes, so it
should be included in kprobes.c. This is easier to use for other
developers.

> 3) Use good hashing algorithm to search djprobe instances.

Hash_list is useful if a node has only one key value. Unfortunately,
a djprobe has a range of address as key value.
__get_djprobe_instance() function is used for searching overlapping
of address ranges. So hash_list is not useful for djprobe.
And, I think the hash list is not so efficient for performance
improvement for the djprobe. Because, the djprobe needs to search
its instance only when it registers probes and executes deferred
processing. It does NOT search when the probe was hit.

> 4)Handle the case where ARCH does not support djprobes transparently.

I think the transparency of djprobe means that the source code
which uses the djprobe does not need any modification when ARCH
does not support djprobes. From this point of view, current code
works transparently.

But current implementation is not so good. I will design it again.

> 5) optimize djprobe data struct

Exactly. I know it is not optimized enough. I think I can reduce
the size of djprobe data structure by moving some members of it
to arguments of register_djprobe() function.
What would you think about it?

> 6) Not convinced how you are atomically updating 5 Bytes (jmp inst )
guaranteeing that
> none of the other processor are executing near those address.

The half of the code which guarantees that none of the other
processor are executing near those address is included in the
other (arch-depend) patch. Djprobe makes a bypass route from
copy of original codes which will be replaced by jmp code. After
all processors finished executing the original codes (and it will
execute the bypass route next time) djprobe inserts jmp code. You
can see this detail in the answer of 8th Question in Q&A text
(Answer of Q:How does the djprobe guarantee no threads and no
processors are executing the modifying area?).

Best regards,
Thanks


-- 
Masami HIRAMATSU
2nd Research Dept.
Hitachi, Ltd., Systems Development Laboratory
E-mail: hiramatu@sdl.hitachi.co.jp


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]