Messages in this thread |  | | From | Linus Torvalds <> | Date | Sun, 1 Oct 2017 12:59:37 -0700 | Subject | Re: [RFC GIT Pull] core watchdog sanitizing |
| |
I refuse to pull this.
Look, I understand what you want to do, but the code is disgusting.
Maybe most of it is fine, but I just couldn't stomach looking at it after just a few lines.
Look at that abortion called "watchdog_nmi_reconfigure()".
It's one single function that does two completely different things based on a static argument.
Whaa? So now you have doubly illegible code: the callers have that insane set of
watchdog_nmi_reconfigure(true/false);
in it, which is illegible garbage. There's no way that makes sense to anybody.
And the actual implementation has
void watchdog_nmi_reconfigure(bool run)
where the whole function is basically a single if-statement on that "run" argument that does two totally different things.
If you don't see how that is pure and utter garbage, I don't know what to say. It's just bad code. Don't do that in the first place, but *definitely* don't do that and then try to send it to me during an rc.
Seriously, instead of that retarded
watchdog_nmi_reconfigure(false); ... watchdog_nmi_reconfigure(true);
you could have written it something like
watchdog_nmi_stop(); ... watchdog_nmi_start();
and it would be _understandable_ code. You don't have to even know the code to understand what's going on.
So get rid of that kind of shit, and I may reconsider. But as is, I look at that patch and say "no, this is worse than the garbage it used to be".
Yeah, yeah, I'll be honest, and you were unlucky, and just happened to hit a pet peeve of mine, but I absolutely *detest* code that describes obvious static things as non-obvious dynamic "flag" arguments to a function for no good reason. There's not even any code sharing between the two cases going on aside from the trivial locking.
That kind of code makes it harder for everybody. It makes it harder for the compiler to generate good code (you need to inline it to get the obvious code generation), it makes it harder for checkers to verify things, and it makes it harder for humans to read and understand what is going on.
If you see code like
watchdog_nmi_reconfigure(true);
and you don't ask yourself "what does 'true' mean in this call site", you're either not human, or you just don't care. Neither of which are good.
In contrast, when you see code like
watchdog_nmi_start();
it kind of documents itself, don't you think?
And yes, I see the comment above the function definition. No, the comment doesn't help. The comment just makes it obvious that somebody realized that the calling convention was too damn confusing. Good. But even better would have been to just not make it that confusing in the first place.
Linus
On Sun, Oct 1, 2017 at 3:34 AM, Thomas Gleixner <tglx@linutronix.de> wrote: > > The watchdog (hard/softlockup detector) code is pretty much broken in its > current state. The patch series addresses this by removing all duct tape > and refactoring it into a workable state.
|  |