lkml.org 
[lkml]   [2014]   [Jun]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH ftrace/core 2/2] ftrace, kprobes: Support IPMODIFY flag to find IP modify conflict
(2014/06/12 14:38), Namhyung Kim wrote:
> Hi Masami,
>
> On Thu, 12 Jun 2014 12:29:09 +0900, Masami Hiramatsu wrote:
>> (2014/06/11 16:41), Namhyung Kim wrote:
>>> Hi Masami,
>>>
>>> On Wed, 11 Jun 2014 10:28:01 +0900, Masami Hiramatsu wrote:
>>>> (2014/06/10 22:53), Namhyung Kim wrote:
>>>>> Hi Masami,
>>>>>
>>>>> 2014-06-10 (화), 10:50 +0000, Masami Hiramatsu:
>>>>>> Introduce FTRACE_OPS_FL_IPMODIFY to avoid conflict among
>>>>>> + /* Update rec->flags */
>>>>>> + do_for_each_ftrace_rec(pg, rec) {
>>>>>> + /* We need to update only differences of filter_hash */
>>>>>> + in_old = !old_hash || ftrace_lookup_ip(old_hash, rec->ip);
>>>>>> + in_new = !new_hash || ftrace_lookup_ip(new_hash, rec->ip);
>>>>>
>>>>> Why not use ftrace_hash_empty() here instead of checking NULL?
>>>>
>>>> Ah, a trick is here. Since an empty filter_hash must hit all, we can not
>>>> enable/disable filter_hash if we use ftrace_hash_empty() here.
>>>>
>>>> To enabling the new_hash, old_hash must be EMPTY_HASH which means in_old
>>>> always be false. To disabling, new_hash is EMPTY_HASH too.
>>>> Please see ftrace_hash_ipmodify_enable/disable/update().
>>>
>>> I'm confused. 8-p I guess what you want to do is checking records in
>>> either of the filter_hash, right? If so, what about this?
>>>
>>> in_old = !ftrace_hash_empty(old_hash) && ftrace_lookup_ip(old_hash, rec->ip);
>>> in_new = !ftrace_hash_empty(new_hash) && ftrace_lookup_ip(new_hash, rec->ip);
>>
>> NO, ftrace_lookup_ip() returns NULL if the hash is empty, so adding
>> !ftrace_hash_empty() is meaningless :)
>
> Ah, you're right!
>
>>
>> Actually, here I intended to have 3 meanings for the new/old_hash arguments,
>> - If it is NULL, it hits all
>> - If it is EMPTY_HASH, it hits nothing
>> - If it has some entries, it hits those entries.
>>
>> And in ftrace.c(__ftrace_hash_rec_update), AFAICS, ops->filter_hash has only
>> 2 meanings,
>> - If it is EMPTY_HASH or NULL, it hits all
>> - If it has some entries, it hits those entries.
>>
>> So I had to do above change...
>
> Then I propose to use a different value/symbol instead of EMPTY_HASH in
> order to prevent future confusion and add some comments there.

I doubt I need another symbol since the EMPTY_HASH means normally empty
and no hit(filter_hash case is a special one). I'd like to add a comment on it.


> [SNIP]
>>>>>> +static int ftrace_hash_ipmodify_enable(struct ftrace_ops *ops)
>>>>>> +{
>>>>>> + struct ftrace_hash *hash = ops->filter_hash;
>>>>>> +
>>>>>> + if (ftrace_hash_empty(hash))
>>>>>> + hash = NULL;
>>>>>> +
>>>>>> + return __ftrace_hash_update_ipmodify(ops, EMPTY_HASH, hash);
>>>>>> +}
>>>>>
>>>>> Please see above comment. You can pass an empty hash as is, or pass
>>>>> NULL as second arg. The same goes to below...
>>>>
>>>> As I said above, that is by design :). EMPTY_HASH means it hits nothing,
>>>> NULL means it hits all.
>>>
>>> But doesn't it make unrelated records also get the flag updated? I'm
>>> curious when new_hash can be empty on _enable() case..
>>
>> NO, _enable() is called right before ftrace_hash_rec_enable(ops,1) which
>> always enables filter_hash (since the 2nd arg is 1). If the filter_hash
>> is empty, ftrace_hash_rec_enable() enables ftrace_ops on all ftrace_recs.
>
> But AFAICS both of kprobes and kpatch call ftrace_set_filter_ip() before
> calling register_ftrace_function(). That means there's no case when
> ops->filter_hash can be empty, right?

No, unless it is registered, FTRACE_OPS_FL_ENABLED flag is not set on the
ftrace_ops. In that case, recs are not updated. :)

Thank you,



--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com


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

\
 
 \ /
  Last update: 2014-06-12 08:41    [W:0.066 / U:8.152 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site