live-patching.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Miroslav Benes <mbenes@suse.cz>
To: Joe Lawrence <joe.lawrence@redhat.com>
Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org,
	nstange@suse.de
Subject: Re: [PATCH 1/2] docs/livepatch: Add new compiler considerations doc
Date: Wed, 2 Sep 2020 15:45:33 +0200 (CEST)	[thread overview]
Message-ID: <alpine.LSU.2.21.2009021452560.23200@pobox.suse.cz> (raw)
In-Reply-To: <20200721161407.26806-2-joe.lawrence@redhat.com>

Hi,

first, I'm sorry for the late reply. Thanks, Josh, for the reminder.

CCing Nicolai. Nicolai, could you take a look at the proposed 
documentation too, please? You have more up-to-date experience.

On Tue, 21 Jul 2020, Joe Lawrence wrote:

> +Examples
> +========
> +
> +Interprocedural optimization (IPA)
> +----------------------------------
> +
> +Function inlining is probably the most common compiler optimization that
> +affects livepatching.  In a simple example, inlining transforms the original
> +code::
> +
> +	foo() { ... [ foo implementation ] ... }
> +
> +	bar() { ...  foo() ...  }
> +
> +to::
> +
> +	bar() { ...  [ foo implementation ] ...  }
> +
> +Inlining is comparable to macro expansion, however the compiler may inline
> +cases which it determines worthwhile (while preserving original call/return
> +semantics in others) or even partially inline pieces of functions (see cold
> +functions in GCC function suffixes section below).
> +
> +To safely livepatch ``foo()`` from the previous example, all of its callers
> +need to be taken into consideration.  For those callers that the compiler had
> +inlined ``foo()``, a livepatch should include a new version of the calling
> +function such that it:
> +
> +  1. Calls a new, patched version of the inlined function, or
> +  2. Provides an updated version of the caller that contains its own inlined
> +     and updated version of the inlined function

I'm afraid the above could cause a confusion...

"1. Calls a new, patched version of the inlined function, or". The 
function is not inlined in this case. Would it be more understandable to 
use function names?

1. Calls a new, patched version of function foo(), or
2. Provides an updated version of bar() that contains its own inlined and 
   updated version of foo() (as seen in the example above).

Not to say that it is again a call of the compiler to decide that, so one 
usually prepares an updated version of foo() and updated version of bar() 
calling to it. Updated foo() has to be there for non-inlined cases anyway.

> +
> +Other interesting IPA examples include:
> +
> +- *IPA-SRA*: removal of unused parameters, replace parameters passed by
> +  referenced by parameters passed by value.  This optimization basically

s/referenced/reference/

> +  violates ABI.
> +
> +  .. note::
> +     GCC changes the name of function.  See GCC function suffixes
> +     section below.
> +
> +- *IPA-CP*: find values passed to functions are constants and then optimizes
> +  accordingly Several clones of a function are possible if a set is limited.

"...accordingly. Several..."

[...]

> +  	void cdev_put(struct cdev *p)
> +  	{
> +  		if (p) {
> +  			struct module *owner = p->owner;
> +  			kobject_put(&p->kobj);
> +  			module_put(owner);
> +  		}
> +  	}

git am complained here about whitespace damage.

[...]

> +kgraft-analysis-tool
> +--------------------
> +
> +With the -fdump-ipa-clones flag, GCC will dump IPA clones that were created
> +by all inter-procedural optimizations in ``<source>.000i.ipa-clones`` files.
> +
> +kgraft-analysis-tool pretty-prints those IPA cloning decisions.  The full
> +list of affected functions provides additional updates that the source-based
> +livepatch author may need to consider.  For example, for the function
> +``scatterwalk_unmap()``:
> +
> +::
> +
> +  $ ./kgraft-ipa-analysis.py --symbol=scatterwalk_unmap aesni-intel_glue.i.000i.ipa-clones
> +  Function: scatterwalk_unmap/2930 (include/crypto/scatterwalk.h:81:60)
> +    isra: scatterwalk_unmap.isra.2/3142 (include/crypto/scatterwalk.h:81:60)
> +      inlining to: helper_rfc4106_decrypt/3007 (arch/x86/crypto/aesni-intel_glue.c:1016:12)
> +      inlining to: helper_rfc4106_decrypt/3007 (arch/x86/crypto/aesni-intel_glue.c:1016:12)
> +      inlining to: helper_rfc4106_encrypt/3006 (arch/x86/crypto/aesni-intel_glue.c:939:12)
> +
> +    Affected functions: 3
> +      scatterwalk_unmap.isra.2/3142 (include/crypto/scatterwalk.h:81:60)
> +      helper_rfc4106_decrypt/3007 (arch/x86/crypto/aesni-intel_glue.c:1016:12)
> +      helper_rfc4106_encrypt/3006 (arch/x86/crypto/aesni-intel_glue.c:939:12)

The example for the github is not up-to-date. The tool now expects 
file_list with *.ipa-clones files and the output is a bit different for 
the recent kernel.

$ echo arch/x86/crypto/aesni-intel_glue.c.000i.ipa-clones | kgraft-ipa-analysis.py --symbol=scatterwalk_unmap /dev/stdin
Parsing file (1/1): arch/x86/crypto/aesni-intel_glue.c.000i.ipa-clones
Function: scatterwalk_unmap/3935 (./include/crypto/scatterwalk.h:59:20) [REMOVED] [object file: arch/x86/crypto/aesni-intel_glue.c.000i.ipa-clones]
  isra: scatterwalk_unmap.isra.8/4117 (./include/crypto/scatterwalk.h:59:20) [REMOVED]
    inlining to: gcmaes_crypt_by_sg/4019 (arch/x86/crypto/aesni-intel_glue.c:682:12) [REMOVED] [edges: 4]
      constprop: gcmaes_crypt_by_sg.constprop.13/4182 (arch/x86/crypto/aesni-intel_glue.c:682:12)

  Affected functions: 3
    scatterwalk_unmap.isra.8/4117 (./include/crypto/scatterwalk.h:59:20) [REMOVED]
    gcmaes_crypt_by_sg/4019 (arch/x86/crypto/aesni-intel_glue.c:682:12) [REMOVED]
    gcmaes_crypt_by_sg.constprop.13/4182 (arch/x86/crypto/aesni-intel_glue.c:682:12)



The rest looks great. Thanks a lot, Joe, for putting it together.

Miroslav

  parent reply	other threads:[~2020-09-02 15:40 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-21 16:14 [PATCH 0/2] livepatch: Add compiler optimization disclaimer/docs Joe Lawrence
2020-07-21 16:14 ` [PATCH 1/2] docs/livepatch: Add new compiler considerations doc Joe Lawrence
2020-07-21 23:04   ` Josh Poimboeuf
2020-07-22 17:03     ` Joe Lawrence
2020-07-22 20:51       ` Josh Poimboeuf
2020-08-06 12:03         ` Petr Mladek
2020-08-10 19:46           ` refactoring livepatch documentation was " Joe Lawrence
2020-09-01 17:12             ` Josh Poimboeuf
2020-09-02 14:00             ` Miroslav Benes
2020-09-02 13:45   ` Miroslav Benes [this message]
2020-07-21 16:14 ` [PATCH 2/2] samples/livepatch: Add README.rst disclaimer Joe Lawrence
2020-08-06 12:07   ` Petr Mladek
2020-09-02 13:46   ` Miroslav Benes

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=alpine.LSU.2.21.2009021452560.23200@pobox.suse.cz \
    --to=mbenes@suse.cz \
    --cc=joe.lawrence@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=live-patching@vger.kernel.org \
    --cc=nstange@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).