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

[Masami Hiramatsu <hiramatu at sdl dot hitachi dot co dot jp>]

Hi, Anil

Keshavamurthy Anil S 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.

I and my co-workers saw my emails in the systemtap archive:
http://sourceware.org/ml/systemtap/2005-q3/msg00629.html
This message has no line-wrapping. So I think it is not caused by my mailer.
Please check the mails again.

>>>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.

Thank you for that information. I'll try it and I'd like to make the CPU-
hotplug support feature as an additional patch.

>>>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.

OK. I'll consider the possibility of that.

>>>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 :-)

Ok, I understand that the thousands of probes cause problem with linear search.
I misunderstood that scale. I will try to introduce a hash table.

>>__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.

OK, that is a problem. I have an idea of "per-probe work"s to solve it.
This will allocate a lot of works and insert it into the workqueues on each cpu.
Is this acceptable?

>>>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.

OK, I will do that.

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