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 core patch


Hi Anil,

Thank you for your advice.

Keshavamurthy@bambi.jf.intel.com wrote:
> Hi Masami,
> 	Sorry for the delayed response. My comments are inline.
> 
> On Thu, Oct 19, 2006 at 06:02:32PM +0900, Masami Hiramatsu wrote:
>> I also updated API reference.
>>
>> API Reference
>> -------------
>> The Djprobe API includes "register_djprobe" function,
>> "unregister_djprobe" function and "commit_djprobes" function.
>> Here are specifications for these functions and the
>> associated probe handlers.
> 
> I am curious as to why you are introducing a new interface
> for registering djprobes. Can't this be done by modifying the
> existing register_kprobe()/unregister_kprobe() and under the
> covers you can choose to implement djprobes. 

There were two reasons.

1) I thought that should be waste of memory. As you can see,
the djprobe structure doesn't have its kprobe nor instruction
buffer, because it is just an interface structure. Instead of
that, it has an pointer to the djprobe_instance structure which
is true instance of the djprobe. This "instance" has the kprobe
structure and instruction buffer. So, I could keep the djprobe
structure small.
2) The other reason is from difference of the usage between djprobe
and kprobe. Kprobe users have to ensure that the target address
points to the 1st byte of instruction. On the other hand, djprobe
users additionally have to count the length of the target
instructions, and ensure those instructions are relocatable.

Therefore, I provided those special interfaces.

If you never mind that the jump-based-kprobe needs two
kprobe structures per one probe point (one is for the interface,
another is for the instance -- for deferred releasing), I can
integrate these interfaces.

> The benifit of hiding djprobes under kprobes would be 
> 
> 1)Users of kernel probe need not have to maintain two
> version of their instrumentation code (one with kprobes 
> and one with djprobes).

Indeed.
(I think users have to prepare additional code which counts
 the length and checks whether the instructions are
 relocatable or not.)

> 2)If for some reason djprobes fails (might be  because there exists
> a kprobes in that djprobe address + size range), then the user of the
> probe interface has to try kprobes in order to succeed the registration.
> However if you choose to implement djprobes under the covers of kprobes, then you can
> transparently support the insertion of the probes, through aggregate kprobes instead
> of failing the djprobes.

I agree. This advantage is important.

> Also with this approach, today's systemtap can easily take advantage of djprobes,
> since djprobes is implemented under the kprobes implementation. Since you are targeting
> your code for mainline, don't worry about kabi of kprobes interface.

Thanks, I'll try it.

> Oaky, some more comments.
> 1) I did not see where you check for whether the 
> target instruction of the djprobe is relocatable.
> Are you currently assuming that the target instruction 
> is simply relocatable?

In this version, that check must be done in the user-space,
because parsing and analyzing CISC instructions are too hard
to be done in the kernel code.
Oshima-san is working for developing this check routine.

> 2) You are not fully populating the pt_regs which the probe handler receives,
> ( you have bogus values in eflags, eip, esp, etc) so I am not sure whether 
> this is a must for instrumentation code who might need these values.

OK, I'll fix that.

> [...]	
>> +int __kprobes djprobe_kprobe(struct kprobe *kp)
> Did not see where you are using this function.

This function is called from kernel/kprobe.c:register_kprobe()

>> +
>> +static __always_inline
>> +    struct djprobe_instance *__create_djprobe_instance(struct arch_djprobe_param
>> +						       *param)
>> +{
>> +	struct djprobe_instance *djpi;
>> +	void * addr = (void *)djprobe_param_address(param);
>> +	/* allocate a new instance */
>> +	djpi = kcalloc(1, sizeof(struct djprobe_instance), GFP_ATOMIC);
> I think you can use GFP_KERNEL.

Indeed.

>> +	if (djpi == NULL) {
>> +		goto out;
>> +	}
>> +
>> +	/* allocate stub */
>> +	djpi->stub.insn = __get_insn_slot(&djprobe_insn_pages);
>> +	if (djpi->stub.insn == NULL) {
>> +		__free_djprobe_instance(djpi);
> kfree(djpi) should do.

Thanks. I'll fix it.

> [...]
>> +
>> +	if ((ret = in_kprobes_functions((long)addr)) != 0)
>> +		return ret;
>> +
>> +	mutex_lock(&djprobe_mutex);
>> +
>> +	/* check confliction with other djprobes */
>> +	djpi = __get_djprobe_instance(addr, len);
>> +	if (djpi) {
>> +		if (djpi->kp.addr == addr) {
>> +			djp->inst = djpi;	/* add to another instance */
>> +			list_add_rcu(&djp->plist, &djpi->plist);
>> +		} else {
>> +			ret = -EEXIST;	/* other djprobes were inserted */
>> +		}
>> +		goto out;
>> +	}
>> +	/* check confliction with kprobes */
>> +	for (i = 1; i < len; i++) {
>> +		if (get_kprobe((void *)((long)addr + i))) {
>> +			ret = -EEXIST;	/* a kprobes were inserted */
>> +			goto out;
>> +		}
>> +	}
> You need similar check in the kprobes registration, as we don;t want
> to insert kprobes on the djprobe where you have jump target address.

I checked it in the kernel/kprobes.c as below:

> --- linux-2.6.19-rc1-mm1.orig/kernel/kprobes.c	2006-10-16 22:04:22.000000000 +0900
> +++ linux-2.6.19-rc1-mm1/kernel/kprobes.c	2006-10-16 22:06:22.000000000 +0900
[...]
> @@ -582,6 +583,16 @@
>  			probed_mod = NULL;
>  	}
> 
> +#ifdef CONFIG_DJPROBE
> +	if (!djprobe_kprobe(p)) {
> +		mutex_lock(&djprobe_mutex);
> +		if (__get_djprobe_instance(p->addr, 1) != NULL) {

This __get_djprobe_instance() returns the djprobe_instance which
partially or fully covers the specified region.

> +			mutex_unlock(&djprobe_mutex);
> +			return -EEXIST;
> +		}
> +	}
> +#endif /* CONFIG_DJPROBE */
> +


Best regards,

-- 
Masami HIRAMATSU
Linux Technology Center
Hitachi, Ltd., Systems Development Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com



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