git: 7eaea04a5bb1 - main - amd64: compare TLB shootdown target to all_cpus

[Ryan Libby]

On Mon, Jan 11, 2021 at 10:22 PM Ryan Libby <rlibby at freebsd.org> wrote:
>
> On Mon, Jan 11, 2021 at 9:59 PM Mateusz Guzik <mjguzik at gmail.com> wrote:
> >
> > This makes my 2 core vm crash on boot:
>
> There's some known broken logic in bitset(9) for comparisons and some
> other operations.  I started some tests and a patch to fix it and clean
> up some stuff, but I haven't polished it off:
>
> https://github.com/rlibby/freebsd/commits/bitset
>
> Long story short, it isn't careful not to examine bits in the last word
> that are past the end of the set.
>

Ah, except MAXCPU is a multiple of the word size on amd64, so
this shouldn't be the issue.  Sorry for the noise.

> >
> > Launching APs: 1
> > Timecounter "TSC-low" frequency 1346899854 Hz quality 1000
> > panic: IPI scoreboard is zero, initiator 1 target 1
> > cpuid = 1
> > time = 1
> > KDB: stack backtrace:
> > db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xffffffff80fb7600
> > vpanic() at vpanic+0x181/frame 0xffffffff80fb7650
> > panic() at panic+0x43/frame 0xffffffff80fb76b0
> > smp_targeted_tlb_shootdown() at smp_targeted_tlb_shootdown+0x434/frame
> > 0xffffffff80fb7730
> > pmap_invalidate_range() at pmap_invalidate_range+0x16f/frame 0xffffffff80fb77a0
> > vm_thread_stack_create() at vm_thread_stack_create+0x45/frame 0xffffffff80fb78e0
> > kstack_import() at kstack_import+0x4e/frame 0xffffffff80fb7910
> > cache_alloc() at cache_alloc+0x3bf/frame 0xffffffff80fb7970
> > cache_alloc_retry() at cache_alloc_retry+0x25/frame 0xffffffff80fb79b0
> > vm_thread_new() at vm_thread_new+0x3d/frame 0xffffffff80fb79d0
> > thread_alloc() at thread_alloc+0x50/frame 0xffffffff80fb7a00
> > kthread_add() at kthread_add+0x42/frame 0xffffffff80fb7aa0
> > kproc_kthread_add() at kproc_kthread_add+0xaf/frame 0xffffffff80fb7bc0
> > intr_event_add_handler() at intr_event_add_handler+0x227/frame
> > 0xffffffff80fb7c30
> > start_softintr() at start_softintr+0xea/frame 0xffffffff80fb7c60
> > mi_startup() at mi_startup+0xec/frame 0xffffffff80fb7cb0
> > btext() at btext+0x2c
> > KDB: enter: panic
> >
> >
> > On 1/12/21, Andrew Gallatin <gallatin at freebsd.org> wrote:
> > > The branch main has been updated by gallatin:
> > >
> > > URL:
> > > https://cgit.FreeBSD.org/src/commit/?id=7eaea04a5bb1dc86c43ce046311e1c1a042994d3
> > >
> > > commit 7eaea04a5bb1dc86c43ce046311e1c1a042994d3
> > > Author:     Andrew Gallatin <gallatin at FreeBSD.org>
> > > AuthorDate: 2021-01-12 01:03:37 +0000
> > > Commit:     Andrew Gallatin <gallatin at FreeBSD.org>
> > > CommitDate: 2021-01-12 01:09:32 +0000
> > >
> > >     amd64: compare TLB shootdown target to all_cpus
> > >
> > >     On amd64, the pmap code passes all_cpus to
> > >     smp_targeted_tlb_shootdown() when unmapping from the
> > >     kernel pmap.  This function has an optimized path to send IPIs
> > >     to all but itself, which it intends to do when the target
> > >     is all cpus.   However, we need to compare the target cpu mask
> > >     with all_cpus, rather than using CPU_ISFULLSET().  Comparing with
> > >     CPU_ISFULLSET() will only work when we have MAXCPU cpus active in
> > >     the system, otherwise, we'll be sending repeated IPIs, rather than
> > >     a single IPI to all CPUs but ourself.
> > >
> > >     Fixing this should reduce the time spent in native_lapic_ipi_wait()
> > >     as we will be sending ipis in parallel, rather than one-by-one.
> > >     This is confirmed by dtrace.
> > >
> > >     Reviewed by: alc, jhb, kib, markj
> > >     Sponsored by:   Netflix
> > >     Differential Revision:  https://reviews.freebsd.org/D28102
> > > ---
> > >  sys/amd64/amd64/mp_machdep.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/sys/amd64/amd64/mp_machdep.c b/sys/amd64/amd64/mp_machdep.c
> > > index 63777014e151..794a11bf1276 100644
> > > --- a/sys/amd64/amd64/mp_machdep.c
> > > +++ b/sys/amd64/amd64/mp_machdep.c
> > > @@ -673,7 +673,7 @@ smp_targeted_tlb_shootdown(cpuset_t mask, pmap_t pmap,
> > > vm_offset_t addr1,
> > >       /*
> > >        * Check for other cpus.  Return if none.
> > >        */
> > > -     if (CPU_ISFULLSET(&mask)) {
> > > +     if (!CPU_CMP(&mask, &all_cpus)) {
> > >               if (mp_ncpus <= 1)
> > >                       goto local_cb;
> > >       } else {
> > > @@ -719,7 +719,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_ISFULLSET(&mask)) {
> > > +     if (!CPU_CMP(&mask, &all_cpus)) {
> > >               ipi_all_but_self(IPI_INVLOP);
> > >               other_cpus = all_cpus;
> > >               CPU_CLR(PCPU_GET(cpuid), &other_cpus);
> > >
> >
> >
> > --
> > Mateusz Guzik <mjguzik gmail.com>