Message ID | 20200818040728.528426-1-anup.patel@wdc.com |
---|---|
State | Accepted |
Headers | show |
Series | [v3,1/2] lib: sbi_init: Avoid thundering hurd problem with coldboot_lock | expand |
On Tue, Aug 18, 2020 at 12:09 PM Anup Patel <anup.patel@wdc.com> wrote: > > We can have thundering hurd problem with coldboot_lock where the > boot HART can potentially starve trying to acquire coldboot_lock > because some of the non-boot HARTs are continuously acquiring and > releasing coldboot_lock. This can happen if MIP.MSIP bit is already > set for some of the non-boot HARTs. > > To avoid thundering hurd problem for coldboot_lock, we use the > __smp_load_acquire() and __smp_store_release() for coldboot_done > flag and use coldboot_lock only for coldboot_wait_hmask. > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > Reviewed-by: Atish Patra <atish.patra@wdc.com> > --- > lib/sbi/sbi_init.c | 20 +++++++++++++------- > 1 file changed, 13 insertions(+), 7 deletions(-) > Reviewed-by: Bin Meng <bin.meng@windriver.com>
On 8/18/20 12:07 AM, Anup Patel wrote: > We can have thundering hurd problem with coldboot_lock where the > boot HART can potentially starve trying to acquire coldboot_lock > because some of the non-boot HARTs are continuously acquiring and > releasing coldboot_lock. This can happen if MIP.MSIP bit is already > set for some of the non-boot HARTs. > > To avoid thundering hurd problem for coldboot_lock, we use the > __smp_load_acquire() and __smp_store_release() for coldboot_done > flag and use coldboot_lock only for coldboot_wait_hmask. > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > Reviewed-by: Atish Patra <atish.patra@wdc.com> Just curious, what are the changes from v1 and v2? --Sean
On 19.08.20 14:03, Sean Anderson wrote: > On 8/18/20 12:07 AM, Anup Patel wrote: >> We can have thundering hurd problem with coldboot_lock where the >> boot HART can potentially starve trying to acquire coldboot_lock >> because some of the non-boot HARTs are continuously acquiring and >> releasing coldboot_lock. This can happen if MIP.MSIP bit is already >> set for some of the non-boot HARTs. >> >> To avoid thundering hurd problem for coldboot_lock, we use the >> __smp_load_acquire() and __smp_store_release() for coldboot_done >> flag and use coldboot_lock only for coldboot_wait_hmask. >> >> Signed-off-by: Anup Patel <anup.patel@wdc.com> >> Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> Reviewed-by: Atish Patra <atish.patra@wdc.com> > > Just curious, what are the changes from v1 and v2? Barriers are realized in a different way in patch 1. Except for the barriers patch 1 follows what I proposed in my original patch and the following discussion. And for sure Jessica was right that at least compiler barriers are needed. Best regards Heinrich
> -----Original Message----- > From: Heinrich Schuchardt <xypron.glpk@gmx.de> > Sent: 19 August 2020 17:39 > To: Sean Anderson <seanga2@gmail.com>; Anup Patel > <Anup.Patel@wdc.com>; Atish Patra <Atish.Patra@wdc.com>; Alistair > Francis <Alistair.Francis@wdc.com> > Cc: opensbi@lists.infradead.org; Anup Patel <anup@brainfault.org> > Subject: Re: [PATCH v3 1/2] lib: sbi_init: Avoid thundering hurd problem with > coldboot_lock > > On 19.08.20 14:03, Sean Anderson wrote: > > On 8/18/20 12:07 AM, Anup Patel wrote: > >> We can have thundering hurd problem with coldboot_lock where the > boot > >> HART can potentially starve trying to acquire coldboot_lock because > >> some of the non-boot HARTs are continuously acquiring and releasing > >> coldboot_lock. This can happen if MIP.MSIP bit is already set for > >> some of the non-boot HARTs. > >> > >> To avoid thundering hurd problem for coldboot_lock, we use the > >> __smp_load_acquire() and __smp_store_release() for coldboot_done > flag > >> and use coldboot_lock only for coldboot_wait_hmask. > >> > >> Signed-off-by: Anup Patel <anup.patel@wdc.com> > >> Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > >> Reviewed-by: Atish Patra <atish.patra@wdc.com> > > > > Just curious, what are the changes from v1 and v2? > > Barriers are realized in a different way in patch 1. > > Except for the barriers patch 1 follows what I proposed in my original patch > and the following discussion. > > And for sure Jessica was right that at least compiler barriers are needed. Yes, v1->v2 is replacing use of atomic_read/write() with__smp_load/store_xyz(). v2->v3 is only a typo-fix in PATCH1 subject. Regards, Anup
> -----Original Message----- > From: Bin Meng <bmeng.cn@gmail.com> > Sent: 18 August 2020 14:46 > To: Anup Patel <Anup.Patel@wdc.com> > Cc: Atish Patra <Atish.Patra@wdc.com>; Alistair Francis > <Alistair.Francis@wdc.com>; Heinrich Schuchardt <xypron.glpk@gmx.de>; > OpenSBI <opensbi@lists.infradead.org>; Anup Patel <anup@brainfault.org> > Subject: Re: [PATCH v3 1/2] lib: sbi_init: Avoid thundering hurd problem with > coldboot_lock > > On Tue, Aug 18, 2020 at 12:09 PM Anup Patel <anup.patel@wdc.com> wrote: > > > > We can have thundering hurd problem with coldboot_lock where the boot > > HART can potentially starve trying to acquire coldboot_lock because > > some of the non-boot HARTs are continuously acquiring and releasing > > coldboot_lock. This can happen if MIP.MSIP bit is already set for some > > of the non-boot HARTs. > > > > To avoid thundering hurd problem for coldboot_lock, we use the > > __smp_load_acquire() and __smp_store_release() for coldboot_done flag > > and use coldboot_lock only for coldboot_wait_hmask. > > > > Signed-off-by: Anup Patel <anup.patel@wdc.com> > > Tested-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > Reviewed-by: Atish Patra <atish.patra@wdc.com> > > --- > > lib/sbi/sbi_init.c | 20 +++++++++++++------- > > 1 file changed, 13 insertions(+), 7 deletions(-) > > > > Reviewed-by: Bin Meng <bin.meng@windriver.com> Applied this patch to the riscv/opensbi repo Regards, Anup
diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c index a7fb848..c438eaa 100644 --- a/lib/sbi/sbi_init.c +++ b/lib/sbi/sbi_init.c @@ -9,6 +9,7 @@ #include <sbi/riscv_asm.h> #include <sbi/riscv_atomic.h> +#include <sbi/riscv_barrier.h> #include <sbi/riscv_locks.h> #include <sbi/sbi_console.h> #include <sbi/sbi_ecall.h> @@ -85,9 +86,10 @@ static void sbi_boot_prints(struct sbi_scratch *scratch, u32 hartid) } static spinlock_t coldboot_lock = SPIN_LOCK_INITIALIZER; -static unsigned long coldboot_done = 0; static struct sbi_hartmask coldboot_wait_hmask = { 0 }; +static unsigned long coldboot_done; + static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid) { unsigned long saved_mie, cmip; @@ -105,16 +107,20 @@ static void wait_for_coldboot(struct sbi_scratch *scratch, u32 hartid) /* Mark current HART as waiting */ sbi_hartmask_set_hart(hartid, &coldboot_wait_hmask); + /* Release coldboot lock */ + spin_unlock(&coldboot_lock); + /* Wait for coldboot to finish using WFI */ - while (!coldboot_done) { - spin_unlock(&coldboot_lock); + while (!__smp_load_acquire(&coldboot_done)) { do { wfi(); cmip = csr_read(CSR_MIP); } while (!(cmip & MIP_MSIP)); - spin_lock(&coldboot_lock); }; + /* Acquire coldboot lock */ + spin_lock(&coldboot_lock); + /* Unmark current HART as waiting */ sbi_hartmask_clear_hart(hartid, &coldboot_wait_hmask); @@ -132,12 +138,12 @@ static void wake_coldboot_harts(struct sbi_scratch *scratch, u32 hartid) { const struct sbi_platform *plat = sbi_platform_ptr(scratch); + /* Mark coldboot done */ + __smp_store_release(&coldboot_done, 1); + /* Acquire coldboot lock */ spin_lock(&coldboot_lock); - /* Mark coldboot done */ - coldboot_done = 1; - /* Send an IPI to all HARTs waiting for coldboot */ for (int i = 0; i <= sbi_scratch_last_hartid(); i++) { if ((i != hartid) &&