Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface

2015-08-04 Thread Steven Rostedt
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

2015-08-04 Thread Andy Lutomirski
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

2015-08-04 Thread Borislav Petkov
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

2015-08-04 Thread Steven Rostedt
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

2015-08-04 Thread Peter Zijlstra
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

2015-08-03 Thread yalin wang

> 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

2015-08-03 Thread Borislav Petkov
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

2015-08-03 Thread Andy Lutomirski
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(&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

2015-08-03 Thread Borislav Petkov
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

2015-08-03 Thread Steven Rostedt
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(&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

2015-08-03 Thread Peter Zijlstra
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

2015-08-03 Thread Steven Rostedt
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(&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

2015-08-03 Thread Peter Zijlstra
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

2015-08-03 Thread Steven Rostedt
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(&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

2015-07-30 Thread Michael Ellerman
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

2015-07-29 Thread Peter Zijlstra
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

2015-07-29 Thread Vlastimil Babka
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

2015-07-28 Thread Heiko Carstens
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

2015-07-28 Thread Peter Zijlstra
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

2015-07-28 Thread Rabin Vincent
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/