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/
