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:   Mon, 4 May 2020 22:14:45 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     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
Subject: Re: [PATCH v4 14/18] static_call: Add static_cond_call()
Message-ID: <20200504201445.GQ3762@hirez.programming.kicks-ass.net>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a53369f3-665a-af0e-efad-09ae456af847@rasmusvillemoes.dk>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, May 04, 2020 at 09:20:03AM +0200, Rasmus Villemoes wrote:

> > So there is something utterly terrible we can do to address both:
> > 
> > void __static_call_nop(void)
> > {
> > }
> > 
> > #define __static_cond_call(name) \
> > ({ \
> > 	void *func = READ_ONCE(STATIC_CALL_KEY(name).func); \
> > 	if (!func) \
> > 		func = &__static_call_nop; \
> > 	(typeof(STATIC_CALL_TRAMP(name))*)func; \
> > })
> > 
> > #define static_cond_call(name) (void)__static_cond_call(name)
> > 
> > This gets us into Undefined Behaviour territory, but it ought to work.
> > 
> > It adds the READ_ONCE(), and it cures the argument evaluation issue.
> 
> 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.

> 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 :/

> 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..

> And that brings me to the other issue I raised - do you have a few
> examples of call sites that could use this, so we can see disassembly
> before/after?

Patch 17 has a few -- which is why I wrote the support in the first
place. Obviously those will never ever hit the !HAVE_STATIC_BRANCH case.

> I'm still concerned that, even if there are no
> side-effects in the arguments, you still force the compiler to
> spill/shuffle registers for call/restore unconditionally, whereas with a
> good'ol if(), all that work is guarded by the load+test.

https://godbolt.org/z/SDRG2q

---
#include <stddef.h>


#define READ_ONCE(var)		(*((volatile typeof(var) *)&(var)))
#define WRITE_ONCE(var, val)	(*((volatile typeof(var) *)&(var)) = (val))

struct static_call_key {
	void *func;
};

#define DECLARE_STATIC_CALL(name, func)	\
	extern struct static_call_key name; \
	extern typeof(func) __SCT__##name;

#define DEFINE_STATIC_COND_CALL(name, _func) \
	DECLARE_STATIC_CALL(name, _func) \
	struct static_call_key name = { \
		.func = NULL, \
	}

static void __static_call_nop(void)
{
}

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

#define static_cond_call(name) (void)__static_cond_call(name)

static void inline static_call_update(struct static_call_key *call, void *func)
{
	WRITE_ONCE(call->func, func);
}

volatile int _x;

void bar(int x)
{
	_x = x;
}

DEFINE_STATIC_COND_CALL(foo, bar);

void ponies(int x)
{
	static_cond_call(foo)(x);
}