Re: [RFC PATCH for 4.15 v12 00/22] Restartable sequences and CPU op vector

[Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>]

On Tue, 21 Nov 2017, Mathieu Desnoyers wrote:
> ----- On Nov 21, 2017, at 12:21 PM, Andi Kleen andi-Vw/NltI1exuRpAAqCnN02g@public.gmane.org wrote:
> 
> > On Tue, Nov 21, 2017 at 09:18:38AM -0500, Mathieu Desnoyers wrote:
> >> Hi,
> >> 
> >> Following changes based on a thorough coding style and patch changelog
> >> review from Thomas Gleixner and Peter Zijlstra, I'm respinning this
> >> series for another RFC.
> >> 
> > My suggestion would be that you also split out the opv system call.
> > That seems to be main contention point currently, and the restartable
> > sequences should be useful without it.
> 
> I consider rseq to be incomplete and a pain to use in various scenarios
> without cpu_opv. 
> 
> About the contention point you refer to:
> 
> Using vDSO as an example of how things should be done is just wrong: the
> vDSO interaction with debugger instruction single-stepping is broken,
> as I detailed in my previous email.

Let me turn that around. You're lamenting about a conditional branch in
your rseq thing for performance reasons and at the same time you want to
force extra code into the VDSO? clock_gettime() is one of the hottest
vsyscalls in certain scenarions. So why would we want to have extra code
there? Just to make debuggers happy. You really can't be serious about
that.

> Thomas' proposal of handling single-stepping with a user-space locking
> fallback, which is pretty much what I had in 2016, pushes a lot of
> complexity to user-space, requires an extra branch in the fast-path,
> as well as additional store-release/load-acquire semantics for consistency.
> I don't plan going down that route.
>
> Other than that, I have not received any concrete alternative proposal to
> properly handle single-stepping.

You provided the details today. Up to that point all we had was handwaving
and inconsistent information.

> The only opposition against cpu_opv is that there *should* be an hypothetical
> simpler solution. The rseq idea is not new: it's been presented by Paul Turner
> in 2012 at LPC. And so far, cpu_opv is the overall simplest and most
> efficient way I encountered to handle single-stepping, and it gives extra
> benefits, as described in my changelog.

That's how you define it and that does not make cpu_opv less complex and
more debuggable. There is no way to debug that and still you claim that it
removes compexity from user space. That ops stuff comes from user space and
is not magically constructed by the kernel. In some of your use cases it
even has different semantics than the rseq section code. So how is that
removing any complexity from user space? All it buys you is an extra branch
less in your rseq hotpath and that's your justification to shove that
thing into the kernel.

The version I reviewed was just undigestable. I did not have time to look
at the hastily cobbled together version of today. Aside of that the
scheduler portion of it has not seen any review from scheduler folks
either.

AFAICT there is not a single reviewed-by tag on the sys_rseq and the
sys_opv patches either.

Are you seriously expecting that new syscalls of that kind are going to be
merged without a deep and thorough review just based on your decision to
declare them ready?

Thanks,

	tglx