(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 :) > 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. Thank you, -- Masami HIRAMATSU Software Platform Research Dept. Linux Technology Research Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: masami.hiramatsu.pt@xxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe live-patching" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
