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

Groups > linux.kernel > #1194955 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface Peter Zijlstra <peterz@infradead.org> - 2015-07-29 10:50 +0200
    Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface Steven Rostedt <rostedt@goodmis.org> - 2015-08-03 21:10 +0200
      Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface Peter Zijlstra <peterz@infradead.org> - 2015-08-03 21:20 +0200
        Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface Steven Rostedt <rostedt@goodmis.org> - 2015-08-03 21:30 +0200
          Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface Peter Zijlstra <peterz@infradead.org> - 2015-08-03 22:10 +0200
            Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface Steven Rostedt <rostedt@goodmis.org> - 2015-08-04 00:00 +0200
              Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface Borislav Petkov <bp@alien8.de> - 2015-08-04 05:40 +0200
                Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface Andy Lutomirski <luto@amacapital.net> - 2015-08-04 06:10 +0200
                  Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface Borislav Petkov <bp@alien8.de> - 2015-08-04 06:30 +0200
                Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface Steven Rostedt <rostedt@goodmis.org> - 2015-08-04 14:10 +0200
                  Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface Borislav Petkov <bp@alien8.de> - 2015-08-04 16:40 +0200
                    Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface Andy Lutomirski <luto@amacapital.net> - 2015-08-04 17:00 +0200

#1194955 — 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1199194

On Wed, 29 Jul 2015 10:49:06 +0200
Peter Zijlstra <peterz@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1199208

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1199214

On Mon, 3 Aug 2015 21:18:16 +0200
Peter Zijlstra <peterz@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1199237

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1199289

On Mon, 3 Aug 2015 22:00:02 +0200
Peter Zijlstra <peterz@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1199397

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1199404

On Mon, Aug 3, 2015 at 8:37 PM, Borislav Petkov <bp@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1199406

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1199734

On Tue, 4 Aug 2015 05:37:33 +0200
Borislav Petkov <bp@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1199916

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1199964

On Tue, Aug 4, 2015 at 7:33 AM, Borislav Petkov <bp@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web