From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 94809 invoked by alias); 23 Oct 2018 13:08:16 -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 94799 invoked by uid 89); 23 Oct 2018 13:08:16 -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=EXPORT, vmlinux, tedious, inspect 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 13:08:10 +0000 Received: from relay1.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id DA400AF4A; Tue, 23 Oct 2018 13:08:07 +0000 (UTC) From: Nicolai Stange To: Qing Zhao Cc: Miroslav Benes , gcc-patches@gcc.gnu.org, Martin Jambor , Martin =?utf-8?Q?Li=C5=A1ka?= , live-patching@vger.kernel.org, Jan Hubicka , richard Biener , nstange@suse.de Subject: Re: [RFC] GCC support for live-patching References: <20180926150649.GD61973@kam.mff.cuni.cz> <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> Date: Tue, 23 Oct 2018 13:37:00 -0000 In-Reply-To: <03295B84-165F-4146-AF21-3C0CBB367934@oracle.com> (Qing Zhao's message of "Mon, 22 Oct 2018 15:17:02 -0500") Message-ID: <87in1s4xqh.fsf@suse.de> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SW-Source: 2018-10/txt/msg01430.txt.bz2 Hi, Qing Zhao writes: >>=20 >> thanks for the proposal. The others have already expressed some of my=20 >> worries and remarks, but I think it would be only right to write them=20 >> again. Especially since I am part of the team responsible for=20 >> implementation and maintenance of live patches here at SUSE, we use kGra= ft=20 >> and we prepare everything manually (compared to kpatch and ksplice). > > One question here, what=E2=80=99s the major benefit to prepare the patch= es manually?=20 There is none. We here at SUSE prefer the source based approach (as opposed to binary diff) for a number of reasons and the manual live patch creation is simply a consequence of not having any tooling for this yet. For reference, source based live patch creation involves the following steps: 1. Determine the initial set of to be patched functions: a.) Inspect the upstream diff for the fix in question, add any touched functions to the initial set. b.) For each function in the initial set, check whether it has been inlined/cloned/optimized and if so, add all its callers to the initial set. Repeat until the initial set has stabilized. 2. Copy & paste the initial set over to the new live patch sources. 3. Make it compile, i.e. recursively copy any needed cpp macro, type, or functions definition and add references to data objects with static storage duration. The rules are: a.) For data objects with static storage duration, a reference to the original must always be made. (If the symbol is EXPORT()ed, then fine. Otherwise, for kGraft, this involves a kallsyms lookup at patch module load time, for upstream kernel live patching, this has been solved with those '.klp....' relocations). b.) If a called function is available as a symbol from either vmlinux or some (usually the patched) module, do not copy the definition, but add a reference to it, just as in a.). c.) If a type, cpp macro or (usually inlined) function is provided by some "public" header in /include/, include that rather than copying the definition. Counterexample: Non-public header outside of include/ like e.g. /fs/btrfs/qgroup.h. d.) Otherwise copy the definition to the live patch module sources. Rule 3b is not strictly necessary, but it helps in reducing the live patch code size which is a factor with _manual_ live patch creation. For 1b.), we need help from GCC. Namely, we want to know when some functions has been optimized and we want it to disable any of those IPA optimization it (currently) isn't capable to report properly. Step 3.) is a bit tedious sometimes TBH and yes, w/o any tooling in place, patch size would be a valid point. However, I'm currently working on that and I'm optimistic that I'll have a working prototype soon. That tool would be given the GCC command line from the original or "live patch target" kernel compilation for the source file in question, the set of functions as determined in 1.) and a number of user provided filter scripts to make the decisions in 3.). As a result, it would output a self-contained, minimal subset of the original kernel sources. With that tooling in place, live patch code size would not be a real concern for us. So in conclusion, what we need from GCC is the information on when we have to live patch callers due to optimizations. If that's not possible for a particular class of optimization, it needs to be disabled. OTOH, we definitely want to keep the set of these disabled optimizations as small as possible in order to limit the impact of live patching on kernel performance. In particular, disabling any of the "cloning" optimizations, which GCC is able to report properly, would be a no-no IMO. IIUC, our preferred selection of allowed IPA optimizations would be provided by what you are referring to as "-flive-patching=3Dinline-clone". >>=20 >>> 1. A study of Kernel live patching schemes. >>>=20 >>> Three major kernel live patching tools: https://lwn.net/Articles/73476= 5/ >>>=20 >>> * ksplice: http://www.ksplice.com/doc/ksplice.pdf >>> * kpatch: https://lwn.net/Articles/597123/ >>> https://github.com/dynup/kpatch >>> * kGraft:=20=20=20=20 >>> https://pdfs.semanticscholar.org/presentation/af4c/895aa3fef0cc2b501317= aaec9d91ba2d704c.pdf >>>=20 >>> In the above, ksplice and kpatch can automatically generate binary patc= hes=20 >>> as following: >>>=20 >>> * 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 sou= rce >>> patch, comparing the binaries, and generating a binary patch module whi= ch=20 >>> includes new binary versions of the functions to be replaced. >>>=20 >>> on the other hand, kGraft offers a way to create patches entirely by ha= nd.=20 >>> The source of the patch is a single C file, easy to review, easy to >>> maintain.=20 >>>=20 >>> In addition to kGraft, there are other live patching tools that prefer >>> creating patches by hand for the similar reason.=20 Out of curiosity: which? Upstream kernel live patching? >>> The compiler support is mainly for the above live patching tools that c= reate=20 >>> patches entirely by hand. the major purpose is: >>>=20 >>> * control patch code size and debug complexity; >>> * keep good run time performance; >>>=20 >>> 2. the major problems of compiler in live patching: >>>=20 >>> For the live patching schemes that create patches by hand, when patchin= g=20 >>> one function, there might a list of functions that will be impacted by= =20 >>> this patched function due to compiler optimization/analyses (mainly IPA >>> optimization/analyses), a complete patch will include the patched funct= ion >>> and all impacted functions. Usually, there are two major factors to be >>> considered in such live patching schemes: >>>=20 >>> * patch code size, one major factor is the length of the list=20 >>> of impacted functions; >>> * run time performance. >>>=20 >>> If we want to control the patch code size, to make the list of impacted= =20 >>> functions minimum, we have to disable corresponding compiler optimizati= ons=20 >>> as much as possible. >>=20 >> Andi already talked about it and I, too, do not understand your worry=20 >> about patch code size. First, it has never been so bad here. Yes,=20 >> sometimes the function closure gets bigger due to optimizations and=20 >> inlining. I've considered it as nothing else than a lack of better=20 >> tooling, because it is indeed something which could be improved a lot.=20 >> Nicolai (CCed) works on a potential solution. It is also one of the topi= cs=20 >> at LPC miniconf in Vancouver. >>=20 >> Second, the idea to disable inlining would not fly at SUSE. I can't=20 >> imagine to even propose it. The kernel heavily relies on the feature. Th= e=20 >> optimizations are a different story and some of those certainly could be= =20 >> disabled with no harm caused. >>=20 >> So let me ask, what is your motivation behind this? Is there a real=20 >> problem you're trying to solve? It may have been mentioned somewhere and= I=20 >> missed it. > > the major functionality we want is: to Only enable static inlining for = live patching for one=20 > of our internal customers. the major purpose is to control the patch co= de size explosion and > debugging complexity due to too much inlining of global functions for the= specific application. > > therefore, I proposed the multiple level of control for -flive-patching t= o satisfy multiple request from=20 > different users.=20 > > So far, from the feedback, I see that among the 4 levels of control, no= ne, only-inline-static, inline, > and inline-clone, =E2=80=9Cnone=E2=80=9D and =E2=80=9Cinline=E2=80=9D a= re NOT needed at all. > > however, -flive-patching =3D [only-inline-static | inline-clone] are > necessary. It would be interesting to learn why that internal customer is so keen about live patch code size? I mean either their live patch creation is somehow automated or not. In the former case, the extra .text of inline-clone over only-inline-static would be handled by tooling anyway and thus, wouldn't cost any additional working hours and be highly unlikely to introduce (new) regressions to be debugged. In the latter case, wouldn't improving the tooling also benefit the only-inline-static case? Thanks, Nicolai --=20 SUSE Linux GmbH, GF: Felix Imend=C3=B6rffer, Jane Smithard, Graham Norton, HRB 21284 (AG N=C3=BCrnberg)