Re: [PATCH v3 5/6] x86/ftrace: Use text_poke()

[Josh Poimboeuf]

On Tue, Jan 28, 2020 at 04:40:46PM +0100, Petr Mladek wrote:
> On Tue 2020-01-28 09:00:14, Josh Poimboeuf wrote:
> > On Tue, Jan 28, 2020 at 10:28:07AM +0100, Miroslav Benes wrote:
> > > I don't think we have something special at SUSE not generally available...
> > > 
> > > ...and I don't think it is really important to discuss that and replying 
> > > to the above, because there is a legitimate use case which relies on the 
> > > flag. We decided to support different use cases right at the beginning.
> > > 
> > > I understand it currently complicates things for objtool, but objtool is 
> > > sensitive to GCC code generation by definition. "Issues" appear with every 
> > > new GCC version. I see no difference here and luckily it is not so 
> > > difficult to fix it.
> > > 
> > > I am happy to help with acting on those objtool warning reports you 
> > > mentioned in the other email. Just Cc me where appropriate. We will take a 
> > > look.
> > 
> > As I said, the objtool warnings aren't even the main issue.
> 
> Great.
> 
> Anyway, I think that we might make your life easier with using
> the proposed -Wsuggest-attribute=noreturn.

Maybe.  Though if I understand correctly, this doesn't help for any of
the new warnings because they're for static functions, and this only
warns about global functions.

> Also it might be possible to create the list of global
> noreturn functions using some gcc tool. Similar way that we get
> the list of functions that need to be livepatched explicitly
> because of the problematic optimizations.
> 
> It sounds like a win-win approach.

I don't quite get how that could be done in an automated way, but ideas
about how to implement it would certainly be welcome.

> > There are N users[*] of CONFIG_LIVEPATCH, where N is perhaps dozens.
> > For N-1 users, they have to suffer ALL the drawbacks, with NONE of the
> > benefits.
> 
> You wrote in the other mail:
> 
>   > The problems associated with it: performance, LTO incompatibility,
>   > clang incompatibility (I think?), the GCC dead code issue.
> 
> SUSE performance team did extensive testing and did not found
> any real performance issues. It was discussed when the option
> was enabled upstream.
> 
> Are the other problems affecting real life usage, please?
> Could you be more specific about them, please?

The original commit mentioned 1-3% scheduler degradation.  And I'd
expect things to worsen over time as interprocedural optimizations
improve.

Also, LTO is coming whether we like it or not.  As is Clang.  Those are
real-world things which will need to work with livepatching sooner or
later.

> > And, even if they wanted those benefits, they have no idea how to get
> > them because the patch creation process isn't documented.
> 
> I do not understand this. All the sample modules and selftests are
> using source based livepatches.

We're talking in circles.  Have you read the thread?

The samples are a (dangerous) joke.  With or without -flive-patching.

> It is actually the only somehow documented way. Sure, the
> documentation might get improved.  Patches are welcome.

Are you suggesting for *me* to send documentation for how *you* build
patches?

> The option is not currently needed by the selftests only because there
> is no selftest for this type of problems. But the problems are real.
> They would actually deserve selftests. Again, patches are welcome.
> 
> My understanding is that the source based livepatches is the future.

I think that still remains to be seen.

> N-1 users are just waiting until the 1 user develops more helper tools
> for this.

No.  N-1 users have no idea how to make (safe) source-based patches in
the first place.  And if *you* don't need the tools, why would anyone
else?  Why not document the process and encourage the existence of other
users so they can get involved and help with the tooling?

> I would really like to hear about some serious problems
> before we do this step back in upstream.

Sometimes you need to take 1 step back before you can take 2 steps
forward.  I regret ACKing the original patch.  It was too early.

-- 
Josh