On Tue, 23 Oct 2018, Qing Zhao wrote: > > > On Oct 23, 2018, at 4:11 AM, Miroslav Benes <mbenes@xxxxxxx> wrote: > >> > >> One question here, what’s the major benefit to prepare the patches manually? > > > > I could almost quote what you wrote below. It is a C file, easy to review > > and maintain. You have everything "under control". It allows to implement > > tricky hacks easily by hand if needed. > > Okay, I see. > > another question here: > > From my understanding of the live patching creation from Nicolai’s email, the patch includes: > > 1. initial patched functions; > 2. all the callers of any patched function if it’s been inlined/cloned/optimized; > 3. recursively copy any needed cpp macro, type, or > functions definition and add references to data objects with static > storage duration. > > during review and maintain procedure, are all the above 3 need to be reviewed and maintained? Either I do not understand, or this is mainly a process question. It is really up to you if you want to review it or not. Possibility is the imporant word here. Source based approach (which I originally replied to and confused it with manual preparation) allows you to do it. > >>> > >>> So let me ask, what is your motivation behind this? Is there a real > >>> problem you're trying to solve? It may have been mentioned somewhere and I > >>> missed it. > >> > >> the major functionality we want is: to Only enable static inlining for live patching for one > >> of our internal customers. the major purpose is to control the patch code size explosion and > >> debugging complexity due to too much inlining of global functions for the specific application. > > > > I hoped for more details, but ok. > at this time, this is the details I have. I can ask more if more details are needed. > > > > >> therefore, I proposed the multiple level of control for -flive-patching to satisfy multiple request from > >> different users. > >> > >> So far, from the feedback, I see that among the 4 levels of control, none, only-inline-static, inline, > >> and inline-clone, “none” and “inline” are NOT needed at all. > >> > >> however, -flive-patching = [only-inline-static | inline-clone] are necessary. > >> > >>> > >>>> > >>>> 3. Details of the proposal: > >>> > >>> This sounds awfully complicated. Especially when there is a dumping option > >>> in GCC thanks to Martin. What information do you miss there? We could > >>> improve the analysis tool. So far, it has given us all the info we need. > >> > >> Yes, it’s TRUE that the tool Martin wrote should serve the same purpose. nothing new from this > >> new GCC option -flive-patch-list compared to Martin’s tool. > >> > >> However, by simply adding this new GCC’s option, we can simplify the whole procedure for helping > >> live-patching. by only running GCC with the new added options once, we can get the impacted function list > >> at the same time. No need to run another tool anymore. > > > > I probably do not understand completely. I thought that using the > > option you would "preprocess" everything during the kernel build and then > > you'd need a tool to get the impacted function list for a given function. > > In that case, Martin's work is more than sufficient. > > > > Now I think you meant to run GCC with a given function, build everything > > and the list. Iteratively for every to-be-patched function. It does not > > sound better to me. > > there might be misunderstanding among us for this part. Let me explain my understanding first: > > 1. with martin’s tool, there are two steps to get the impacted function list for patched functions: > > Step1, build kernel with GCC with -fdump-ipa-clones + a bunch of options to disable bunch of ipa optimizations. ; > Step2, using the tool kgraft-ipa-analysis.py to analyze the dumped file from step1 to report the impacted function list. > > 2. with the new functionality of the GCC proposed in this proposal, -flive-patching -flive-patch-list > > Step1, build kernel with GCC with -flive-patching -flive-patch-list > > then gcc will automatically disable the unsafe ipa optimizations and report the impacted function list with the safe ipa optimizations. > > compare 1 and 2, I think that 2 is better and much more convenient than 1. another benefit from 2 is: > > if later we want more ipa optimization to be On for live-patching for the runtime performance purpose, we can expand it easily to include those > ipa optimization and at the same time report the additional impacted function list with the new ipa optimizations. > > however, for 1, this will be not easy to be extended. > > do I miss anything here? No, thanks for clearing it up. > > > >> this is the major benefit from this new option. > >> > >> anyway, if most of the people think that this new option is not necessary, I am fine to delete it. > >>> > >>> In the end, I'd be more than happy with what has been proposed in this > >>> thread by the others. To have a way to guarantee that GCC would not apply > >>> an optimization that could potentially destroy our effort to livepatch a > >>> running system. > >> > >> So, the major functionality you want from GCC is: > >> > >> -flive-patching=inline-clone > >> > >> Only enable inlining and all optimizations that internally create clone, > >> for example, cloning, ipa-sra, partial inlining, etc; disable all > >> other IPA optimizations/analyses. > >> > >> As a result, when patching a routine, all its callers and its clones’ > >> callers need to be patched as well. > >> > >> ? > > > > I cannot decide that. Perhaps. Josh called this hypothetical option > > -fpreserve-function-abi, which seems self-explaining to me. > > in the option -fpreserve-function-abi, will inlining and cloning are enabled by default? I think it more or less corresponds to your inline-clone. Miroslav
