Hi Quing,
On Thu, Oct 18 2018, Qing Zhao wrote:
> Hi,
>
> After more detailed study of GCC’s IPA optimizations, further study of the current available kernel live patching schemes and
> other live-patching user’s request, I came up with the following initial proposal in GCC to mainly support live-patching users who
> manually create patches.
>
> Please take a look at the writeup, and let me know your opinions and suggestions.
thank you for writing down your thoughts. My general note is that I'd
be wary with proposing (and implementing?) many options which would
eventually remain virtually unused with all the problems this entails.
I would concentrate on the few things that (kernel) live-patching
engineers really request and plan to routinely use now. I do not think
that just because you can imagine a live-patching option might be useful
in some set of peculiar circumstances, it is worth having in GCC. If we
find that we need more later, we can always add them.
More comments inline:
>
> thanks a lot.
>
> Qing
>
> ===========
>
> options to help live patching in GCC
>
> Qing Zhao
>
> 10/17/2018
> ================================================
>
> 0. The proposal:
>
> Provide two first class options in GCC to help live-patching users
> who manually generate patches.
>
> A. an option to control GCC's IPA optimizations to provide a safe
> compilation for live-patching purpose. At the same time, provides
> multiple-level control of patch code-size and run time performance
> tradeoff.
>
> -fease-live-patching={none|only-inline-static|inline|inline-clone}
I also agree that -flive-patching is much better.
> B. an option to compute the impacted function lists and dump them for
> live-patching users.
>
> -flive-patching-list={func_name|all}{,dump_func_name}
Do you really think someone will ever want to use one function but not
the other? Combining them into one would give us the flexibility to
change our mind in the future whether we want to identify all affected
functions or disable a particular analysis/transformation.
My preference would be to only have -flive-patching that would behave
like the default options for both options you propose, i.e. for each
function/symbol, dump to a file all other functions which are affected
in any way and disable any transformation where we cannot (easily) do
so.
...
> 3. Details of the proposal:
>
> What should GCC provide to live-patching users who manually create
> patches?
>
> A. an option to control GCC's IPA optimizations to provide a safe
> compilation for live-patching purpose. At the same time, provides
> multiple levels of patch code-size and run time performance tradeoff.
>
> -fease-live-patching={none|only-inline-static|inline|inline-clone}
>
> -fease-live-patching=none
>
> Disable all ipa optimizations/analyses in GCC. As a result, any
> routine can be patched independently without impacting other routines.
How do you plan to reconcile this option with attribute always_inline?
We inline such functions even when you pass -fno-inline on the command
line and the description of the attribute says that "failure to inline
such a function is diagnosed as an error." The only solution which I
can think of is to make combination of your proposed option and
always_inline attribute an unsupported one. But (AFAIK) since Linux
kernel uses it everywhere, it would make -fease-live-patching=none
impossible to use with Linux kernel.
Additionally, I am quite certain that nobody will find the performance
degradation caused by not inlining anything at all in Linux kernel
acceptable in production. It is not only my opinion, I have discussed
this with our live patching engineers and they agreed that if you find
out you have to patch a spinlock routine, you are very much... eh, to
put it mildly, you have just found yourself in an impossible situation.
So to conclude, I really think introducing this option is a bad idea, it
is impossible to define well and unlikely to be used by anybody.
>
> -fease-live-patching=only-inline-static
>
> Only enable inlining of static functions, disable all other IPA
> optimizations/analyses. As a result, when patching a static routine,
> all its callers need to be patches as well.
My general concern applies here as well, I still think the default
behavior is better for everybody. But you might still convince Honza to
accept this because implementation-wise it is not difficult.
OTOH, I am no sure this is really defined well either, do we really want
say "static" functions and not functions with internal linkage? My
concern is not only use with LTO but also functions in anonymous C++
namespaces. I know Linux kernel does not really care for either but I
guess we should still be more careful when defining user facing options.
>
> -fease-live-patching=inline
>
> Only enable inlining, disable all other IPA optimization/analyses.
> As a result, when patching a routine, all its callers need to be patches
> as well.
I am not convinced there is any merit in this behavior over the proposed
default but well, if you really think it is useful, it is easy to do.
> -fease-live-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.
This would be my preferred (first) implementation (later on we may
discover we can do more, perhaps without involving clones) but I dislike
the name and description. You really do not want to rely on internals
such as the notion of a "clone" when describing user-visible features.
As I have written above, I'd define it simply by dump all other affected
functions and disable any analysis/transformation for which this is not
easily possible. This would give the user exactly what they want and
leave us with room to change (improve!) how we do stuff internally in
the future.
>
> When -fease-live-patching specified without any value, the default value
> is "inline-clone".
>
> B. an option to compute the impacted function lists and dump them for
> live-patching users.
>
> -flive-patching-list={func_name|all}{,dump_func_name}
>
Do you have any plans how to regularly test this works reliably?
Do you volunteer to implement this so that it works even when
-flive-patching is not given on the command line? If not, I really
suggest combining these two options into one.
Can you please find out how just having the default for both options
would not work for you? I would be good to know so that we can better
think about how to define the non-default behaviors.
Thanks,
Martin
