Re: [PATCH 1/3] X86/db: Change __this_cpu_read() to this_cpu_read() in hw_breakpoint_active()

[Borislav Petkov <bp@alien8.de>]

On Tue, Dec 14, 2021 at 10:51:23AM +0800, Lai Jiangshan wrote:
> The commit message was checked via VIM spellchecker.  It did highlight
> denylist, noinstr, noinstrument, complexify, and a lot more.
> 
> There are too many false-negative results from VIM spellchecker, and

I don't know how your vim is configured but my vim spellchecker
highlights only those words which I mentioned.

> I searched denylist, complexify via google and they are used by some
> other places so I kept them.

And this is where the problem is: you can't just search for words on the
net and whether someone used them or created them and then assume that
the reader would know them and understand what you mean.

Writing commit messages is not an exercise in creative writing. Rather,
your commit messages must be *maximally* *understandable* to the
reviewer so that she/he doesn't have to

a) decipher your commit message
b) decipher your diff

because that's twice the work.

So the goal is to explain why you're doing a change in the clearest way
possible - not do fancy.

Also, even if you use known words, there's this other problem with
formulation: a sentence full of only correct words doesn't make it
understandable to others. So try to stick to simple, even boring
formulations - you can be sure the reader would know what you mean.

> I'm sorry for not searching in the kernel tree to find a proper
> word for noinstrument, not searching the web for better words for
> denylist, complexify.
> 
> I will change a spellchecker and improve my English.

Thanks for the effort!

> What I wanted to say in this paragraph is that why I chose this way to fix
> it since there are several ways/policies to fix it.
> 
> "Changing __this_cpu_read() to this_cpu_read() is fit for" this policy.
> 
> I don't think it can be seen in the diff.

What "policy"? Fit for what?

This is what I mean: the formulation sounds weird and it makes me wonder
what you're *actually* trying to say?

> > I don't really follow the argument for why this_cpu_read();

See, Peter has a hard time understanding your reasoning either.

> >        /*
> >         * Must not hit a breakpoint in check_preempt_disabled()
> >         */
> >        return raw_cpu_read(cpu_dr7) & DR_GLOBAL_ENABLE_MASK;
> 
> Although, this comment is describing raw_cpu_read() obviously, I often
> can't get which code is a comment in other places referring to due
> to later changes with new code added and removed.

Each comment must belong to the code it comments - otherwise it needs to
be fixed/removed.

In this particular case, it could be over hw_breakpoint_active() too.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette