From: Huacai Chen <chenhuacai@kernel.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Huacai Chen <chenhuacai@loongson.cn>,
Arnd Bergmann <arnd@arndb.de>,
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: Sat, 9 Sep 2023 16:05:44 +0800 [thread overview]
Message-ID: <CAAhV-H64=ZWBnzFmtS-FMB83nn21DSm20adrXdsrYkoka_oyjw@mail.gmail.com> (raw)
In-Reply-To: <CAHk-=wh-uoZ_XsZ4fDodqjUY+rYJq=D3RVme3f=zBDB5neWhKQ@mail.gmail.com>
Hi, Linus,
On Sat, Sep 9, 2023 at 3:48 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> 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.
Thank you for your suggestions, I will make cleanup patches for the
two issues before v6.6 is released.
Huacai
>
> Linus
>
next prev parent reply other threads:[~2023-09-09 8:06 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-08 11:10 [GIT PULL] LoongArch changes for v6.6 Huacai Chen
2023-09-08 19:48 ` Linus Torvalds
2023-09-09 8:05 ` Huacai Chen [this message]
2023-09-08 20:00 ` pr-tracker-bot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAAhV-H64=ZWBnzFmtS-FMB83nn21DSm20adrXdsrYkoka_oyjw@mail.gmail.com' \
--to=chenhuacai@kernel.org \
--cc=arnd@arndb.de \
--cc=chenhuacai@loongson.cn \
--cc=guoren@kernel.org \
--cc=jiaxun.yang@flygoat.com \
--cc=kernel@xen0n.name \
--cc=linux-arch@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=loongarch@lists.linux.dev \
--cc=torvalds@linux-foundation.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).