Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, 4 Aug 2015 16:33:44 +0200 Borislav Petkov wrote: > Hell, we can drop that "key" or "branch" from the whole API for all I > know. "static_" is enough for me to say what the thing is. But that's > just me - I like short names - no poems in code and sh*t. > > Thoughts, comments? I agree to get rid of the "key" and the "branch", I never liked them in the first place. I prefer static_set_true/false() but I'm also fine with static_enable/disable() as long as the initializers are consistent. > > So yeah, I absolutely see the problematic here and also the need for > more bikeshedding. And this time, that bikeshedding is important. > Right, because the broken part of this nuclear plant, was the bike shed, and it's color was so ugly that the people in the nuclear plant kept making mistakes by being distracted by that damn shed! -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, Aug 4, 2015 at 7:33 AM, Borislav Petkov wrote: > On Tue, Aug 04, 2015 at 08:06:45AM -0400, Steven Rostedt wrote: >> I just don't like the inconsistency of the initialization and the >> setting. >> >> Either have: >> >> DEFINE_STATIC_KEY_TRUE() >> DEFINE_STATIC_KEY_FALSE() >> >> and >> >> static_branch_set_true() >> static_branch_set_false() >> >> >> or have: >> >> DEFINE_STATIC_KEY_ENABLED() >> DEFINE_STATIC_KEY_DISABLED() >> >> and >> >> static_branch_enable() >> static_branch_disable() >> >> >> But having the DEFINE_STATIC_KEY_TRUE() and static_branch_enable() is >> confusing, as enable does not mean "make true"! >> >> This may seem as bike shedding, but terminology *is* important, and >> being inconsistent just makes it more probable to have bugs. > > I absolutely agree but I read "enable" as enable the branch, so no > confusion there. Now, it's a whole another question where we branch to. > And that can be confusing. > > Now, let's get back to our example: > > +static DEFINE_STATIC_KEY_FALSE(__use_tsc); > > We don't use the TSC by default. And that's correct, we need to > calibrate it first. > > After calibration: > > + static_branch_enable(&__use_tsc); > > Now here we can get confused: we enable the branch but where we branch > to? The key name helps here but it is still not quite 100% clear. I'd > prefer to have: > > static_enable(&__use_tsc); If everything's consistent about "static_key", then I still like "static_key_set_true" or "static_key_set". "static_key_enable" is okay but not fantastic IMO, and "static_branch_enable" is just confusing. --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, Aug 04, 2015 at 08:06:45AM -0400, Steven Rostedt wrote:
> I just don't like the inconsistency of the initialization and the
> setting.
>
> Either have:
>
> DEFINE_STATIC_KEY_TRUE()
> DEFINE_STATIC_KEY_FALSE()
>
> and
>
> static_branch_set_true()
> static_branch_set_false()
>
>
> or have:
>
> DEFINE_STATIC_KEY_ENABLED()
> DEFINE_STATIC_KEY_DISABLED()
>
> and
>
> static_branch_enable()
> static_branch_disable()
>
>
> But having the DEFINE_STATIC_KEY_TRUE() and static_branch_enable() is
> confusing, as enable does not mean "make true"!
>
> This may seem as bike shedding, but terminology *is* important, and
> being inconsistent just makes it more probable to have bugs.
I absolutely agree but I read "enable" as enable the branch, so no
confusion there. Now, it's a whole another question where we branch to.
And that can be confusing.
Now, let's get back to our example:
+static DEFINE_STATIC_KEY_FALSE(__use_tsc);
We don't use the TSC by default. And that's correct, we need to
calibrate it first.
After calibration:
+ static_branch_enable(&__use_tsc);
Now here we can get confused: we enable the branch but where we branch
to? The key name helps here but it is still not quite 100% clear. I'd
prefer to have:
static_enable(&__use_tsc);
which basically says, we can use the TSC from now on. No branch, no key,
no nada. It looks like a boolean variable of sorts which says, use the
TSC from now on.
Which equally speaks for your other version:
static_set_true(&__use_tsc);
Now this looks pretty understandable to me.
Then, the usage site looks like this:
+ if (static_likely(&__use_tsc)) {
+ u64 tsc_now = rdtsc();
+
+ /* return the value in ns */
+ return cycles_2_ns(tsc_now);
+ }
which basically says two things:
* if the static key is enabled, i.e. the boolean var is set to true.
and
* this is a likely key, i.e., the code in brackets should come first in
the layout and the code we branch to comes later.
Hell, we can drop that "key" or "branch" from the whole API for all I
know. "static_" is enough for me to say what the thing is. But that's
just me - I like short names - no poems in code and sh*t.
Thoughts, comments?
So yeah, I absolutely see the problematic here and also the need for
more bikeshedding. And this time, that bikeshedding is important.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, 4 Aug 2015 05:37:33 +0200 Borislav Petkov wrote: > On Mon, Aug 03, 2015 at 05:57:57PM -0400, Steven Rostedt wrote: > > That's implementation details, not a general concept that users will > > need to know about. > > Why? > > It is a branch, regardless of which insn is used on which arch - it is > either active and you *branch* to that code or *inactive* and you don't. > So now it is actually what it should've been from the beginning... I just don't like the inconsistency of the initialization and the setting. Either have: DEFINE_STATIC_KEY_TRUE() DEFINE_STATIC_KEY_FALSE() and static_branch_set_true() static_branch_set_false() or have: DEFINE_STATIC_KEY_ENABLED() DEFINE_STATIC_KEY_DISABLED() and static_branch_enable() static_branch_disable() But having the DEFINE_STATIC_KEY_TRUE() and static_branch_enable() is confusing, as enable does not mean "make true"! This may seem as bike shedding, but terminology *is* important, and being inconsistent just makes it more probable to have bugs. -- Steve > > I realize simplifying the terminology around those jump labels/static > branches things comes kinda unnatural now. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, Aug 04, 2015 at 02:50:32PM +0800, yalin wang wrote:
> is this means static_key_{true,false}() are deprecated ?
Yes.
> do you need mark static_key_{true,false}() as deprecated?
> like this:
> static __always_inline __deprecated bool static_key_false(struct static_key
> *key) ?
Not until I (or someone else) has made an effort to convert most (if not
all) current users of that interface.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
> On Jul 28, 2015, at 21:21, Peter Zijlstra wrote:
>
> There are various problems and short-comings with the current
> static_key interface:
>
> - static_key_{true,false}() read like a branch depending on the key
> value, instead of the actual likely/unlikely branch depending on
> init value.
>
> - static_key_{true,false}() are, as stated above, tied to the
> static_key init values STATIC_KEY_INIT_{TRUE,FALSE}.
>
> - we're limited to the 2 (out of 4) possible options that compile to
> a default NOP because that's what our arch_static_branch() assembly
> emits.
>
> So provide a new static_key interface:
>
> DEFINE_STATIC_KEY_TRUE(name);
> DEFINE_STATIC_KEY_FALSE(name);
>
> Which define a key of different types with an initial true/false
> value.
>
> Then allow:
>
> static_branch_likely()
> static_branch_unlikely()
>
> to take a key of either type and emit the right instruction for the
> case.
>
> This means adding a second arch_static_branch_jump() assembly helper
> which emits a JMP per default.
>
> In order to determine the right instruction for the right state,
> encode the branch type in the LSB of jump_entry::key.
>
> Signed-off-by: Peter Zijlstra (Intel)
> ---
>
is this means static_key_{true,false}() are deprecated ?
do you need mark static_key_{true,false}() as deprecated?
like this:
static __always_inline __deprecated bool static_key_false(struct static_key
*key) ?
Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, 4 Aug 2015 05:37:33 +0200 Borislav Petkov b...@alien8.de wrote: On Mon, Aug 03, 2015 at 05:57:57PM -0400, Steven Rostedt wrote: That's implementation details, not a general concept that users will need to know about. Why? It is a branch, regardless of which insn is used on which arch - it is either active and you *branch* to that code or *inactive* and you don't. So now it is actually what it should've been from the beginning... I just don't like the inconsistency of the initialization and the setting. Either have: DEFINE_STATIC_KEY_TRUE() DEFINE_STATIC_KEY_FALSE() and static_branch_set_true() static_branch_set_false() or have: DEFINE_STATIC_KEY_ENABLED() DEFINE_STATIC_KEY_DISABLED() and static_branch_enable() static_branch_disable() But having the DEFINE_STATIC_KEY_TRUE() and static_branch_enable() is confusing, as enable does not mean make true! This may seem as bike shedding, but terminology *is* important, and being inconsistent just makes it more probable to have bugs. -- Steve I realize simplifying the terminology around those jump labels/static branches things comes kinda unnatural now. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, Aug 04, 2015 at 02:50:32PM +0800, yalin wang wrote:
is this means static_key_{true,false}() are deprecated ?
Yes.
do you need mark static_key_{true,false}() as deprecated?
like this:
static __always_inline __deprecated bool static_key_false(struct static_key
*key) ?
Not until I (or someone else) has made an effort to convert most (if not
all) current users of that interface.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Jul 28, 2015, at 21:21, Peter Zijlstra pet...@infradead.org wrote:
There are various problems and short-comings with the current
static_key interface:
- static_key_{true,false}() read like a branch depending on the key
value, instead of the actual likely/unlikely branch depending on
init value.
- static_key_{true,false}() are, as stated above, tied to the
static_key init values STATIC_KEY_INIT_{TRUE,FALSE}.
- we're limited to the 2 (out of 4) possible options that compile to
a default NOP because that's what our arch_static_branch() assembly
emits.
So provide a new static_key interface:
DEFINE_STATIC_KEY_TRUE(name);
DEFINE_STATIC_KEY_FALSE(name);
Which define a key of different types with an initial true/false
value.
Then allow:
static_branch_likely()
static_branch_unlikely()
to take a key of either type and emit the right instruction for the
case.
This means adding a second arch_static_branch_jump() assembly helper
which emits a JMP per default.
In order to determine the right instruction for the right state,
encode the branch type in the LSB of jump_entry::key.
Signed-off-by: Peter Zijlstra (Intel) pet...@infradead.org
---
is this means static_key_{true,false}() are deprecated ?
do you need mark static_key_{true,false}() as deprecated?
like this:
static __always_inline __deprecated bool static_key_false(struct static_key
*key) ?
Thanks
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, Aug 4, 2015 at 7:33 AM, Borislav Petkov b...@alien8.de wrote: On Tue, Aug 04, 2015 at 08:06:45AM -0400, Steven Rostedt wrote: I just don't like the inconsistency of the initialization and the setting. Either have: DEFINE_STATIC_KEY_TRUE() DEFINE_STATIC_KEY_FALSE() and static_branch_set_true() static_branch_set_false() or have: DEFINE_STATIC_KEY_ENABLED() DEFINE_STATIC_KEY_DISABLED() and static_branch_enable() static_branch_disable() But having the DEFINE_STATIC_KEY_TRUE() and static_branch_enable() is confusing, as enable does not mean make true! This may seem as bike shedding, but terminology *is* important, and being inconsistent just makes it more probable to have bugs. I absolutely agree but I read enable as enable the branch, so no confusion there. Now, it's a whole another question where we branch to. And that can be confusing. Now, let's get back to our example: +static DEFINE_STATIC_KEY_FALSE(__use_tsc); We don't use the TSC by default. And that's correct, we need to calibrate it first. After calibration: + static_branch_enable(__use_tsc); Now here we can get confused: we enable the branch but where we branch to? The key name helps here but it is still not quite 100% clear. I'd prefer to have: static_enable(__use_tsc); If everything's consistent about static_key, then I still like static_key_set_true or static_key_set. static_key_enable is okay but not fantastic IMO, and static_branch_enable is just confusing. --Andy -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, Aug 04, 2015 at 08:06:45AM -0400, Steven Rostedt wrote:
I just don't like the inconsistency of the initialization and the
setting.
Either have:
DEFINE_STATIC_KEY_TRUE()
DEFINE_STATIC_KEY_FALSE()
and
static_branch_set_true()
static_branch_set_false()
or have:
DEFINE_STATIC_KEY_ENABLED()
DEFINE_STATIC_KEY_DISABLED()
and
static_branch_enable()
static_branch_disable()
But having the DEFINE_STATIC_KEY_TRUE() and static_branch_enable() is
confusing, as enable does not mean make true!
This may seem as bike shedding, but terminology *is* important, and
being inconsistent just makes it more probable to have bugs.
I absolutely agree but I read enable as enable the branch, so no
confusion there. Now, it's a whole another question where we branch to.
And that can be confusing.
Now, let's get back to our example:
+static DEFINE_STATIC_KEY_FALSE(__use_tsc);
We don't use the TSC by default. And that's correct, we need to
calibrate it first.
After calibration:
+ static_branch_enable(__use_tsc);
Now here we can get confused: we enable the branch but where we branch
to? The key name helps here but it is still not quite 100% clear. I'd
prefer to have:
static_enable(__use_tsc);
which basically says, we can use the TSC from now on. No branch, no key,
no nada. It looks like a boolean variable of sorts which says, use the
TSC from now on.
Which equally speaks for your other version:
static_set_true(__use_tsc);
Now this looks pretty understandable to me.
Then, the usage site looks like this:
+ if (static_likely(__use_tsc)) {
+ u64 tsc_now = rdtsc();
+
+ /* return the value in ns */
+ return cycles_2_ns(tsc_now);
+ }
which basically says two things:
* if the static key is enabled, i.e. the boolean var is set to true.
and
* this is a likely key, i.e., the code in brackets should come first in
the layout and the code we branch to comes later.
Hell, we can drop that key or branch from the whole API for all I
know. static_ is enough for me to say what the thing is. But that's
just me - I like short names - no poems in code and sh*t.
Thoughts, comments?
So yeah, I absolutely see the problematic here and also the need for
more bikeshedding. And this time, that bikeshedding is important.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, 4 Aug 2015 16:33:44 +0200 Borislav Petkov b...@alien8.de wrote: Hell, we can drop that key or branch from the whole API for all I know. static_ is enough for me to say what the thing is. But that's just me - I like short names - no poems in code and sh*t. Thoughts, comments? I agree to get rid of the key and the branch, I never liked them in the first place. I prefer static_set_true/false() but I'm also fine with static_enable/disable() as long as the initializers are consistent. So yeah, I absolutely see the problematic here and also the need for more bikeshedding. And this time, that bikeshedding is important. Right, because the broken part of this nuclear plant, was the bike shed, and it's color was so ugly that the people in the nuclear plant kept making mistakes by being distracted by that damn shed! -- Steve -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Mon, Aug 03, 2015 at 09:07:53PM -0700, Andy Lutomirski wrote:
> Except that, with the new interface, static_key_likely is the other
> way around, right? If the key is true (i.e. enabled), then it doesn't
> branch.
>
> I think of the key as a boolean thing that happens to work by code
> patching under the hood. The fancy patching affects the performance
> but doesn't really make it functionally different from a regular
> variable. How about making it extra explicit:
>
> static_key_set(, value);
>
> where value is a bool or maybe even an unsigned int?
Let's have an actual example:
+ if (static_branch_likely(&__use_tsc)) {
+ u64 tsc_now = rdtsc();
+
+ /* return the value in ns */
+ return cycles_2_ns(tsc_now);
+ }
Well, I can see how the likely/unlikely things can confuse. They
actually don't have anything to do with where we will branch to but how
the code will be laid out, AFAICT. So I'm reading this as:
if (use_tsc)) {
RDTSC;
return;
}
and then it is straightforward.
So in this case, the jump will be disabled and we won't branch anywhere.
It actually becomes:
RDTSC;
return;
which can't get any more optimal than it is.
Hmm, yeah, I see how that can be confusing... But the asm is finally
fine. Hey, at least one thing...
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Mon, Aug 3, 2015 at 8:37 PM, Borislav Petkov wrote: > On Mon, Aug 03, 2015 at 05:57:57PM -0400, Steven Rostedt wrote: >> That's implementation details, not a general concept that users will >> need to know about. > > Why? > > It is a branch, regardless of which insn is used on which arch - it is > either active and you *branch* to that code or *inactive* and you don't. > So now it is actually what it should've been from the beginning... Except that, with the new interface, static_key_likely is the other way around, right? If the key is true (i.e. enabled), then it doesn't branch. I think of the key as a boolean thing that happens to work by code patching under the hood. The fancy patching affects the performance but doesn't really make it functionally different from a regular variable. How about making it extra explicit: static_key_set(, value); where value is a bool or maybe even an unsigned int? --Andy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Mon, Aug 03, 2015 at 05:57:57PM -0400, Steven Rostedt wrote: > That's implementation details, not a general concept that users will > need to know about. Why? It is a branch, regardless of which insn is used on which arch - it is either active and you *branch* to that code or *inactive* and you don't. So now it is actually what it should've been from the beginning... I realize simplifying the terminology around those jump labels/static branches things comes kinda unnatural now. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Mon, 3 Aug 2015 22:00:02 +0200
Peter Zijlstra wrote:
> On Mon, Aug 03, 2015 at 03:28:10PM -0400, Steven Rostedt wrote:
> > Technically, one can think: "activate the branch", but we are
> > activating not the branch, but the jump label itself.
>
> No you are enabling the branch, you're making the branch body active.
By making the statement "true".
Otherwise we could just have:
static_branch_likely() {
[..]
}
And remove the "if".
Then it would make sense to enable or disable it.
>
> There is no enable/disable/true/false for the jump label, only NOP or
> JUMP, and either can result in an active branch body.
That's implementation details, not a general concept that users will
need to know about.
-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Mon, Aug 03, 2015 at 03:28:10PM -0400, Steven Rostedt wrote: > Technically, one can think: "activate the branch", but we are > activating not the branch, but the jump label itself. No you are enabling the branch, you're making the branch body active. There is no enable/disable/true/false for the jump label, only NOP or JUMP, and either can result in an active branch body. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Mon, 3 Aug 2015 21:18:16 +0200 Peter Zijlstra wrote: > On Mon, Aug 03, 2015 at 03:03:59PM -0400, Steven Rostedt wrote: > > > I wonder if static_branch_set_false() would be a better name to > > understand. What does "disable" / "enable" mean? > > "make false" / "make true" ? Check a local dictionary. > > http://lmgtfy.com/?q=enable I know the definition on enable :-p > > "2. computing: make (a device or system) operational; active" > > A value can be true/false, an action that makes true/false is > enable/disable. enable is more "activate" and disable is more "deactivate" not "make true" and "make false". It's subtle, but there is a difference. Try switching it around in other contexts. One could "disable networking" but saying "make networking false" doesn't make sense. Technically, one can think: "activate the branch", but we are activating not the branch, but the jump label itself. It's not as clear as setting it to "true" or "false". What the static_branch does is already confusing enough, we should try to use the terminology that is as clear as possible. "set_true" is more understandable than "enable" when one can question, what exactly are we "enabling"? -- Steve -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Mon, Aug 03, 2015 at 03:03:59PM -0400, Steven Rostedt wrote: > I wonder if static_branch_set_false() would be a better name to > understand. What does "disable" / "enable" mean? "make false" / "make true" ? Check a local dictionary. http://lmgtfy.com/?q=enable "2. computing: make (a device or system) operational; active" A value can be true/false, an action that makes true/false is enable/disable. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Wed, 29 Jul 2015 10:49:06 +0200
Peter Zijlstra wrote:
> On Wed, Jul 29, 2015 at 09:19:22AM +0200, Vlastimil Babka wrote:
>
> > How would one define a static key that's e.g. expected to be mostly false,
> > but
> > with initial value of true, e.g. during boot?
>
> DEFINE_STATIC_KEY_TRUE(blah);
>
> will get you the true at boot time.
>
> You'll then want to use:
>
> if (static_branch_unlikely()) {
> /* code that mostly doesn't happen */
> }
>
> To indicate you expect it to be false most of the time. And you'll flip
> it to false at runtime using:
>
> static_branch_disable();
I wonder if static_branch_set_false() would be a better name to
understand. What does "disable" / "enable" mean?
If we declare it "TRUE" when defining it, it only makes sense to change
it to "false" later on.
-- Steve
>
> If GCC co-operates, the body of the branch will be placed out-of-line,
> we'll emit a jump to it by default, but once you disable it, we'll nop
> the jump and fall straight through.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Mon, 3 Aug 2015 22:00:02 +0200
Peter Zijlstra pet...@infradead.org wrote:
On Mon, Aug 03, 2015 at 03:28:10PM -0400, Steven Rostedt wrote:
Technically, one can think: activate the branch, but we are
activating not the branch, but the jump label itself.
No you are enabling the branch, you're making the branch body active.
By making the statement true.
Otherwise we could just have:
static_branch_likely(blah) {
[..]
}
And remove the if.
Then it would make sense to enable or disable it.
There is no enable/disable/true/false for the jump label, only NOP or
JUMP, and either can result in an active branch body.
That's implementation details, not a general concept that users will
need to know about.
-- Steve
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Mon, Aug 03, 2015 at 09:07:53PM -0700, Andy Lutomirski wrote:
Except that, with the new interface, static_key_likely is the other
way around, right? If the key is true (i.e. enabled), then it doesn't
branch.
I think of the key as a boolean thing that happens to work by code
patching under the hood. The fancy patching affects the performance
but doesn't really make it functionally different from a regular
variable. How about making it extra explicit:
static_key_set(key, value);
where value is a bool or maybe even an unsigned int?
Let's have an actual example:
+ if (static_branch_likely(__use_tsc)) {
+ u64 tsc_now = rdtsc();
+
+ /* return the value in ns */
+ return cycles_2_ns(tsc_now);
+ }
Well, I can see how the likely/unlikely things can confuse. They
actually don't have anything to do with where we will branch to but how
the code will be laid out, AFAICT. So I'm reading this as:
if (use_tsc)) {
RDTSC;
return;
}
and then it is straightforward.
So in this case, the jump will be disabled and we won't branch anywhere.
It actually becomes:
RDTSC;
return;
which can't get any more optimal than it is.
Hmm, yeah, I see how that can be confusing... But the asm is finally
fine. Hey, at least one thing...
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Mon, Aug 3, 2015 at 8:37 PM, Borislav Petkov b...@alien8.de wrote: On Mon, Aug 03, 2015 at 05:57:57PM -0400, Steven Rostedt wrote: That's implementation details, not a general concept that users will need to know about. Why? It is a branch, regardless of which insn is used on which arch - it is either active and you *branch* to that code or *inactive* and you don't. So now it is actually what it should've been from the beginning... Except that, with the new interface, static_key_likely is the other way around, right? If the key is true (i.e. enabled), then it doesn't branch. I think of the key as a boolean thing that happens to work by code patching under the hood. The fancy patching affects the performance but doesn't really make it functionally different from a regular variable. How about making it extra explicit: static_key_set(key, value); where value is a bool or maybe even an unsigned int? --Andy -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Mon, Aug 03, 2015 at 05:57:57PM -0400, Steven Rostedt wrote: That's implementation details, not a general concept that users will need to know about. Why? It is a branch, regardless of which insn is used on which arch - it is either active and you *branch* to that code or *inactive* and you don't. So now it is actually what it should've been from the beginning... I realize simplifying the terminology around those jump labels/static branches things comes kinda unnatural now. -- Regards/Gruss, Boris. ECO tip #101: Trim your mails when you reply. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Wed, 29 Jul 2015 10:49:06 +0200
Peter Zijlstra pet...@infradead.org wrote:
On Wed, Jul 29, 2015 at 09:19:22AM +0200, Vlastimil Babka wrote:
How would one define a static key that's e.g. expected to be mostly false,
but
with initial value of true, e.g. during boot?
DEFINE_STATIC_KEY_TRUE(blah);
will get you the true at boot time.
You'll then want to use:
if (static_branch_unlikely(blah)) {
/* code that mostly doesn't happen */
}
To indicate you expect it to be false most of the time. And you'll flip
it to false at runtime using:
static_branch_disable(blah);
I wonder if static_branch_set_false(blah) would be a better name to
understand. What does disable / enable mean?
If we declare it TRUE when defining it, it only makes sense to change
it to false later on.
-- Steve
If GCC co-operates, the body of the branch will be placed out-of-line,
we'll emit a jump to it by default, but once you disable it, we'll nop
the jump and fall straight through.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Mon, Aug 03, 2015 at 03:03:59PM -0400, Steven Rostedt wrote: I wonder if static_branch_set_false(blah) would be a better name to understand. What does disable / enable mean? make false / make true ? Check a local dictionary. http://lmgtfy.com/?q=enable 2. computing: make (a device or system) operational; active A value can be true/false, an action that makes true/false is enable/disable. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Mon, 3 Aug 2015 21:18:16 +0200 Peter Zijlstra pet...@infradead.org wrote: On Mon, Aug 03, 2015 at 03:03:59PM -0400, Steven Rostedt wrote: I wonder if static_branch_set_false(blah) would be a better name to understand. What does disable / enable mean? make false / make true ? Check a local dictionary. http://lmgtfy.com/?q=enable I know the definition on enable :-p 2. computing: make (a device or system) operational; active A value can be true/false, an action that makes true/false is enable/disable. enable is more activate and disable is more deactivate not make true and make false. It's subtle, but there is a difference. Try switching it around in other contexts. One could disable networking but saying make networking false doesn't make sense. Technically, one can think: activate the branch, but we are activating not the branch, but the jump label itself. It's not as clear as setting it to true or false. What the static_branch does is already confusing enough, we should try to use the terminology that is as clear as possible. set_true is more understandable than enable when one can question, what exactly are we enabling? -- Steve -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Mon, Aug 03, 2015 at 03:28:10PM -0400, Steven Rostedt wrote: Technically, one can think: activate the branch, but we are activating not the branch, but the jump label itself. No you are enabling the branch, you're making the branch body active. There is no enable/disable/true/false for the jump label, only NOP or JUMP, and either can result in an active branch body. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, 2015-07-28 at 15:21 +0200, Peter Zijlstra wrote: > There are various problems and short-comings with the current > static_key interface: ... > --- > arch/powerpc/include/asm/jump_label.h | 19 This looks sane and seems to be working, so powerpc bits: Acked-by: Michael Ellerman cheers -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, 2015-07-28 at 15:21 +0200, Peter Zijlstra wrote: There are various problems and short-comings with the current static_key interface: ... --- arch/powerpc/include/asm/jump_label.h | 19 This looks sane and seems to be working, so powerpc bits: Acked-by: Michael Ellerman m...@ellerman.id.au cheers -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Wed, Jul 29, 2015 at 09:19:22AM +0200, Vlastimil Babka wrote:
> How would one define a static key that's e.g. expected to be mostly false, but
> with initial value of true, e.g. during boot?
DEFINE_STATIC_KEY_TRUE(blah);
will get you the true at boot time.
You'll then want to use:
if (static_branch_unlikely()) {
/* code that mostly doesn't happen */
}
To indicate you expect it to be false most of the time. And you'll flip
it to false at runtime using:
static_branch_disable();
If GCC co-operates, the body of the branch will be placed out-of-line,
we'll emit a jump to it by default, but once you disable it, we'll nop
the jump and fall straight through.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On 07/28/2015 03:21 PM, Peter Zijlstra wrote:
> +/*
> -- */
> +
> +/*
> + * Two type wrappers around static_key, such that we can use compile time
> + * type differentiation to emit the right code.
> + *
> + * All the below code is macros in order to play type games.
> + */
> +
> +struct static_key_true {
> + struct static_key key;
> +};
> +
> +struct static_key_false {
> + struct static_key key;
> +};
> +
> +#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, }
How would one define a static key that's e.g. expected to be mostly false, but
with initial value of true, e.g. during boot?
Is the following legal? Should there be an API wrapper as well?
(struct static_key_false){ .key = STATIC_KEY_INIT_TRUE, }
Otherwise the new API seems like a big improvement, thanks :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, Jul 28, 2015 at 03:21:01PM +0200, Peter Zijlstra wrote:
> There are various problems and short-comings with the current
> static_key interface:
>
> - static_key_{true,false}() read like a branch depending on the key
>value, instead of the actual likely/unlikely branch depending on
>init value.
>
> - static_key_{true,false}() are, as stated above, tied to the
>static_key init values STATIC_KEY_INIT_{TRUE,FALSE}.
>
> - we're limited to the 2 (out of 4) possible options that compile to
>a default NOP because that's what our arch_static_branch() assembly
>emits.
>
> So provide a new static_key interface:
>
> DEFINE_STATIC_KEY_TRUE(name);
> DEFINE_STATIC_KEY_FALSE(name);
>
> Which define a key of different types with an initial true/false
> value.
>
> Then allow:
>
>static_branch_likely()
>static_branch_unlikely()
>
> to take a key of either type and emit the right instruction for the
> case.
>
> This means adding a second arch_static_branch_jump() assembly helper
> which emits a JMP per default.
>
> In order to determine the right instruction for the right state,
> encode the branch type in the LSB of jump_entry::key.
>
> Signed-off-by: Peter Zijlstra (Intel)
> ---
> arch/arm/include/asm/jump_label.h | 24 +++--
> arch/arm64/include/asm/jump_label.h | 18 +++-
> arch/mips/include/asm/jump_label.h| 19
> arch/powerpc/include/asm/jump_label.h | 19
> arch/s390/include/asm/jump_label.h| 19
> arch/sparc/include/asm/jump_label.h | 35 ++--
> arch/x86/include/asm/jump_label.h | 21
> include/linux/jump_label.h| 143
> --
> kernel/jump_label.c | 35 ++--
> 9 files changed, 294 insertions(+), 39 deletions(-)
for the s390 part:
Acked-by: Heiko Carstens
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, Jul 28, 2015 at 03:21:01PM +0200, Peter Zijlstra wrote:
There are various problems and short-comings with the current
static_key interface:
- static_key_{true,false}() read like a branch depending on the key
value, instead of the actual likely/unlikely branch depending on
init value.
- static_key_{true,false}() are, as stated above, tied to the
static_key init values STATIC_KEY_INIT_{TRUE,FALSE}.
- we're limited to the 2 (out of 4) possible options that compile to
a default NOP because that's what our arch_static_branch() assembly
emits.
So provide a new static_key interface:
DEFINE_STATIC_KEY_TRUE(name);
DEFINE_STATIC_KEY_FALSE(name);
Which define a key of different types with an initial true/false
value.
Then allow:
static_branch_likely()
static_branch_unlikely()
to take a key of either type and emit the right instruction for the
case.
This means adding a second arch_static_branch_jump() assembly helper
which emits a JMP per default.
In order to determine the right instruction for the right state,
encode the branch type in the LSB of jump_entry::key.
Signed-off-by: Peter Zijlstra (Intel) pet...@infradead.org
---
arch/arm/include/asm/jump_label.h | 24 +++--
arch/arm64/include/asm/jump_label.h | 18 +++-
arch/mips/include/asm/jump_label.h| 19
arch/powerpc/include/asm/jump_label.h | 19
arch/s390/include/asm/jump_label.h| 19
arch/sparc/include/asm/jump_label.h | 35 ++--
arch/x86/include/asm/jump_label.h | 21
include/linux/jump_label.h| 143
--
kernel/jump_label.c | 35 ++--
9 files changed, 294 insertions(+), 39 deletions(-)
for the s390 part:
Acked-by: Heiko Carstens heiko.carst...@de.ibm.com
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Wed, Jul 29, 2015 at 09:19:22AM +0200, Vlastimil Babka wrote:
How would one define a static key that's e.g. expected to be mostly false, but
with initial value of true, e.g. during boot?
DEFINE_STATIC_KEY_TRUE(blah);
will get you the true at boot time.
You'll then want to use:
if (static_branch_unlikely(blah)) {
/* code that mostly doesn't happen */
}
To indicate you expect it to be false most of the time. And you'll flip
it to false at runtime using:
static_branch_disable(blah);
If GCC co-operates, the body of the branch will be placed out-of-line,
we'll emit a jump to it by default, but once you disable it, we'll nop
the jump and fall straight through.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On 07/28/2015 03:21 PM, Peter Zijlstra wrote:
+/*
-- */
+
+/*
+ * Two type wrappers around static_key, such that we can use compile time
+ * type differentiation to emit the right code.
+ *
+ * All the below code is macros in order to play type games.
+ */
+
+struct static_key_true {
+ struct static_key key;
+};
+
+struct static_key_false {
+ struct static_key key;
+};
+
+#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, }
How would one define a static key that's e.g. expected to be mostly false, but
with initial value of true, e.g. during boot?
Is the following legal? Should there be an API wrapper as well?
(struct static_key_false){ .key = STATIC_KEY_INIT_TRUE, }
Otherwise the new API seems like a big improvement, thanks :)
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, Jul 28, 2015 at 07:00:55PM +0200, Rabin Vincent wrote: > > This is missing an include of asm/unified.h for the WASM(): > > diff --git a/arch/arm/include/asm/jump_label.h > b/arch/arm/include/asm/jump_label.h > index f8bc12f..34f7b69 100644 > --- a/arch/arm/include/asm/jump_label.h > +++ b/arch/arm/include/asm/jump_label.h > @@ -4,6 +4,7 @@ > #ifndef __ASSEMBLY__ > > #include > +#include > > #define JUMP_LABEL_NOP_SIZE 4 > > With that added, it builds and works fine on ARM. Duh, thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, Jul 28, 2015 at 03:21:01PM +0200, Peter Zijlstra wrote:
> --- a/arch/arm/include/asm/jump_label.h
> +++ b/arch/arm/include/asm/jump_label.h
> @@ -7,20 +7,28 @@
>
> #define JUMP_LABEL_NOP_SIZE 4
>
> -#ifdef CONFIG_THUMB2_KERNEL
> -#define JUMP_LABEL_NOP "nop.w"
> -#else
> -#define JUMP_LABEL_NOP "nop"
> -#endif
> +static __always_inline bool arch_static_branch(struct static_key *key, bool
> branch)
> +{
> + asm_volatile_goto("1:\n\t"
> + WASM(nop) "\n\t"
> + ".pushsection __jump_table, \"aw\"\n\t"
> + ".word 1b, %l[l_yes], %c0\n\t"
> + ".popsection\n\t"
> + : : "i" (&((char *)key)[branch]) : : l_yes);
> +
> + return false;
> +l_yes:
> + return true;
> +}
>
> -static __always_inline bool arch_static_branch(struct static_key *key)
> +static __always_inline bool arch_static_branch_jump(struct static_key *key,
> bool branch)
> {
> asm_volatile_goto("1:\n\t"
> - JUMP_LABEL_NOP "\n\t"
> + WASM(b) " %l[l_yes]\n\t"
>".pushsection __jump_table, \"aw\"\n\t"
>".word 1b, %l[l_yes], %c0\n\t"
>".popsection\n\t"
> - : : "i" (key) : : l_yes);
> + : : "i" (&((char *)key)[branch]) : : l_yes);
>
> return false;
> l_yes:
This is missing an include of asm/unified.h for the WASM():
diff --git a/arch/arm/include/asm/jump_label.h
b/arch/arm/include/asm/jump_label.h
index f8bc12f..34f7b69 100644
--- a/arch/arm/include/asm/jump_label.h
+++ b/arch/arm/include/asm/jump_label.h
@@ -4,6 +4,7 @@
#ifndef __ASSEMBLY__
#include
+#include
#define JUMP_LABEL_NOP_SIZE 4
With that added, it builds and works fine on ARM.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH -v2 6/8] jump_label: Add a new static_key interface
There are various problems and short-comings with the current
static_key interface:
- static_key_{true,false}() read like a branch depending on the key
value, instead of the actual likely/unlikely branch depending on
init value.
- static_key_{true,false}() are, as stated above, tied to the
static_key init values STATIC_KEY_INIT_{TRUE,FALSE}.
- we're limited to the 2 (out of 4) possible options that compile to
a default NOP because that's what our arch_static_branch() assembly
emits.
So provide a new static_key interface:
DEFINE_STATIC_KEY_TRUE(name);
DEFINE_STATIC_KEY_FALSE(name);
Which define a key of different types with an initial true/false
value.
Then allow:
static_branch_likely()
static_branch_unlikely()
to take a key of either type and emit the right instruction for the
case.
This means adding a second arch_static_branch_jump() assembly helper
which emits a JMP per default.
In order to determine the right instruction for the right state,
encode the branch type in the LSB of jump_entry::key.
Signed-off-by: Peter Zijlstra (Intel)
---
arch/arm/include/asm/jump_label.h | 24 +++--
arch/arm64/include/asm/jump_label.h | 18 +++-
arch/mips/include/asm/jump_label.h| 19
arch/powerpc/include/asm/jump_label.h | 19
arch/s390/include/asm/jump_label.h| 19
arch/sparc/include/asm/jump_label.h | 35 ++--
arch/x86/include/asm/jump_label.h | 21
include/linux/jump_label.h| 143 --
kernel/jump_label.c | 35 ++--
9 files changed, 294 insertions(+), 39 deletions(-)
--- a/arch/arm/include/asm/jump_label.h
+++ b/arch/arm/include/asm/jump_label.h
@@ -7,20 +7,28 @@
#define JUMP_LABEL_NOP_SIZE 4
-#ifdef CONFIG_THUMB2_KERNEL
-#define JUMP_LABEL_NOP "nop.w"
-#else
-#define JUMP_LABEL_NOP "nop"
-#endif
+static __always_inline bool arch_static_branch(struct static_key *key, bool
branch)
+{
+ asm_volatile_goto("1:\n\t"
+WASM(nop) "\n\t"
+".pushsection __jump_table, \"aw\"\n\t"
+".word 1b, %l[l_yes], %c0\n\t"
+".popsection\n\t"
+: : "i" (&((char *)key)[branch]) : : l_yes);
+
+ return false;
+l_yes:
+ return true;
+}
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch_jump(struct static_key *key,
bool branch)
{
asm_volatile_goto("1:\n\t"
-JUMP_LABEL_NOP "\n\t"
+WASM(b) " %l[l_yes]\n\t"
".pushsection __jump_table, \"aw\"\n\t"
".word 1b, %l[l_yes], %c0\n\t"
".popsection\n\t"
-: : "i" (key) : : l_yes);
+: : "i" (&((char *)key)[branch]) : : l_yes);
return false;
l_yes:
--- a/arch/arm64/include/asm/jump_label.h
+++ b/arch/arm64/include/asm/jump_label.h
@@ -26,14 +26,28 @@
#define JUMP_LABEL_NOP_SIZEAARCH64_INSN_SIZE
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch(struct static_key *key, bool
branch)
{
asm goto("1: nop\n\t"
".pushsection __jump_table, \"aw\"\n\t"
".align 3\n\t"
".quad 1b, %l[l_yes], %c0\n\t"
".popsection\n\t"
-: : "i"(key) : : l_yes);
+: : "i"(&((char *)key)[branch]) : : l_yes);
+
+ return false;
+l_yes:
+ return true;
+}
+
+static __always_inline bool arch_static_branch_jump(struct static_key *key,
bool branch)
+{
+ asm goto("1: b %l[l_yes]\n\t"
+".pushsection __jump_table, \"aw\"\n\t"
+".align 3\n\t"
+".quad 1b, %l[l_yes], %c0\n\t"
+".popsection\n\t"
+: : "i"(&((char *)key)[branch]) : : l_yes);
return false;
l_yes:
--- a/arch/mips/include/asm/jump_label.h
+++ b/arch/mips/include/asm/jump_label.h
@@ -26,14 +26,29 @@
#define NOP_INSN "nop"
#endif
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch(struct static_key *key, bool
branch)
{
asm_volatile_goto("1:\t" NOP_INSN "\n\t"
"nop\n\t"
".pushsection __jump_table, \"aw\"\n\t"
WORD_INSN " 1b, %l[l_yes], %0\n\t"
".popsection\n\t"
- : : "i" (key) : : l_yes);
+ : : "i" (&((char *)key)[branch]) : : l_yes);
+
+ return false;
+l_yes:
+ return true;
+}
+
+static __always_inline bool arch_static_branch_jump(struct static_key *key,
bool branch)
+{
+ asm_volatile_goto("1:\tj %l[l_yes]\n\t"
+ "nop\n\t"
+ ".pushsection __jump_table, \"aw\"\n\t"
+ WORD_INSN " 1b, %l[l_yes], %0\n\t"
+ ".popsection\n\t"
+ : : "i" (&((char
[PATCH -v2 6/8] jump_label: Add a new static_key interface
There are various problems and short-comings with the current
static_key interface:
- static_key_{true,false}() read like a branch depending on the key
value, instead of the actual likely/unlikely branch depending on
init value.
- static_key_{true,false}() are, as stated above, tied to the
static_key init values STATIC_KEY_INIT_{TRUE,FALSE}.
- we're limited to the 2 (out of 4) possible options that compile to
a default NOP because that's what our arch_static_branch() assembly
emits.
So provide a new static_key interface:
DEFINE_STATIC_KEY_TRUE(name);
DEFINE_STATIC_KEY_FALSE(name);
Which define a key of different types with an initial true/false
value.
Then allow:
static_branch_likely()
static_branch_unlikely()
to take a key of either type and emit the right instruction for the
case.
This means adding a second arch_static_branch_jump() assembly helper
which emits a JMP per default.
In order to determine the right instruction for the right state,
encode the branch type in the LSB of jump_entry::key.
Signed-off-by: Peter Zijlstra (Intel) pet...@infradead.org
---
arch/arm/include/asm/jump_label.h | 24 +++--
arch/arm64/include/asm/jump_label.h | 18 +++-
arch/mips/include/asm/jump_label.h| 19
arch/powerpc/include/asm/jump_label.h | 19
arch/s390/include/asm/jump_label.h| 19
arch/sparc/include/asm/jump_label.h | 35 ++--
arch/x86/include/asm/jump_label.h | 21
include/linux/jump_label.h| 143 --
kernel/jump_label.c | 35 ++--
9 files changed, 294 insertions(+), 39 deletions(-)
--- a/arch/arm/include/asm/jump_label.h
+++ b/arch/arm/include/asm/jump_label.h
@@ -7,20 +7,28 @@
#define JUMP_LABEL_NOP_SIZE 4
-#ifdef CONFIG_THUMB2_KERNEL
-#define JUMP_LABEL_NOP nop.w
-#else
-#define JUMP_LABEL_NOP nop
-#endif
+static __always_inline bool arch_static_branch(struct static_key *key, bool
branch)
+{
+ asm_volatile_goto(1:\n\t
+WASM(nop) \n\t
+.pushsection __jump_table, \aw\\n\t
+.word 1b, %l[l_yes], %c0\n\t
+.popsection\n\t
+: : i (((char *)key)[branch]) : : l_yes);
+
+ return false;
+l_yes:
+ return true;
+}
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch_jump(struct static_key *key,
bool branch)
{
asm_volatile_goto(1:\n\t
-JUMP_LABEL_NOP \n\t
+WASM(b) %l[l_yes]\n\t
.pushsection __jump_table, \aw\\n\t
.word 1b, %l[l_yes], %c0\n\t
.popsection\n\t
-: : i (key) : : l_yes);
+: : i (((char *)key)[branch]) : : l_yes);
return false;
l_yes:
--- a/arch/arm64/include/asm/jump_label.h
+++ b/arch/arm64/include/asm/jump_label.h
@@ -26,14 +26,28 @@
#define JUMP_LABEL_NOP_SIZEAARCH64_INSN_SIZE
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch(struct static_key *key, bool
branch)
{
asm goto(1: nop\n\t
.pushsection __jump_table, \aw\\n\t
.align 3\n\t
.quad 1b, %l[l_yes], %c0\n\t
.popsection\n\t
-: : i(key) : : l_yes);
+: : i(((char *)key)[branch]) : : l_yes);
+
+ return false;
+l_yes:
+ return true;
+}
+
+static __always_inline bool arch_static_branch_jump(struct static_key *key,
bool branch)
+{
+ asm goto(1: b %l[l_yes]\n\t
+.pushsection __jump_table, \aw\\n\t
+.align 3\n\t
+.quad 1b, %l[l_yes], %c0\n\t
+.popsection\n\t
+: : i(((char *)key)[branch]) : : l_yes);
return false;
l_yes:
--- a/arch/mips/include/asm/jump_label.h
+++ b/arch/mips/include/asm/jump_label.h
@@ -26,14 +26,29 @@
#define NOP_INSN nop
#endif
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch(struct static_key *key, bool
branch)
{
asm_volatile_goto(1:\t NOP_INSN \n\t
nop\n\t
.pushsection __jump_table, \aw\\n\t
WORD_INSN 1b, %l[l_yes], %0\n\t
.popsection\n\t
- : : i (key) : : l_yes);
+ : : i (((char *)key)[branch]) : : l_yes);
+
+ return false;
+l_yes:
+ return true;
+}
+
+static __always_inline bool arch_static_branch_jump(struct static_key *key,
bool branch)
+{
+ asm_volatile_goto(1:\tj %l[l_yes]\n\t
+ nop\n\t
+ .pushsection __jump_table, \aw\\n\t
+ WORD_INSN 1b, %l[l_yes], %0\n\t
+ .popsection\n\t
+ : : i (((char *)key)[branch]) : : l_yes);
+
return false;
l_yes:
return true;
---
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, Jul 28, 2015 at 07:00:55PM +0200, Rabin Vincent wrote: This is missing an include of asm/unified.h for the WASM(): diff --git a/arch/arm/include/asm/jump_label.h b/arch/arm/include/asm/jump_label.h index f8bc12f..34f7b69 100644 --- a/arch/arm/include/asm/jump_label.h +++ b/arch/arm/include/asm/jump_label.h @@ -4,6 +4,7 @@ #ifndef __ASSEMBLY__ #include linux/types.h +#include asm/unified.h #define JUMP_LABEL_NOP_SIZE 4 With that added, it builds and works fine on ARM. Duh, thanks! -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface
On Tue, Jul 28, 2015 at 03:21:01PM +0200, Peter Zijlstra wrote:
--- a/arch/arm/include/asm/jump_label.h
+++ b/arch/arm/include/asm/jump_label.h
@@ -7,20 +7,28 @@
#define JUMP_LABEL_NOP_SIZE 4
-#ifdef CONFIG_THUMB2_KERNEL
-#define JUMP_LABEL_NOP nop.w
-#else
-#define JUMP_LABEL_NOP nop
-#endif
+static __always_inline bool arch_static_branch(struct static_key *key, bool
branch)
+{
+ asm_volatile_goto(1:\n\t
+ WASM(nop) \n\t
+ .pushsection __jump_table, \aw\\n\t
+ .word 1b, %l[l_yes], %c0\n\t
+ .popsection\n\t
+ : : i (((char *)key)[branch]) : : l_yes);
+
+ return false;
+l_yes:
+ return true;
+}
-static __always_inline bool arch_static_branch(struct static_key *key)
+static __always_inline bool arch_static_branch_jump(struct static_key *key,
bool branch)
{
asm_volatile_goto(1:\n\t
- JUMP_LABEL_NOP \n\t
+ WASM(b) %l[l_yes]\n\t
.pushsection __jump_table, \aw\\n\t
.word 1b, %l[l_yes], %c0\n\t
.popsection\n\t
- : : i (key) : : l_yes);
+ : : i (((char *)key)[branch]) : : l_yes);
return false;
l_yes:
This is missing an include of asm/unified.h for the WASM():
diff --git a/arch/arm/include/asm/jump_label.h
b/arch/arm/include/asm/jump_label.h
index f8bc12f..34f7b69 100644
--- a/arch/arm/include/asm/jump_label.h
+++ b/arch/arm/include/asm/jump_label.h
@@ -4,6 +4,7 @@
#ifndef __ASSEMBLY__
#include linux/types.h
+#include asm/unified.h
#define JUMP_LABEL_NOP_SIZE 4
With that added, it builds and works fine on ARM.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
