Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1486687 > unrolled thread

[PATCH 0/2] jump_labels: Embedding static keys inside structures

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] jump_labels: Embedding static keys inside structures Marc Zyngier <marc.zyngier@arm.com> - 2016-09-19 19:30 +0200
    [PATCH 1/2] jump_labels: Add API to deal with keys embedded in structures Marc Zyngier <marc.zyngier@arm.com> - 2016-09-19 19:30 +0200
      Re: [PATCH 1/2] jump_labels: Add API to deal with keys embedded in  structures Peter Zijlstra <peterz@infradead.org> - 2016-09-20 11:50 +0200
        Re: [PATCH 1/2] jump_labels: Add API to deal with keys embedded in  structures Marc Zyngier <marc.zyngier@arm.com> - 2016-09-20 11:50 +0200
        Re: [PATCH 1/2] jump_labels: Add API to deal with keys embedded in  structures Christoffer Dall <christoffer.dall@linaro.org> - 2016-09-20 14:30 +0200
          Re: [PATCH 1/2] jump_labels: Add API to deal with keys embedded in  structures Peter Zijlstra <peterz@infradead.org> - 2016-09-20 14:50 +0200
            Re: [PATCH 1/2] jump_labels: Add API to deal with keys embedded in  structures Christoffer Dall <christoffer.dall@linaro.org> - 2016-09-20 15:00 +0200
              Re: [PATCH 1/2] jump_labels: Add API to deal with keys embedded in  structures Peter Zijlstra <peterz@infradead.org> - 2016-09-22 09:50 +0200
    [PATCH 2/2] dynamic_debug: Use updated jump label API Marc Zyngier <marc.zyngier@arm.com> - 2016-09-19 19:30 +0200

#1486687 — [PATCH 0/2] jump_labels: Embedding static keys inside structures

The jump_label API assumes that the static keys will be allocated as
top-level static variables. Nothing wrong with that, but this makes it
slightly awkward when trying to embed the static key inside another
structure. The user ends up diving into the internals of the API,
making the result a bit awkward.

This series adds the usual DECLARE/INIT macros that make it possible
to embed the key in macros, as well as initialize it correctly. As an
example, the dynamic_debug.h file is converted to this API.

The ulterior motive behind that patch is this[1] bit of KVM/ARM, which
is using such a construct and could use the cleanup.

Patches on top of arm64/for-next/core, which already contains some
jump label changes.

[1] https://git.kernel.org/cgit/linux/kernel/git/kvmarm/kvmarm.git/commit/?h=queue&id=db6ca86d92d9d8f8e0bf9c1210a90ddf70e76136

Marc Zyngier (2):
  jump_labels: Add API to deal with keys embedded in structures
  dynamic_debug: Use updated jump label API

 Documentation/static-keys.txt | 19 +++++++++++++++++++
 include/linux/dynamic_debug.h | 22 ++++++++++------------
 include/linux/jump_label.h    | 21 ++++++++++++++++++---
 3 files changed, 47 insertions(+), 15 deletions(-)

-- 
2.1.4

[toc] | [next] | [standalone]

#1486689 — [PATCH 1/2] jump_labels: Add API to deal with keys embedded in structures

It is desirable to allow static keys to be integrated in structures,
as it can lead do slightly more readable code. But the current API
only provides DEFINE_STATIC_KEY_TRUE/FALSE, which is not exactly
nice and leads to the following idiom:

	static struct {
		int			foo;
		struct static_key_false	key;
	} bar = {
		.key	= STATIC_KEY_FALSE_INIT,
	};

	[...]

	if (static_branch_unlikely(&bar.key))
		foo = -1;

which doesn't follow the recommended API, and uses the internals
of the static key implementation.

This patch introduces DECLARE_STATIC_KEY_TRUE/FALSE, as well as
INIT_STATIC_KEY_TRUE/FALSE, which abstract such construct and
allow the internals to evolve without having to fix everything else:

	static struct {
		int			 foo;
		DECLARE_STATIC_KEY_FALSE(key);
	} bar = {
		INIT_STATIC_KEY_FALSE(.key),
	};

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 Documentation/static-keys.txt | 19 +++++++++++++++++++
 include/linux/jump_label.h    | 21 ++++++++++++++++++---
 2 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/Documentation/static-keys.txt b/Documentation/static-keys.txt
index ea8d7b4..a2fedb2 100644
--- a/Documentation/static-keys.txt
+++ b/Documentation/static-keys.txt
@@ -15,6 +15,10 @@ The updated API replacements are:
 
 DEFINE_STATIC_KEY_TRUE(key);
 DEFINE_STATIC_KEY_FALSE(key);
+DECLARE_STATIC_KEY_TRUE(key);
+DECLARE_STATIC_KEY_FALSE(key);
+INIT_STATIC_KEY_TRUE(key);
+INIT_STATIC_KEY_FALSE(key);
 DEFINE_STATIC_KEY_ARRAY_TRUE(keys, count);
 DEFINE_STATIC_KEY_ARRAY_FALSE(keys, count);
 static_branch_likely()
@@ -142,6 +146,21 @@ static_branch_inc(), will change the branch back to true. Likewise, if the
 key is initialized false, a 'static_branch_inc()', will change the branch to
 true. And then a 'static_branch_dec()', will again make the branch false.
 
+Should the key be declared in a structure and required to be
+initialized when such structure is defined, the following construct
+can be used:
+
+    	struct foo {
+    		DECLARE_STATIC_KEY_FALSE(key);
+    	};
+
+	static struct foo bar = {
+    		INIT_STATIC_KEY_FALSE(.key),
+    	};
+
+(respectively DECLARE_STATIC_KEY_TRUE/INIT_STATIC_KEY_TRUE for the
+opposite case).
+
 Where an array of keys is required, it can be defined as:
 
 	DEFINE_STATIC_KEY_ARRAY_TRUE(keys, count);
diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index a534c7f..10ee414 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -21,6 +21,10 @@
  *
  * DEFINE_STATIC_KEY_TRUE(key);
  * DEFINE_STATIC_KEY_FALSE(key);
+ * DECLARE_STATIC_KEY_TRUE(key);
+ * DECLARE_STATIC_KEY_FALSE(key);
+ * INIT_STATIC_KEY_TRUE(key);
+ * INIT_STATIC_KEY_FALSE(key);
  * DEFINE_STATIC_KEY_ARRAY_TRUE(keys, count);
  * DEFINE_STATIC_KEY_ARRAY_FALSE(keys, count);
  * static_branch_likely()
@@ -36,7 +40,10 @@
  * "if (static_branch_unlikely(&key))", in which case we will generate an
  * unconditional branch to the out-of-line true branch. Keys that are
  * initially true or false can be using in both static_branch_unlikely()
- * and static_branch_likely() statements.
+ * and static_branch_likely() statements. DECLARE_STATIC_KEY_TRUE/FALSE
+ * can be used to declare a key that will be defined somewhere else.
+ * INIT_STATIC_KEY_TRUE/FALSE initialize a static key that can be declared
+ * in another structure.
  *
  * At runtime we can change the branch target by setting the key
  * to true via a call to static_branch_enable(), or false using
@@ -266,11 +273,19 @@ struct static_key_false {
 #define STATIC_KEY_TRUE_INIT  (struct static_key_true) { .key = STATIC_KEY_INIT_TRUE,  }
 #define STATIC_KEY_FALSE_INIT (struct static_key_false){ .key = STATIC_KEY_INIT_FALSE, }
 
+#define DECLARE_STATIC_KEY_TRUE(name)	struct static_key_true name
+
+#define DECLARE_STATIC_KEY_FALSE(name)	struct static_key_false name
+
+#define INIT_STATIC_KEY_TRUE(name)	name = STATIC_KEY_TRUE_INIT
+
+#define INIT_STATIC_KEY_FALSE(name)	name = STATIC_KEY_FALSE_INIT
+
 #define DEFINE_STATIC_KEY_TRUE(name)	\
-	struct static_key_true name = STATIC_KEY_TRUE_INIT
+	DECLARE_STATIC_KEY_TRUE(name) = STATIC_KEY_TRUE_INIT
 
 #define DEFINE_STATIC_KEY_FALSE(name)	\
-	struct static_key_false name = STATIC_KEY_FALSE_INIT
+	DECLARE_STATIC_KEY_FALSE(name) = STATIC_KEY_FALSE_INIT
 
 #define DEFINE_STATIC_KEY_ARRAY_TRUE(name, count)		\
 	struct static_key_true name[count] = {			\
-- 
2.1.4

[toc] | [prev] | [next] | [standalone]

#1487185 — Re: [PATCH 1/2] jump_labels: Add API to deal with keys embedded in structures

On Mon, Sep 19, 2016 at 06:21:27PM +0100, Marc Zyngier wrote:
> It is desirable to allow static keys to be integrated in structures,
> as it can lead do slightly more readable code. But the current API
> only provides DEFINE_STATIC_KEY_TRUE/FALSE, which is not exactly
> nice and leads to the following idiom:
> 
> 	static struct {
> 		int			foo;
> 		struct static_key_false	key;
> 	} bar = {
> 		.key	= STATIC_KEY_FALSE_INIT,
> 	};
> 
> 	[...]
> 
> 	if (static_branch_unlikely(&bar.key))
> 		foo = -1;
> 
> which doesn't follow the recommended API, and uses the internals
> of the static key implementation.
> 
> This patch introduces DECLARE_STATIC_KEY_TRUE/FALSE, as well as
> INIT_STATIC_KEY_TRUE/FALSE, which abstract such construct and
> allow the internals to evolve without having to fix everything else:
> 
> 	static struct {
> 		int			 foo;
> 		DECLARE_STATIC_KEY_FALSE(key);
> 	} bar = {
> 		INIT_STATIC_KEY_FALSE(.key),
> 	};

Hurm..

I think I like the first better, it looks more like actual C. Either way
around you need to now manually match up the type and initializer.

[toc] | [prev] | [next] | [standalone]

#1487187 — Re: [PATCH 1/2] jump_labels: Add API to deal with keys embedded in structures

On 20/09/16 10:42, Peter Zijlstra wrote:
> On Mon, Sep 19, 2016 at 06:21:27PM +0100, Marc Zyngier wrote:
>> It is desirable to allow static keys to be integrated in structures,
>> as it can lead do slightly more readable code. But the current API
>> only provides DEFINE_STATIC_KEY_TRUE/FALSE, which is not exactly
>> nice and leads to the following idiom:
>>
>> 	static struct {
>> 		int			foo;
>> 		struct static_key_false	key;
>> 	} bar = {
>> 		.key	= STATIC_KEY_FALSE_INIT,
>> 	};
>>
>> 	[...]
>>
>> 	if (static_branch_unlikely(&bar.key))
>> 		foo = -1;
>>
>> which doesn't follow the recommended API, and uses the internals
>> of the static key implementation.
>>
>> This patch introduces DECLARE_STATIC_KEY_TRUE/FALSE, as well as
>> INIT_STATIC_KEY_TRUE/FALSE, which abstract such construct and
>> allow the internals to evolve without having to fix everything else:
>>
>> 	static struct {
>> 		int			 foo;
>> 		DECLARE_STATIC_KEY_FALSE(key);
>> 	} bar = {
>> 		INIT_STATIC_KEY_FALSE(.key),
>> 	};
> 
> Hurm..
> 
> I think I like the first better, it looks more like actual C. Either way
> around you need to now manually match up the type and initializer.

Fair enough, I'll stick to that for the KVM code.

Thanks for the quick feedback.

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1487287 — Re: [PATCH 1/2] jump_labels: Add API to deal with keys embedded in structures

On Tue, Sep 20, 2016 at 11:42:23AM +0200, Peter Zijlstra wrote:
> On Mon, Sep 19, 2016 at 06:21:27PM +0100, Marc Zyngier wrote:
> > It is desirable to allow static keys to be integrated in structures,
> > as it can lead do slightly more readable code. But the current API
> > only provides DEFINE_STATIC_KEY_TRUE/FALSE, which is not exactly
> > nice and leads to the following idiom:
> > 
> > 	static struct {
> > 		int			foo;
> > 		struct static_key_false	key;
> > 	} bar = {
> > 		.key	= STATIC_KEY_FALSE_INIT,
> > 	};
> > 
> > 	[...]
> > 
> > 	if (static_branch_unlikely(&bar.key))
> > 		foo = -1;
> > 
> > which doesn't follow the recommended API, and uses the internals
> > of the static key implementation.
> > 
> > This patch introduces DECLARE_STATIC_KEY_TRUE/FALSE, as well as
> > INIT_STATIC_KEY_TRUE/FALSE, which abstract such construct and
> > allow the internals to evolve without having to fix everything else:
> > 
> > 	static struct {
> > 		int			 foo;
> > 		DECLARE_STATIC_KEY_FALSE(key);
> > 	} bar = {
> > 		INIT_STATIC_KEY_FALSE(.key),
> > 	};
> 
> Hurm..
> 
> I think I like the first better, it looks more like actual C. Either way
> around you need to now manually match up the type and initializer.
> 

It may have been one of my review comments the prompted these patches,
because from reading Documentation/static-keys.txt, it seems that
referencing 'struct static_key' directly should be deprecated, and
instead developers should use the update API replacements.

I wonder if it's worth slightly updating the documentation then?

Thanks,
-Christoffer

[toc] | [prev] | [next] | [standalone]

#1487300 — Re: [PATCH 1/2] jump_labels: Add API to deal with keys embedded in structures

On Tue, Sep 20, 2016 at 02:25:14PM +0200, Christoffer Dall wrote:
> On Tue, Sep 20, 2016 at 11:42:23AM +0200, Peter Zijlstra wrote:
> > On Mon, Sep 19, 2016 at 06:21:27PM +0100, Marc Zyngier wrote:
> > > It is desirable to allow static keys to be integrated in structures,
> > > as it can lead do slightly more readable code. But the current API
> > > only provides DEFINE_STATIC_KEY_TRUE/FALSE, which is not exactly
> > > nice and leads to the following idiom:
> > > 
> > > 	static struct {
> > > 		int			foo;
> > > 		struct static_key_false	key;
> > > 	} bar = {
> > > 		.key	= STATIC_KEY_FALSE_INIT,
> > > 	};
> > > 
> > > 	[...]
> > > 
> > > 	if (static_branch_unlikely(&bar.key))
> > > 		foo = -1;
> > > 
> > > which doesn't follow the recommended API, and uses the internals
> > > of the static key implementation.
> > > 
> > > This patch introduces DECLARE_STATIC_KEY_TRUE/FALSE, as well as
> > > INIT_STATIC_KEY_TRUE/FALSE, which abstract such construct and
> > > allow the internals to evolve without having to fix everything else:
> > > 
> > > 	static struct {
> > > 		int			 foo;
> > > 		DECLARE_STATIC_KEY_FALSE(key);
> > > 	} bar = {
> > > 		INIT_STATIC_KEY_FALSE(.key),
> > > 	};
> > 
> > Hurm..
> > 
> > I think I like the first better, it looks more like actual C. Either way
> > around you need to now manually match up the type and initializer.
> > 
> 
> It may have been one of my review comments the prompted these patches,
> because from reading Documentation/static-keys.txt, it seems that
> referencing 'struct static_key' directly should be deprecated, and
> instead developers should use the update API replacements.

'struct static_key' should indeed not be used and is deprecated. 'struct
static_key_{true,false}' however should be fine.

Part of the problem is naming, everything using 'struct static_key' has
_insane_ names and the API is utterly confusing. The other part is that
the new 2 type API simply has more functionality.

[toc] | [prev] | [next] | [standalone]

#1487314 — Re: [PATCH 1/2] jump_labels: Add API to deal with keys embedded in structures

On Tue, Sep 20, 2016 at 02:42:58PM +0200, Peter Zijlstra wrote:
> On Tue, Sep 20, 2016 at 02:25:14PM +0200, Christoffer Dall wrote:
> > On Tue, Sep 20, 2016 at 11:42:23AM +0200, Peter Zijlstra wrote:
> > > On Mon, Sep 19, 2016 at 06:21:27PM +0100, Marc Zyngier wrote:
> > > > It is desirable to allow static keys to be integrated in structures,
> > > > as it can lead do slightly more readable code. But the current API
> > > > only provides DEFINE_STATIC_KEY_TRUE/FALSE, which is not exactly
> > > > nice and leads to the following idiom:
> > > > 
> > > > 	static struct {
> > > > 		int			foo;
> > > > 		struct static_key_false	key;
> > > > 	} bar = {
> > > > 		.key	= STATIC_KEY_FALSE_INIT,
> > > > 	};
> > > > 
> > > > 	[...]
> > > > 
> > > > 	if (static_branch_unlikely(&bar.key))
> > > > 		foo = -1;
> > > > 
> > > > which doesn't follow the recommended API, and uses the internals
> > > > of the static key implementation.
> > > > 
> > > > This patch introduces DECLARE_STATIC_KEY_TRUE/FALSE, as well as
> > > > INIT_STATIC_KEY_TRUE/FALSE, which abstract such construct and
> > > > allow the internals to evolve without having to fix everything else:
> > > > 
> > > > 	static struct {
> > > > 		int			 foo;
> > > > 		DECLARE_STATIC_KEY_FALSE(key);
> > > > 	} bar = {
> > > > 		INIT_STATIC_KEY_FALSE(.key),
> > > > 	};
> > > 
> > > Hurm..
> > > 
> > > I think I like the first better, it looks more like actual C. Either way
> > > around you need to now manually match up the type and initializer.
> > > 
> > 
> > It may have been one of my review comments the prompted these patches,
> > because from reading Documentation/static-keys.txt, it seems that
> > referencing 'struct static_key' directly should be deprecated, and
> > instead developers should use the update API replacements.
> 
> 'struct static_key' should indeed not be used and is deprecated. 'struct
> static_key_{true,false}' however should be fine.

ah, ok, didn't realize this, especially since static_key_false() is also
listed as deprecated ;)

> 
> Part of the problem is naming, everything using 'struct static_key' has
> _insane_ names and the API is utterly confusing. The other part is that
> the new 2 type API simply has more functionality.

right, ok.  As long as you're happy with slightly increased use of
directly embeddeing struct static_key_{true,false}, we're good.

Sorry for both if I encouraged confusion here.

Thanks,
-Christoffer

[toc] | [prev] | [next] | [standalone]

#1488603 — Re: [PATCH 1/2] jump_labels: Add API to deal with keys embedded in structures

On Tue, Sep 20, 2016 at 02:54:01PM +0200, Christoffer Dall wrote:
> On Tue, Sep 20, 2016 at 02:42:58PM +0200, Peter Zijlstra wrote:
> > 'struct static_key' should indeed not be used and is deprecated. 'struct
> > static_key_{true,false}' however should be fine.
> 
> ah, ok, didn't realize this, especially since static_key_false() is also
> listed as deprecated ;)

Yeah, that's one of the horribly badly named things we wants to get rid
of :-) But note, its a function not a type.


> > Part of the problem is naming, everything using 'struct static_key' has
> > _insane_ names and the API is utterly confusing. The other part is that
> > the new 2 type API simply has more functionality.
> 
> right, ok.  As long as you're happy with slightly increased use of
> directly embeddeing struct static_key_{true,false}, we're good.
> 
> Sorry for both if I encouraged confusion here.

No problem, jump_labels have been cursed with confusion.

[toc] | [prev] | [next] | [standalone]

#1486701 — [PATCH 2/2] dynamic_debug: Use updated jump label API

Now that we have brand new DECLARE_STATIC_KEY_TRUE/FALSE and
INIT_STATIC_KEY_TRUE/FALSE as first class APIs, convert the
dynamic_debug infrastructure to use it.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 include/linux/dynamic_debug.h | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/include/linux/dynamic_debug.h b/include/linux/dynamic_debug.h
index 546d680..a180dd1 100644
--- a/include/linux/dynamic_debug.h
+++ b/include/linux/dynamic_debug.h
@@ -39,8 +39,8 @@ struct _ddebug {
 	unsigned int flags:8;
 #ifdef HAVE_JUMP_LABEL
 	union {
-		struct static_key_true dd_key_true;
-		struct static_key_false dd_key_false;
+		DECLARE_STATIC_KEY_TRUE(dd_key_true);
+		DECLARE_STATIC_KEY_FALSE(dd_key_false);
 	} key;
 #endif
 } __attribute__((aligned(8)));
@@ -70,7 +70,7 @@ void __dynamic_netdev_dbg(struct _ddebug *descriptor,
 			  const struct net_device *dev,
 			  const char *fmt, ...);
 
-#define DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, key, init)	\
+#define DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, dd_key_init)	\
 	static struct _ddebug  __aligned(8)			\
 	__attribute__((section("__verbose"))) name = {		\
 		.modname = KBUILD_MODNAME,			\
@@ -79,24 +79,22 @@ void __dynamic_netdev_dbg(struct _ddebug *descriptor,
 		.format = (fmt),				\
 		.lineno = __LINE__,				\
 		.flags = _DPRINTK_FLAGS_DEFAULT,		\
-		dd_key_init(key, init)				\
+		dd_key_init					\
 	}
 
 #ifdef HAVE_JUMP_LABEL
 
-#define dd_key_init(key, init) key = (init)
-
 #ifdef DEBUG
 #define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
-	DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, .key.dd_key_true, \
-					  (STATIC_KEY_TRUE_INIT))
+	DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt,			\
+					  INIT_STATIC_KEY_TRUE(.key.dd_key_true))
 
 #define DYNAMIC_DEBUG_BRANCH(descriptor) \
 	static_branch_likely(&descriptor.key.dd_key_true)
 #else
 #define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
-	DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, .key.dd_key_false, \
-					  (STATIC_KEY_FALSE_INIT))
+	DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt,			\
+					  INIT_STATIC_KEY_FALSE(.key.dd_key_false))
 
 #define DYNAMIC_DEBUG_BRANCH(descriptor) \
 	static_branch_unlikely(&descriptor.key.dd_key_false)
@@ -104,10 +102,10 @@ void __dynamic_netdev_dbg(struct _ddebug *descriptor,
 
 #else
 
-#define dd_key_init(key, init)
+#define dd_key_do_nothing
 
 #define DEFINE_DYNAMIC_DEBUG_METADATA(name, fmt) \
-	DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, 0, 0)
+	DEFINE_DYNAMIC_DEBUG_METADATA_KEY(name, fmt, dd_key_do_nothing)
 
 #ifdef DEBUG
 #define DYNAMIC_DEBUG_BRANCH(descriptor) \
-- 
2.1.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web