From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 20920 invoked by alias); 23 Oct 2018 09:11:42 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 20900 invoked by uid 89); 23 Oct 2018 09:11:41 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,KAM_SHORT,SPF_PASS autolearn=ham version=3.3.2 spammy=hacks, hoped X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 23 Oct 2018 09:11:37 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id B2C9AAFB2; Tue, 23 Oct 2018 09:11:35 +0000 (UTC) Date: Tue, 23 Oct 2018 09:37:00 -0000 From: Miroslav Benes To: Qing Zhao cc: gcc-patches@gcc.gnu.org, Martin Jambor , =?ISO-8859-15?Q?Martin_Li=A8ka?= , live-patching@vger.kernel.org, Jan Hubicka , richard Biener , nstange@suse.de Subject: Re: [RFC] GCC support for live-patching In-Reply-To: <03295B84-165F-4146-AF21-3C0CBB367934@oracle.com> Message-ID: References: <20180926171642.GG61973@kam.mff.cuni.cz> <131EB12C-DC35-4B3C-9F89-40EBA5317261@oracle.com> <20180927085829.GJ61973@kam.mff.cuni.cz> <817E4C4A-F81B-4E09-9645-04E38A1B9E74@oracle.com> <5CB6BDBE-3F49-4BFE-AF10-5E8181C49181@oracle.com> <1a023bdc-28a6-eb41-b449-4d096f12064f@suse.cz> <048D9997-B7AF-444A-BF7E-79944DE8F174@oracle.com> <3E37D3A8-2D19-41C2-BA8A-8F0EFA1B4D5C@oracle.com> <10a54034-279b-a406-8466-55558effbf24@suse.cz> <980ECBBE-86E9-49EC-A573-96CDCB850BE5@oracle.com> <03295B84-165F-4146-AF21-3C0CBB367934@oracle.com> User-Agent: Alpine 2.21 (LSU 202 2017-01-01) MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="1678380546-629804866-1540283742=:31349" Content-ID: X-SW-Source: 2018-10/txt/msg01404.txt.bz2 This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --1678380546-629804866-1540283742=:31349 Content-Type: text/plain; CHARSET=UTF-8 Content-Transfer-Encoding: 8BIT Content-ID: Content-length: 8527 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 --1678380546-629804866-1540283742=:31349--