Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 5 May 2020 16:27:44 -0400 (EDT)
From:   Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To:     ndesaulniers <ndesaulniers@google.com>
Cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        x86 <x86@kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        rostedt <rostedt@goodmis.org>,
        Masami Hiramatsu <mhiramat@kernel.org>,
        bristot <bristot@redhat.com>, jbaron <jbaron@akamai.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@kernel.org>, Nadav Amit <namit@vmware.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Andy Lutomirski <luto@kernel.org>,
        Ard Biesheuvel <ard.biesheuvel@linaro.org>,
        Josh Poimboeuf <jpoimboe@redhat.com>,
        Paolo Bonzini <pbonzini@redhat.com>,
        "H.J. Lu" <hjl.tools@gmail.com>,
        clang-built-linux <clang-built-linux@googlegroups.com>
Message-ID: <470458191.1021.1588710464160.JavaMail.zimbra@efficios.com>
In-Reply-To: <CAKwvOdnLze0e3Vwmb1Xdqwcwe9h6gnAwGnt3ksiNX7ENb_3Y9w@mail.gmail.com>
References: <20200501202849.647891881@infradead.org> <20200504201445.GQ3762@hirez.programming.kicks-ass.net> <20200505093625.GE5298@hirez.programming.kicks-ass.net> <CAKwvOd=cP8UCX0+5pZ3AqzvOM8LKzLJJ_heDhrghqJdOnHoGMg@mail.gmail.com> <CAKwvOdkL+2Gvn2mkZ8cdHN=1F5cHQHii57ocD0RFeLJxEt=TUQ@mail.gmail.com> <CAHk-=wiUd=fcpegFLK4VK9iFfrO5BmpGKDszGpuyJkDdz4JaoQ@mail.gmail.com> <656098739.766.1588705237442.JavaMail.zimbra@efficios.com> <CAKwvOdnLze0e3Vwmb1Xdqwcwe9h6gnAwGnt3ksiNX7ENb_3Y9w@mail.gmail.com>
Subject: Re: [PATCH v4 14/18] static_call: Add static_cond_call()
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Thread-Topic: static_call: Add static_cond_call()
Thread-Index: LKX/yK89nuESaon3x7N5kSfc1EEjRQ==
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

----- On May 5, 2020, at 3:57 PM, ndesaulniers ndesaulniers@google.com wrote:

> On Tue, May 5, 2020 at 12:00 PM Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> ----- On May 5, 2020, at 2:48 PM, Linus Torvalds torvalds@linux-foundation.org
>> wrote:
>> [...]
>> >
>> > Your initial reaction that "you can't compile away the read and the
>> > test of NULL" was correct, I think.
>>
>> I suspect this pattern of "if (func != NULL) func(...)" could be semantically
>> changed to just invoking an empty function which effectively does nothing.
>> This would remove the need to do a pointer check in the first place. But maybe
>> I'm missing something subtle about why it has not been done in this context.
> 
> Good idea, this eliminates the check: https://godbolt.org/z/Xugo9w
> but you still have an indirect tail call (I think a direct tail call
> is the desired solution?)

Actually, if the goal is to do code patching of the call, I wonder
what makes it OK to "guess" all the call patterns generated by the compiler ?
AFAIU this is not an ABI in any way. For instance, a new compiler version could
choose to add some no-op instructions within this pattern just because it feels
like it.

For static jumps, we worked with the compiler people to add "asm goto ()" so
we could express a jump in assembly which would branch outside of the asm.
Emitting the jump in assembly allows us to control the exact code pattern,
which can then be patched, and the asm goto operands allow the compiler to
be in control of all the allowed branch targets.

I'm again possibly missing something, but it looks like this proposal of static_call()
(especially the static_cond_call part) is trying to just assume the common call
patterns generated by the compilers, and patch those. What is the expected behavior
if a compiler ends up generating unknown code patterns in future versions ?

I would think a more robust approach would be to, again, work with the compiler people
and introduce something like:

asm call ("asm goes here" : : : funcA, funcB, funcC )

which would allow patching the call emitted _in assembly_ between the various
available targets. Bonus points if the compiler can let the asm know whether
it's a standard call or tail-call.

Then once we have that, we can start doing fun stuff like adding a conditional
within the assembly, but I don't see why the conditional should be the same
variable as the actual function pointer: each can be changed independently as
long as the function pointer always points to a "valid" function (not NULL).

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com