On Mon, 22 Oct 2018, Qing Zhao wrote:

> Hi, 
> 
> thanks for the comments.
> 
> > 
> > thanks for the proposal. The others have already expressed some of my 
> > worries and remarks, but I think it would be only right to write them 
> > again. Especially since I am part of the team responsible for 
> > implementation and maintenance of live patches here at SUSE, we use kGraft 
> > and we prepare everything manually (compared to kpatch and ksplice).
> 
> 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.
 
> >> 1. A study of Kernel live patching schemes.
> >> 
> >> Three major kernel live patching tools:  https://lwn.net/Articles/734765/
> >> 
> >> * ksplice:   http://www.ksplice.com/doc/ksplice.pdf
> >> * kpatch:    https://lwn.net/Articles/597123/
> >>            https://github.com/dynup/kpatch
> >> * kGraft:    
> >> https://pdfs.semanticscholar.org/presentation/af4c/895aa3fef0cc2b501317aaec9d91ba2d704c.pdf
> >> 
> >> In the above, ksplice and kpatch can automatically generate binary patches 
> >> as following:
> >> 
> >>   * a collection of tools which convert a source diff patch to a patch
> >> module. They work by compiling the kernel both with and without the source
> >> patch, comparing the binaries, and generating a binary patch module which 
> >> includes new binary versions of the functions to be replaced.
> >> 
> >> on the other hand, kGraft offers a way to create patches entirely by hand. 
> >> The source of the patch is a single C file, easy to review, easy to
> >> maintain. 
> >> 
> >> In addition to kGraft, there are other live patching tools that prefer
> >> creating patches by hand for the similar reason. 
> >> 
> >> The compiler support is mainly for the above live patching tools that create 
> >> patches entirely by hand. the major purpose is:
> >> 
> >> * control patch code size and debug complexity;
> >> * keep good run time performance;
> >> 
> >> 2. the major problems of compiler in live patching:
> >> 
> >> For the live patching schemes that create patches by hand, when patching 
> >> one function, there might a list of functions that will be impacted by 
> >> this patched function due to compiler optimization/analyses (mainly IPA
> >> optimization/analyses), a complete patch will include the patched function
> >> and all impacted functions. Usually, there are two major factors to be
> >> considered in such live patching schemes:
> >> 
> >> * patch code size, one major factor is the length of the list 
> >> of impacted functions;
> >> * run time performance.
> >> 
> >> If we want to control the patch code size, to make the list of impacted 
> >> functions minimum, we have to disable corresponding compiler optimizations 
> >> as much as possible.
> > 
> > Andi already talked about it and I, too, do not understand your worry 
> > about patch code size. First, it has never been so bad here. Yes, 
> > sometimes the function closure gets bigger due to optimizations and 
> > inlining. I've considered it as nothing else than a lack of better 
> > tooling, because it is indeed something which could be improved a lot. 
> > Nicolai (CCed) works on a potential solution. It is also one of the topics 
> > at LPC miniconf in Vancouver.
> > 
> > Second, the idea to disable inlining would not fly at SUSE. I can't 
> > imagine to even propose it. The kernel heavily relies on the feature. The 
> > optimizations are a different story and some of those certainly could be 
> > disabled with no harm caused.
> > 
> > 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.
 
> 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.
> 
> > 
> >> On the other hand, in order to keep good run time performance, we need to 
> >> keep the compiler optimization as much as possible. 
> >> 
> >> So, there should be some tradeoff between these two factors. 
> >> 
> >> The following are two major categories of compiler optimizations 
> >> we should considered:
> >> 
> >> A. compiler optimizations/analyses that extract ipa info from
> >> a routine's body, and use such info to guide other optimization.
> >> 
> >> Since the body of the routine might be changed for live patching, 
> >> the ipa info extracted from the body of the routine also changes,
> >> as a result, all the routines that directly or indirectly utilize 
> >> the ipa info from this routine are in the list of the impacted 
> >> routines.  
> >> 
> >> Most of the IPA analyses and optimization belong to this category. 
> >> 
> >> Although theoretically the impacted routine list from such ipa 
> >> phases could be computed, the list might be huge. Such huge list
> >> of impacted routine might explode the patch code size too much. 
> >> 
> >> Therefore, it might be more pratical to just completely disable such
> >> ipa optimizations/analyses.
> >> 
> >> B. Inlining, and all optimizaitons that internally create clone. 
> >> for example, cloning, ipa-sra, partial inlining, etc.
> >> We can track the effect and impacted routine of such optimization 
> >> easily. 
> >> Such kind of optimization could be kept, but the compiler
> >> should provide the list of impacted functions if a routine need to 
> >> be patched.
> >> 
> >> There is patch code size explosion potential even with only enabling
> >> inlining and cloning for live patching. Users need a way to control 
> >> the inlining and cloning in order to control the code size explosion 
> >> and complexity of debugging.
> >> 
> >> 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.
 
> 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.

Regards,
Miroslav