Message-ID: <52F1C4C3.2070404@hitachi.com>
Date:	Wed, 05 Feb 2014 13:57:39 +0900
From:	Masami Hiramatsu <masami.hiramatsu.pt@...achi.com>
To:	Chen Gang <gang.chen.5i5j@...il.com>
Cc:	ananth@...ibm.com, anil.s.keshavamurthy@...el.com,
	Håvard Skinnemoen 
	<hskinnemoen@...il.com>, David Miller <davem@...emloft.net>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Hans-Christian Egtvedt <egtvedt@...fundet.no>,
	"yrl.pp-manager.tt@...achi.com" <yrl.pp-manager.tt@...achi.com>,
	Ingo Molnar <mingo@...e.hu>
Subject: Re: [PATCH] kernel: kprobe: move all *kretprobe* generic implementation
 to CONFIG_KRETPROBES enabled area

(2014/02/05 12:08), Chen Gang wrote:
>>>>>>>> Anyway, I don't think those inlined functions to be changed, because
>>>>>>>> most of them are internal functions. If CONFIG_KRETPROBES=n, it just
>>>>>>>> be ignored.
>>>>>>>>
>>>>>>>
>>>>>>> In original implementation, if CONFIG_KRETPROBES=n, kretprobe_assert(),
>>>>>>> disable_kretprobe(), and enable_kretprobe() are not ignored.
>>>>>>
>>>>>> Really? where are they called? I mean, those functions do not have
>>>>>> any instance unless your module uses it (but that is not what the kernel
>>>>>> itself should help).
>>>>>>
>>>>>
>>>>> If what you said correct (I guess so), for me, we still need let them in
>>>>> CONFIG_KRETPROBES area, and without any dummy outside, just like another
>>>>> *kprobe* static inline functions have done in "include/linux/kprobes.h".
>>>>
>>>> kretprobe_assert() is only for the internal check. So we don't need to care
>>>> about, and disable/enable_kretprobe() are anyway returns -EINVAL because
>>>> kretprobe can not be registered. And all of them are inlined functions.
>>>> In that case, we don't need to care about it.
>>>
>>> Hmm... it is related with code 'consistency':
>>>
>>>  - these static inline functions are kretprobe generic implementation,
>>>    and we are trying to let all kretprobe generic implementation within
>>>    CONFIG_KRETPROBES area.
>>
>> No, actually, kretprobe is just built on the kprobe. enable/disable_kretprobe
>> just wrapped the kprobe methods. And kretprobe_assert() is just for kretprobe
>> internal use. that is not an API. Moving only the kretprobe_assert() into the
>> CONFIG_KRETPROBE area is not bad, but since it is just a static inline function,
>> if there is no caller, it just be ignored, no side effect.
>>
> 
> OK, I can understand.
> 
> And do you mean enable/disable_kretprobe() are API? if so, we have to
> implement them whether CONFIG_KRETPROBES enabled or disabled.
> 
> And when CONFIG_KRETPROBES=n, just like what you originally said: we
> need returns -EINVAL directly (either, I am not quite sure whether the
> input parameter will be NULL, in this case).

Both are API, and when implementing it I had also considered that, but
I decided to stay it in inline-function wrapper. The reason why is,
that enable/disable_k*probe require the registered k*probes. If the
kernel hacker uses those functions, they must ensure registering his
k*probes, otherwise it does not work correctly. If the CONFIG_KRETPROBES=n,
register_kretprobe() always fails, this means that the code has
no chance to call those functions (it must be).

>>>  - And original kprobe static inline functions have done like that,
>>>    in same header file, if no obvious reasons, we can try to follow.
>>
>> It is no reasons to follow that too. Please keep your patch simple as much
>> as possible.
>>
> 
> "keep our patch simple as much as posssible" sounds reasonable to me.
> After skip "include/linux/kprobe.h", our patch's subject (include
> comments) also need be changed (I will/should change it).
> 
> For me, "include/linux/kprobe.h" can also be improved, but it can be
> another patch for it (not only for kretprobe, but also for jpobe).

if that "improvement" means "simplify", it is acceptable. Now I don't like
ifdefs of CONFIG_KPROBES and dummy functions, since if CONFIG_KPROBES=n,
other kernel modules can also check the kconfig and decide what they do
(or don't).
Perhaps, what we've really needed is "just enough able to compile",
not the fully covered dummy APIs.

>>>> I just concerned that it is a waste of memory if there are useless kretprobe
>>>> related instances are built when CONFIG_KRETPROBES=n.
>>>>
>>>
>>> Yeah, that is also one of reason (3rd reason).
>>>
>>>
>>> And if necessary, please help check what we have done whether already
>>> "let all kretprobe generic implementation within CONFIG_KRETPROBES area"
>>> (exclude declaration, struct/union definition, and architecture
>>> implementation).
>>
>> As I commented, your changes in kernel/kprobes.c are good to me except
>> two functions. That's all what we need to fix :)
>>
> 
> I will send a patch for it (since subject changed, we need not mark
> "patch v2"), thanks.  :-)

OK, I'll review that.

Thank you!


-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@...achi.com


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists