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

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