(2014/11/27 19:52), Petr Mladek wrote:
> On Thu 2014-11-27 19:06:37, Masami Hiramatsu wrote:
>> (2014/11/27 0:27), Josh Poimboeuf wrote:
>>> On Wed, Nov 26, 2014 at 10:18:24AM +0100, Jiri Kosina wrote:
>>>> On Wed, 26 Nov 2014, Masami Hiramatsu wrote:
>>>>
>>>>>> Note to Steve:
>>>>>> Masami's IPMODIFY patch is heading for -next via your tree. Once it
>>>>>> arrives,
>>>>>> I'll rebase and make the change to set IPMODIFY. Do not pull this for
>>>>>> -next
>>>>>> yet. This version (v4) is for review and gathering acks.
>>>>>
>>>>> BTW, as we discussed IPMODIFY is an exclusive flag. So if we allocate
>>>>> ftrace_ops for each function in each patch, it could be conflict each
>>>>> other.
>>>>
>>>> Yup, this corresponds to what Petr brought up yesterday. There are cases
>>>> where all solutions (kpatch, kgraft, klp) would allocate multiple
>>>> ftrace_ops for a single function entry (think of patching one function
>>>> multiple times in a row).
>>>>
>>>> So it's not as easy as just setting the flag.
>>>>
>>>>> Maybe we need to have another ops hashtable to find such conflict and
>>>>> new handler to handle it.
>>>>
>>>> If I understand your proposal correctly, that would sound like a hackish
>>>> workaround, trying to basically trick the IPMODIFY flag semantics you just
>>>> implemented :)
>>>
>>> I think Masami may be proposing something similar to what we do in
>>> kpatch today. We have a single ftrace_ops and handler which is used for
>>> all functions. The handler accesses a global hash of kpatch_func
>>> structs which is indexed by the original function's IP address.
>>
>> Hmm, I think both is OK. kpatch method is less memory consuming and
>> will have a bigger overhead. However, as Steven talked at Plumbers Conf.,
>> he will introduce a direct code modifying interface for ftrace. After
>> that is introduced, we don't need to care about performance degradation
>> by patching :)
>
> Yup, I would prefer to have ftrace_ops per (original) function entry. I mean
> that new patches will reuse the existing ftrace_ops for already
> patched functions. They will just create new ftrace_ops from the
> not-yet-patched symbols.
However, too many ftrace_ops will get bigger overhead if conflicts
happened on any entry, since on such entry ftrace walks through
all registered ftrace_ops and checks its filter. It's a downside.
Perhaps, ftrace needs to have 2 different ftrace_ops lists, one
is for managing, and one is for walk through. And the latter list
drops the ftrace_ops which has trampoline and whose all filtered ip
is exclusively used (iow, such ftrace_ops never be hit in the walk
through).
> Using a single ftrace_ops everywhere would kill the win from
> Steven's direct ftrace optimization.
It depends on the implementation and interface. E.g. an explicit
path-change optimization interface for each address with ftrace_ops,
like as
ftrace_change_path_ip(ftrace_ops *ops, unsigned long old_addr,
unsigned long new_addr);
This checks ftrace_ops is already registered and has given old_addr
in filter, ensures no other ftrace_ops are registered on given address,
and then optimizes the fentry call to jump to the new_addr.
Anyway, at this point it is not a major discussion point, it's a
kind of minor implementation issue for performance and memory consuming.
We can switch it without changing API.
Thank you,
>>> It actually works out pretty well because it nicely encapsulates the
>>> knowledge about which functions are patched in a single place. And it
>>> makes it easy to track function versions (for incremental patching and
>>> rollback).
>>>
>>>> What I'd propose instead is to make sure that we always have
>>>> just a ftrace_ops per function entry, and only update the pointers there
>>>> as necessary. Fortunately we can do the switch atomically, by making use
>>>> of ->private.
>>>
>>> But how would you update multiple functions atomically, to enforce
>>> per-thread consistency?
>>
>> At this point, both can do it atomically. We just need an atomic flag
>> for applying patches.
>
> By other words, we would need something like the "kgr_immutable" flag from
> kGraft. It will make sure that everybody stays with the current code until
> all function entries are updated.
>
> Best Regards,
> Petr
--
Masami HIRAMATSU
Software Platform Research Dept. Linux Technology Research Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu...@hitachi.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
