Messages in this thread | | | Date | Mon, 4 May 2020 22:14:45 +0200 | From | Peter Zijlstra <> | Subject | Re: [PATCH v4 14/18] static_call: Add static_cond_call() |
| |
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); }
| |