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;
Subject: Re: [PATCH v4 14/18] static_call: Add static_cond_call()
To:     Peter Zijlstra <peterz@infradead.org>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>
Cc:     x86@kernel.org, linux-kernel@vger.kernel.org, rostedt@goodmis.org,
        mhiramat@kernel.org, bristot@redhat.com, jbaron@akamai.com,
        torvalds@linux-foundation.org, tglx@linutronix.de,
        mingo@kernel.org, namit@vmware.com, hpa@zytor.com, luto@kernel.org,
        ard.biesheuvel@linaro.org, jpoimboe@redhat.com,
        pbonzini@redhat.com, mathieu.desnoyers@efficios.com
References: <20200501202849.647891881@infradead.org>
 <20200501202944.593400184@infradead.org>
 <1238787e-d97d-f09b-d76d-2df2dc273f4b@rasmusvillemoes.dk>
 <20200503125813.GL3762@hirez.programming.kicks-ass.net>
 <a53369f3-665a-af0e-efad-09ae456af847@rasmusvillemoes.dk>
 <20200504201445.GQ3762@hirez.programming.kicks-ass.net>
From:   Rasmus Villemoes <linux@rasmusvillemoes.dk>
Message-ID: <6cb31c72-ce0e-7923-daca-ce5ef414940b@rasmusvillemoes.dk>
Date:   Tue, 5 May 2020 09:50:26 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.7.0
MIME-Version: 1.0
In-Reply-To: <20200504201445.GQ3762@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 04/05/2020 22.14, Peter Zijlstra wrote:
> On Mon, May 04, 2020 at 09:20:03AM +0200, Rasmus Villemoes wrote:
> 
>>
>> Indeed, that is horrible. And it "fixes" the argument evaluation by
>> changing the !HAVE_STATIC_CALL case to match the HAVE_STATIC_CALL, not
>> the other way around,
> 
> Correct; making it the other way is far more 'interesting'. It would
> basically mean combining the static_branch and static_call, but that
> would also make it less optimal for simple forwarding cases.

Yes, I can see how implementing that would be quite hard.

>> which means that it is not a direct equivalent to the
>>
>>   if (foo)
>>      foo(a, b, c)
>>
>> [which pattern of course has the READ_ONCE issue, but each individual
>> existing site with that may be ok for various reasons].
>>
>> Is gcc smart enough to change the if (!func) to a jump across the
>> function call (but still evaluting side effects in args), or is
>> __static_call_nop actually emitted and called?
> 
> I was hoping it would be clever, but I just tried (find below) and it is
> not -- although there's always hoping a newer version / clang might be
> smarter.
> 
> It does indeed emit the nop function :/

Hm.

>> If the latter, then one
>> might as well patch the write-side to do "WRITE_ONCE(foo, func ? :
>> __static_call_nop)" and elide the test from __static_cond_call() - in
>> fact, that just becomes a single READ_ONCE. [There's probably some
>> annoying issue with making sure static initialization of foo points at
>> __static_call_nop].
> 
> But that would not give a more clever compiler the ability to do the
> 'right' thing here..

True. However, I think it's unlikely we'll see a compiler being that
clever anytime soon.

Anyway, it's hard to judge what version of the !HAVE_STATIC_CALL
implementation is best when there's no !HAVE_STATIC_CALL use cases to
look at. I just want to ensure that whatever limitations or gotchas
(e.g., "arguments are evaluated regardless of NULLness of func", or
alternatively "arguments must not have side effects") it ends up with
get documented.

> 
> #define __static_cond_call(name) \
> ({ \
> 	void *func = READ_ONCE(name.func); \
> 	if (!func) \
> 		func = &__static_call_nop; \
> 	(typeof(__SCT__##name)*)func; \
> })

I think you can just make it

#define __static_cond_call(name) \
( \
(typeof(__SCT__##name)*) ((void *)name.func ? : (void *)__static_call_nop) \
)

but that simplification is not enough to make gcc change its mind about
how to compile it :( But I'm guessing that various sanitizers or static
checkers might complain about the UB.

Rasmus