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

[Keshavamurthy Anil S <anil dot s dot keshavamurthy at intel dot com>]

On Thu, Oct 06, 2005 at 11:50:24AM +0900, Masami Hiramatsu wrote:
> 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.

I guess it is to do with your mailer, I have seen something like this when
I used to send my mails from MS outlook and now I have moved to mutt and
don't have any problems.

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

Your are very welcome.

> 
> > 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?
To test this you don;t have to have a machine which can plug/unplug CPUs,
just enable CONFIG_HOTPLUG_CPU and when this kernel boots, you see
/sys/devices/system/cpu/cpuX/online file. echo'ing '0' onto this file
will offline the cpu and echo'ing '1' onto this file will bring the cpu
online there by affecting cpu_online_map on which you are depending on.

> 
> > 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.
I understand that you need some infrastructural changes to existing
Kprobes and I have no problems you modifying the existing kprobes to
accommodate djprobes, what I ment was the core djprobe logic can go
into separate file for better readability and for some architecture
which does not yet support djprobe can exclude djprobe.c file from compiling.

> 
> > 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.
Humm..This is not a excuse for not to use hashing algorithm :-)

> __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.
With the Djprobe, the number of probes that can
be inserted will move to the order of few thousands. And for every single
register or unregister djprobes call, searching this thousands of entries
is not at all an efficient way to implement something in the kernel.

Also, I see the same linear search happening inside work_check_djprobe_instance().
As I understand you are scheduling this function on all the cpus and inside this
function you are doing a linear search for djprobe instances that too holding
a spin lock and thus making other thread executing this function on different cpus to
wait untill you finish serial search on this cpu. Hence my suggestion to look into
optimizing this search.


> 
> > 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.
Okay, thanks.
> 
> > 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?
Yes, that would be better and you can store those 
extra arguments in the djprobe instance structure for later use.

> 
> > 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?).
I will look at this again and will get back to you later.

Thanks,
Anil Keshavamurthy