Date: Tue, 12 Jan 2021 23:22:38 +0100 From: Mateusz Guzik <mjguzik@gmail.com> To: John Baldwin <jhb@freebsd.org> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 44121a0fbee0 - main - amd64: fix tlb shootdown when all cpus are passed in the bitmap Message-ID: <CAGudoHH_A54c=O3-=v0GNax8UakLpV-hpWShZp6=yHKPzf5-Qg@mail.gmail.com> In-Reply-To: <912ef087-e9e8-1bb5-5063-88bccb834db1@FreeBSD.org> References: <202101120854.10C8s6Gr075398@gitrepo.freebsd.org> <912ef087-e9e8-1bb5-5063-88bccb834db1@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On 1/12/21, John Baldwin <jhb@freebsd.org> wrote: > On 1/12/21 12:54 AM, Mateusz Guzik wrote: >> The branch main has been updated by mjg: >> >> URL: >> https://cgit.FreeBSD.org/src/commit/?id=44121a0fbee0272e23bfb90601d177ba9e9cda9b >> >> commit 44121a0fbee0272e23bfb90601d177ba9e9cda9b >> Author: Mateusz Guzik <mjg@FreeBSD.org> >> AuthorDate: 2021-01-12 08:47:32 +0000 >> Commit: Mateusz Guzik <mjg@FreeBSD.org> >> CommitDate: 2021-01-12 08:47:32 +0000 >> >> amd64: fix tlb shootdown when all cpus are passed in the bitmap >> >> Right now the routine leaves the current CPU in the map, later >> tripping >> on an assert when filling in the scoreboard: panic: IPI scoreboard is >> zero, initiator 1 target 1 >> >> Instead pre-check if all CPUs are present in the map and remember >> that >> outcome for later. >> >> Fixes: 7eaea04a5bb1dc86 ("amd64: compare TLB shootdown target to >> all_cpus") >> Reviewed by: kib >> Differential Revision: https://reviews.freebsd.org/D28111 >> --- >> sys/amd64/amd64/mp_machdep.c | 15 ++++++--------- >> 1 file changed, 6 insertions(+), 9 deletions(-) >> >> diff --git a/sys/amd64/amd64/mp_machdep.c b/sys/amd64/amd64/mp_machdep.c >> index 794a11bf1276..aa62c2086fa4 100644 >> --- a/sys/amd64/amd64/mp_machdep.c >> +++ b/sys/amd64/amd64/mp_machdep.c >> @@ -660,6 +660,7 @@ smp_targeted_tlb_shootdown(cpuset_t mask, pmap_t pmap, >> vm_offset_t addr1, >> * (zeroing slot) and reading from it below (wait for >> * acknowledgment). >> */ >> - if (!CPU_CMP(&mask, &all_cpus)) { >> + if (is_all) { >> ipi_all_but_self(IPI_INVLOP); >> other_cpus = all_cpus; >> CPU_CLR(PCPU_GET(cpuid), &other_cpus); > > At this point isn't other_cpus == mask, so we could just drop other_cpus and > use 'mask' > directly now? > The previous iteration is: while ((cpu = CPU_FFS(&mask1)) != 0) { cpu--; CPU_CLR(cpu, &mask1); KASSERT(*invl_scoreboard_slot(cpu) != 0, ("IPI scoreboard is zero, initiator %d target %d", PCPU_GET(cpuid), cpu)); *invl_scoreboard_slot(cpu) = 0; } and the one after that: while ((cpu = CPU_FFS(&other_cpus)) != 0) { cpu--; CPU_CLR(cpu, &other_cpus); p_cpudone = invl_scoreboard_slot(cpu); while (atomic_load_int(p_cpudone) != generation) ia32_pause(); } iow when someone(tm) implements CPU_FOREACH_MASK or similar instead of needlessly modifying it the code will get itself a good cleanup. The point of the change at hand was to fix the immediate problem. -- Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGudoHH_A54c=O3-=v0GNax8UakLpV-hpWShZp6=yHKPzf5-Qg>