All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jisheng Zhang <jszhang@kernel.org>
To: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Paul Walmsley <paul.walmsley@sifive.com>,
	aou@eecs.berkeley.edu, ryabinin.a.a@gmail.com, glider@google.com,
	andreyknvl@gmail.com, dvyukov@google.com,
	vincenzo.frascino@arm.com, alexandre.ghiti@canonical.com,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
	kasan-dev@googlegroups.com
Subject: Re: [PATCH v6 RESEND 0/2] use static key to optimize pgtable_l4_enabled
Date: Thu, 12 Jan 2023 01:28:40 +0800	[thread overview]
Message-ID: <Y77xyNPNqnFQUqAx@xhacker> (raw)
In-Reply-To: <Y5W0bv8Y/zCc+Fco@xhacker>

On Sun, Dec 11, 2022 at 06:44:04PM +0800, Jisheng Zhang wrote:
> On Sat, Oct 08, 2022 at 10:28:35PM +0800, Jisheng Zhang wrote:
> > On Thu, Oct 06, 2022 at 04:37:57PM +0800, Jisheng Zhang wrote:
> > > On Wed, Oct 05, 2022 at 06:05:28PM -0700, Palmer Dabbelt wrote:
> > > > On Sun, 21 Aug 2022 07:09:16 PDT (-0700), jszhang@kernel.org wrote:
> > > > > The pgtable_l4|[l5]_enabled check sits at hot code path, performance
> > > > > is impacted a lot. Since pgtable_l4|[l5]_enabled isn't changed after
> > > > > boot, so static key can be used to solve the performance issue[1].
> > > > > 
> > > > > An unified way static key was introduced in [2], but it only targets
> > > > > riscv isa extension. We dunno whether SV48 and SV57 will be considered
> > > > > as isa extension, so the unified solution isn't used for
> > > > > pgtable_l4[l5]_enabled now.
> > > > > 
> > > > > patch1 fixes a NULL pointer deference if static key is used a bit earlier.
> > > > > patch2 uses the static key to optimize pgtable_l4|[l5]_enabled.
> > > > > 
> > > > > [1] http://lists.infradead.org/pipermail/linux-riscv/2021-December/011164.html
> > > > > [2] https://lore.kernel.org/linux-riscv/20220517184453.3558-1-jszhang@kernel.org/T/#t
> > > > > 
> > > > > Since v5:
> > > > >  - Use DECLARE_STATIC_KEY_FALSE
> > > > > 
> > > > > Since v4:
> > > > >  - rebased on v5.19-rcN
> > > > >  - collect Reviewed-by tags
> > > > >  - Fix kernel panic issue if SPARSEMEM is enabled by moving the
> > > > >    riscv_finalise_pgtable_lx() after sparse_init()
> > > > > 
> > > > > Since v3:
> > > > >  - fix W=1 call to undeclared function 'static_branch_likely' error
> > > > > 
> > > > > Since v2:
> > > > >  - move the W=1 warning fix to a separate patch
> > > > >  - move the unified way to use static key to a new patch series.
> > > > > 
> > > > > Since v1:
> > > > >  - Add a W=1 warning fix
> > > > >  - Fix W=1 error
> > > > >  - Based on v5.18-rcN, since SV57 support is added, so convert
> > > > >    pgtable_l5_enabled as well.
> > > > > 
> > > > > 
> > > > > Jisheng Zhang (2):
> > > > >   riscv: move sbi_init() earlier before jump_label_init()
> > > > >   riscv: turn pgtable_l4|[l5]_enabled to static key for RV64
> > > > > 
> > > > >  arch/riscv/include/asm/pgalloc.h    | 16 ++++----
> > > > >  arch/riscv/include/asm/pgtable-32.h |  3 ++
> > > > >  arch/riscv/include/asm/pgtable-64.h | 60 ++++++++++++++++++---------
> > > > >  arch/riscv/include/asm/pgtable.h    |  5 +--
> > > > >  arch/riscv/kernel/cpu.c             |  4 +-
> > > > >  arch/riscv/kernel/setup.c           |  2 +-
> > > > >  arch/riscv/mm/init.c                | 64 ++++++++++++++++++-----------
> > > > >  arch/riscv/mm/kasan_init.c          | 16 ++++----
> > > > >  8 files changed, 104 insertions(+), 66 deletions(-)
> > > > 
> > > > Sorry for being slow here, but it looks like this still causes some early
> > > > boot hangs.  Specifically kasan+sparsemem is failing.  As you can probably
> > > > see from the latency I'm still a bit buried right now so I'm not sure when
> > > > I'll have a chance to take more of a look.
> > > 
> > > Hi Palmer,
> > > 
> > > Before V4, there is a bug which can cause kernel panic when SPARSEMEM
> > > is enabled, V4 have fixed it by moving the riscv_finalise_pgtable_lx()
> > > after sparse_init(). And I just tested the riscv-pgtable_static_key
> > > branch in your tree, enabling KASAN and SPARSEMEM, system booted fine.
> > > I'm not sure what happened. Could you please send me your kernel
> > > config file? I want to fix any issue which can block this series being
> > > merged in 6.1-rc1.
> > 
> > Hi Palmer,
> > 
> > I know you are busy ;) Do you have time to send me your test kernel
> > config file so that I can reproduce the "early boot hang"?
> > 
> > Thanks
> 
> Hi Palmer,
> 
> I think the early boot hangs maybe the same as the one which has been
> fixed by commit 9f2ac64d6ca6 ("riscv: mm: add missing memcpy in
> kasan_init"). Will you give this series another try for v6.2-rc1? If
> the boot hang can still be reproduced, could you please send me your
> .config file?
> 
> Thanks in advance
Hi all,

Just request to comment what to do with this patch, I think there
are two independent points to consult:

1. IIRC, Palmer gave this patch two chances to merge in early versions
but he found boot hangs if enable KASAN and SPARSEMEM, while I can't
reproduce the boot hang. And I also expect the hang should be fixed by
commit 9f2ac64d6ca6 ("riscv: mm: add missing memcpy in kasan_init")

2. Now we know alternative is preferred than static branch for ISA
extensions dynamic code patching. So we also need to switch static
branch usage here to alternative mechanism, but the problem is
SV48 and SV57 are not ISA extensions, so we can't directly make use
of the recently introduced riscv_has_extension_likely|unlikely()[1] 
which is based on alternative mechanism.

Any comments are appreciated.

Thanks in advance

[1] https://lore.kernel.org/linux-riscv/20230111171027.2392-1-jszhang@kernel.org/T/#t

WARNING: multiple messages have this Message-ID (diff)
From: Jisheng Zhang <jszhang@kernel.org>
To: Palmer Dabbelt <palmer@dabbelt.com>
Cc: Paul Walmsley <paul.walmsley@sifive.com>,
	aou@eecs.berkeley.edu, ryabinin.a.a@gmail.com, glider@google.com,
	andreyknvl@gmail.com, dvyukov@google.com,
	vincenzo.frascino@arm.com, alexandre.ghiti@canonical.com,
	linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
	kasan-dev@googlegroups.com
Subject: Re: [PATCH v6 RESEND 0/2] use static key to optimize pgtable_l4_enabled
Date: Thu, 12 Jan 2023 01:28:40 +0800	[thread overview]
Message-ID: <Y77xyNPNqnFQUqAx@xhacker> (raw)
In-Reply-To: <Y5W0bv8Y/zCc+Fco@xhacker>

On Sun, Dec 11, 2022 at 06:44:04PM +0800, Jisheng Zhang wrote:
> On Sat, Oct 08, 2022 at 10:28:35PM +0800, Jisheng Zhang wrote:
> > On Thu, Oct 06, 2022 at 04:37:57PM +0800, Jisheng Zhang wrote:
> > > On Wed, Oct 05, 2022 at 06:05:28PM -0700, Palmer Dabbelt wrote:
> > > > On Sun, 21 Aug 2022 07:09:16 PDT (-0700), jszhang@kernel.org wrote:
> > > > > The pgtable_l4|[l5]_enabled check sits at hot code path, performance
> > > > > is impacted a lot. Since pgtable_l4|[l5]_enabled isn't changed after
> > > > > boot, so static key can be used to solve the performance issue[1].
> > > > > 
> > > > > An unified way static key was introduced in [2], but it only targets
> > > > > riscv isa extension. We dunno whether SV48 and SV57 will be considered
> > > > > as isa extension, so the unified solution isn't used for
> > > > > pgtable_l4[l5]_enabled now.
> > > > > 
> > > > > patch1 fixes a NULL pointer deference if static key is used a bit earlier.
> > > > > patch2 uses the static key to optimize pgtable_l4|[l5]_enabled.
> > > > > 
> > > > > [1] http://lists.infradead.org/pipermail/linux-riscv/2021-December/011164.html
> > > > > [2] https://lore.kernel.org/linux-riscv/20220517184453.3558-1-jszhang@kernel.org/T/#t
> > > > > 
> > > > > Since v5:
> > > > >  - Use DECLARE_STATIC_KEY_FALSE
> > > > > 
> > > > > Since v4:
> > > > >  - rebased on v5.19-rcN
> > > > >  - collect Reviewed-by tags
> > > > >  - Fix kernel panic issue if SPARSEMEM is enabled by moving the
> > > > >    riscv_finalise_pgtable_lx() after sparse_init()
> > > > > 
> > > > > Since v3:
> > > > >  - fix W=1 call to undeclared function 'static_branch_likely' error
> > > > > 
> > > > > Since v2:
> > > > >  - move the W=1 warning fix to a separate patch
> > > > >  - move the unified way to use static key to a new patch series.
> > > > > 
> > > > > Since v1:
> > > > >  - Add a W=1 warning fix
> > > > >  - Fix W=1 error
> > > > >  - Based on v5.18-rcN, since SV57 support is added, so convert
> > > > >    pgtable_l5_enabled as well.
> > > > > 
> > > > > 
> > > > > Jisheng Zhang (2):
> > > > >   riscv: move sbi_init() earlier before jump_label_init()
> > > > >   riscv: turn pgtable_l4|[l5]_enabled to static key for RV64
> > > > > 
> > > > >  arch/riscv/include/asm/pgalloc.h    | 16 ++++----
> > > > >  arch/riscv/include/asm/pgtable-32.h |  3 ++
> > > > >  arch/riscv/include/asm/pgtable-64.h | 60 ++++++++++++++++++---------
> > > > >  arch/riscv/include/asm/pgtable.h    |  5 +--
> > > > >  arch/riscv/kernel/cpu.c             |  4 +-
> > > > >  arch/riscv/kernel/setup.c           |  2 +-
> > > > >  arch/riscv/mm/init.c                | 64 ++++++++++++++++++-----------
> > > > >  arch/riscv/mm/kasan_init.c          | 16 ++++----
> > > > >  8 files changed, 104 insertions(+), 66 deletions(-)
> > > > 
> > > > Sorry for being slow here, but it looks like this still causes some early
> > > > boot hangs.  Specifically kasan+sparsemem is failing.  As you can probably
> > > > see from the latency I'm still a bit buried right now so I'm not sure when
> > > > I'll have a chance to take more of a look.
> > > 
> > > Hi Palmer,
> > > 
> > > Before V4, there is a bug which can cause kernel panic when SPARSEMEM
> > > is enabled, V4 have fixed it by moving the riscv_finalise_pgtable_lx()
> > > after sparse_init(). And I just tested the riscv-pgtable_static_key
> > > branch in your tree, enabling KASAN and SPARSEMEM, system booted fine.
> > > I'm not sure what happened. Could you please send me your kernel
> > > config file? I want to fix any issue which can block this series being
> > > merged in 6.1-rc1.
> > 
> > Hi Palmer,
> > 
> > I know you are busy ;) Do you have time to send me your test kernel
> > config file so that I can reproduce the "early boot hang"?
> > 
> > Thanks
> 
> Hi Palmer,
> 
> I think the early boot hangs maybe the same as the one which has been
> fixed by commit 9f2ac64d6ca6 ("riscv: mm: add missing memcpy in
> kasan_init"). Will you give this series another try for v6.2-rc1? If
> the boot hang can still be reproduced, could you please send me your
> .config file?
> 
> Thanks in advance
Hi all,

Just request to comment what to do with this patch, I think there
are two independent points to consult:

1. IIRC, Palmer gave this patch two chances to merge in early versions
but he found boot hangs if enable KASAN and SPARSEMEM, while I can't
reproduce the boot hang. And I also expect the hang should be fixed by
commit 9f2ac64d6ca6 ("riscv: mm: add missing memcpy in kasan_init")

2. Now we know alternative is preferred than static branch for ISA
extensions dynamic code patching. So we also need to switch static
branch usage here to alternative mechanism, but the problem is
SV48 and SV57 are not ISA extensions, so we can't directly make use
of the recently introduced riscv_has_extension_likely|unlikely()[1] 
which is based on alternative mechanism.

Any comments are appreciated.

Thanks in advance

[1] https://lore.kernel.org/linux-riscv/20230111171027.2392-1-jszhang@kernel.org/T/#t

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

  reply	other threads:[~2023-01-11 17:39 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-08-21 14:09 [PATCH v6 RESEND 0/2] use static key to optimize pgtable_l4_enabled Jisheng Zhang
2022-08-21 14:09 ` Jisheng Zhang
2022-08-21 14:09 ` [PATCH v6 RESEND 1/2] riscv: move sbi_init() earlier before jump_label_init() Jisheng Zhang
2022-08-21 14:09   ` Jisheng Zhang
2022-08-21 14:09 ` [PATCH v6 RESEND 2/2] riscv: turn pgtable_l4|[l5]_enabled to static key for RV64 Jisheng Zhang
2022-08-21 14:09   ` Jisheng Zhang
2022-10-06  1:05 ` [PATCH v6 RESEND 0/2] use static key to optimize pgtable_l4_enabled Palmer Dabbelt
2022-10-06  1:05   ` Palmer Dabbelt
2022-10-06  8:37   ` Jisheng Zhang
2022-10-06  8:37     ` Jisheng Zhang
2022-10-08 14:28     ` Jisheng Zhang
2022-10-08 14:28       ` Jisheng Zhang
2022-12-11 10:43       ` Jisheng Zhang
2022-12-11 10:43         ` Jisheng Zhang
2023-01-11 17:28         ` Jisheng Zhang [this message]
2023-01-11 17:28           ` Jisheng Zhang
2023-01-11 19:00           ` Andrew Jones
2023-01-11 19:00             ` Andrew Jones
2023-01-11 20:51             ` Jessica Clarke
2023-01-11 20:51               ` Jessica Clarke

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=Y77xyNPNqnFQUqAx@xhacker \
    --to=jszhang@kernel.org \
    --cc=alexandre.ghiti@canonical.com \
    --cc=andreyknvl@gmail.com \
    --cc=aou@eecs.berkeley.edu \
    --cc=dvyukov@google.com \
    --cc=glider@google.com \
    --cc=kasan-dev@googlegroups.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=palmer@dabbelt.com \
    --cc=paul.walmsley@sifive.com \
    --cc=ryabinin.a.a@gmail.com \
    --cc=vincenzo.frascino@arm.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.