Messages in this thread | | | Date | Wed, 27 Sep 2017 10:56:17 +0200 | From | Jean Delvare <> | Subject | Re: [PATCH] firmware: dmi_scan: Drop dmi_initialized |
| |
Hi Peter,
On Mon, 25 Sep 2017 11:26:44 +0200, Peter Zijlstra wrote: > On Mon, Sep 25, 2017 at 11:00:11AM +0200, Jean Delvare wrote: > > Then we have that in common. While reading the code and its history, I > > was worried that the justification to add this warning in the first > > place was technically weak. Not every coding error must automatically > > translate to a patch to make the code robust against said error. > > Sometimes you just have to admit that you did not pay attention as you > > should have, fix your mistake, possibly document it for others, and > > move on. Otherwise we end up with slow bloated code. > > That WARN_ON() is a form of documentation.
Sort of, but not the best form in my opinion. I mean, if a WARN triggers, you'll have to spot it in the kernel log, and then figure out why the condition which triggered it was evaluated to false. In this specific case, this means grepping the code for "dmi_initialized" to figure out where it should have been set. The error string ("dmi check: not initialized yet") helps a bit, but that's still not as clear and immediate as an explicit "dmi_scan_machine must be called before this function is called" in the function description.
> And if you care about performance for your code path, hide it under some > CONFIG_*_DEBUG,
I'd love to, but I couldn't find a generic one which would actually be disabled on production kernels, and introducing a configuration option for just this seems overkill.
> (...) but in general WARN_ON() isn't terribly expensive > (depending entirely on the complexity of the condition of course).
The condition is pretty trivial here, so it should indeed be fairly cheap. However you have to multiply it by the number of times it is called. When this WARN was introduced, there were 61 calls to dmi_check_system() in the whole kernel tree. Today we have 164 and the count keeps increasing. And some of them in common paths which get called repeatedly. I just instrumented the function to see how many times it was called on my x86_64 workstation and the answer is 110. That's not a one-time thing.
-- Jean Delvare SUSE L3 Support
| |