Messages in this thread | | | From | Andy Lutomirski <> | Date | Tue, 4 Aug 2015 07:51:49 -0700 | Subject | Re: [PATCH -v2 6/8] jump_label: Add a new static_key interface |
| |
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
| |