Re: [PATCH v4 14/18] static_call: Add static_cond_call()

[Peter Zijlstra]

On Wed, May 06, 2020 at 12:24:55PM -0500, Josh Poimboeuf wrote:

> On that note, what do you think about tweaking the naming from
> 
>   DEFINE_STATIC_COND_CALL(name, typename);
>   static_cond_call(name)(args...);
> 
> to
> 
>   DEFINE_STATIC_CALL_NO_FUNC(name, typename);
>   static_call_if_func(name)(args...);
> 
> ?
> 
> Seems clearer to me.  They're still STATIC_CALLs, so it seems logical to
> keep those two words together.  And NO_FUNC clarifies the initialized
> value.
> 
> Similarly RETTRAMP could be ARCH_DEFINE_STATIC_CALL_NO_FUNC.

So I dislike static_call_if_func(), that's so much typing. Also, I
prefer ARCH_*_RETTRAMP as it clearly describes what the thing is.

How is something like this? 

---
--- a/include/linux/static_call.h
+++ b/include/linux/static_call.h
@@ -16,7 +16,7 @@
  *
  *   DECLARE_STATIC_CALL(name, func);
  *   DEFINE_STATIC_CALL(name, func);
- *   DEFINE_STATIC_COND_CALL(name, typename);
+ *   DEFINE_STATIC_CALL_NULL(name, typename);
  *   static_call(name)(args...);
  *   static_cond_call(name)(args...);
  *   static_call_update(name, func);
@@ -54,6 +54,43 @@
  *   rather than calling through the trampoline.  This requires objtool or a
  *   compiler plugin to detect all the static_call() sites and annotate them
  *   in the .static_call_sites section.
+ *
+ *
+ * Notes on NULL function pointers:
+ *
+ *   Static_call()s support NULL functions, with many of the caveats that
+ *   regular function pointers have.
+ *
+ *   Clearly calling a NULL function pointer is 'BAD', so too for
+ *   static_call()s (although when HAVE_STATIC_CALL it might not be immediately
+ *   fatal). A NULL static_call can be the result of:
+ *
+ *     DECLARE_STATIC_CALL_NULL(my_static_call, void (*)(int));
+ *
+ *   which is equivalent to declaring a NULL function pointer with just a
+ *   typename:
+ *
+ *     void (*my_func_ptr)(int arg1) = NULL;
+ *
+ *   or using static_call_update() with a NULL function. In both cases the
+ *   HAVE_STATIC_CALL implementation will patch the trampoline with a RET
+ *   instruction, instead of an immediate tail-call JMP. HAVE_STATIC_CALL_INLINE
+ *   architectures can patch the trampoline call to a NOP.
+ *
+ *   In all cases, any argument evaluation is unconditional. Unlike a regular
+ *   conditional function pointer call:
+ *
+ *     if (my_func_ptr)
+ *         my_func_ptr(arg1)
+ *
+ *   where the argument evaludation also depends on the pointer value.
+ *
+ *   When calling a static_call that can be NULL, use:
+ *
+ *     static_cond_call(name)(arg1);
+ *
+ *   which will include the required value tests to avoid NULL-pointer
+ *   dereferences.
  */
 
 #include <linux/types.h>
@@ -122,7 +159,7 @@ extern int static_call_text_reserved(voi
 	};								\
 	ARCH_DEFINE_STATIC_CALL_TRAMP(name, _func)
 
-#define DEFINE_STATIC_COND_CALL(name, _func)				\
+#define DEFINE_STATIC_CALL_NULL(name, _func)				\
 	DECLARE_STATIC_CALL(name, _func);				\
 	struct static_call_key STATIC_CALL_KEY(name) = {		\
 		.func = NULL,						\
@@ -154,7 +191,7 @@ struct static_call_key {
 	};								\
 	ARCH_DEFINE_STATIC_CALL_TRAMP(name, _func)
 
-#define DEFINE_STATIC_COND_CALL(name, _func)				\
+#define DEFINE_STATIC_CALL_NULL(name, _func)				\
 	DECLARE_STATIC_CALL(name, _func);				\
 	struct static_call_key STATIC_CALL_KEY(name) = {		\
 		.func = NULL,						\
@@ -198,7 +235,7 @@ struct static_call_key {
 		.func = _func,						\
 	}
 
-#define DEFINE_STATIC_COND_CALL(name, _func)				\
+#define DEFINE_STATIC_CALL_NULL(name, _func)				\
 	DECLARE_STATIC_CALL(name, _func);				\
 	struct static_call_key STATIC_CALL_KEY(name) = {		\
 		.func = NULL,						\