Re: [GIT PULL] LoongArch changes for v6.6

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Huacai Chen <chenhuacai@loongson.cn>
Cc: Arnd Bergmann <arnd@arndb.de>,
	Huacai Chen <chenhuacai@kernel.org>,
	loongarch@lists.linux.dev, linux-arch@vger.kernel.org,
	linux-kernel@vger.kernel.org, Guo Ren <guoren@kernel.org>,
	Xuerui Wang <kernel@xen0n.name>,
	Jiaxun Yang <jiaxun.yang@flygoat.com>
Subject: Re: [GIT PULL] LoongArch changes for v6.6
Date: Fri, 8 Sep 2023 12:48:07 -0700	[thread overview]
Message-ID: <CAHk-=wh-uoZ_XsZ4fDodqjUY+rYJq=D3RVme3f=zBDB5neWhKQ@mail.gmail.com> (raw)
In-Reply-To: <20230908111053.1660033-1-chenhuacai@loongson.cn>

On Fri, 8 Sept 2023 at 04:11, Huacai Chen <chenhuacai@loongson.cn> wrote:
>
> 7, Add KASAN (Kernel Address Sanitizer) support

I have pulled this, but please at least consider

 (a) don't use the stupid and random __HAVE_ARCH_xyz defines

Yes, yes, we have historical uses of it. That doesn't make it good.
Instead of making up new random symbol names, just *USE* the names you
are defining per architecture.

IOW, instead of doing

  #define __HAVE_ARCH_SHADOW_MAP

and defining your own helper replacement functions for
kasan_mem_to_shadow() etc, just use those names as-is.

So if you have an architecture that has its own version of
"kasan_mem_to_shadow()", then use *THAT* name for the #ifdef too.
Don't make up another name entirely of the form "__HAVE_ARCH_xyz".

Example: architectures can override the default generic versions of
"arch_atomic_xyz()" operations, and the way you do that is (for
example)

  static __always_inline int arch_atomic_add_return(int i, atomic_t *v)
  {
        return i + xadd(&v->counter, i);
  }
  #define arch_atomic_add_return arch_atomic_add_return

note how the define to let you know the name exists is the name itself
(and because the implementation is an inline function, not the macro,
the marker is then just a "define the name to itself").

End result: no made-up secondary names for other things. When you grep
for the thing that is used, you find both the implementation and the
marker for "look, I am overriding it".

And also

 (b) do you really want to inline those kasan_mem_to_shadow() and
kasan_shadow_to_mem() things?

Yes, the caller is addr_has_metadata(), and that one is
"__always_inline",. because otherwise objtool would complain about
doing function calls in SMAP-enabled regions.

HOWEVER:

 - on LoongArch, I don't think you have that objtool / SMAP issue

 - and if  you did, the function should be __always_inline, not just
plain inline anyway

so I get the feeling that the inline is simply wrong either way. Maybe
that function should just be declared, with the implementation being
out-of-line? It seems unnecessarily big to be an inline function, and
it doesn't seem performance-critical?

Neither of the above issues are critical, and the second one in
particular really is just a "did you really mean to do that" kind of
reaction, but since I was looking at this, I decided to write up my
reactions.

              Linus