On 28 June 2018 at 11:02, Ard Biesheuvel [off-list ref] wrote:

On 28 June 2018 at 10:50, Peter Zijlstra [off-list ref] wrote:

quoted

On Wed, Jun 27, 2018 at 06:06:01PM +0200, Ard Biesheuvel wrote:

quoted

diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 86ec0652d3b1..aa203dffe72c 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h

@@ -121,6 +121,32 @@ struct static_key {
 #include <asm/jump_label.h>

 #ifndef __ASSEMBLY__
+#ifdef CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE
+
+struct jump_entry {
+     int code;
+     int target;
+     int key;
+};

I much prefer you use 'u32' there.

Actually, they are signed so that would be s32. But yeah, I can change that.

quoted

quoted

+static void jump_label_swap(void *a, void *b, int size)
+{
+     long delta = (unsigned long)a - (unsigned long)b;
+     struct jump_entry *jea = a;
+     struct jump_entry *jeb = b;
+     struct jump_entry tmp = *jea;
+
+     jea->code       = jeb->code - delta;
+     jea->target     = jeb->target - delta;
+     jea->key        = jeb->key - delta;
+
+     jeb->code       = tmp.code + delta;
+     jeb->target     = tmp.target + delta;
+     jeb->key        = tmp.key + delta;
+}
+
 static void
 jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop)
 {

@@ -56,7 +72,9 @@ jump_label_sort_entries(struct jump_entry *start, struct jump_entry *stop)

      size = (((unsigned long)stop - (unsigned long)start)
                                      / sizeof(struct jump_entry));
-     sort(start, size, sizeof(struct jump_entry), jump_label_cmp, NULL);
+     sort(start, size, sizeof(struct jump_entry), jump_label_cmp,
+          IS_ENABLED(CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE) ? jump_label_swap
+                                                           : NULL);
 }

That will result in jump_label_swap being an unused symbol for some
compile options.

No, and isn't that the point of IS_ENABLED()? The compiler sees a
reference to jump_label_swap(), so it won't complain about it being
unused.

quoted

Would it not be much nicer to write that like:

static void jump_label_swap(void *a, void *b, int size)
{
        struct jump_entry *jea = a, *jeb = b;

#ifdef CONFIG_HAVE_ARCH_JUMP_LABEL_RELATIVE
        long delta = a - b;

        jea->code += delta;
        jea->target += delta;
        jea->key += delta;

        jeb->code -= delta;
        jeb->target -= delta;
        jeb->key -= delta;
#else

        swap(*jea, *jeb);
}

And then unconditionally use jump_label_swap().

Meh. I thought IS_ENABLED() was preferred over #ifdef, no? That way,
the compiler always sees the code, and simply discards it without
complaining if it ends up left unused.

... and it means the sort() routine will unconditionally perform an
indirect function call even if the arch does not require it.