Date: Tue, 18 Aug 2020 10:04:19 -0400 From: Mark Johnston <markj@freebsd.org> To: Mateusz Guzik <mjguzik@gmail.com> Cc: current@freebsd.org Subject: Re: "panic: malloc(M_WAITOK) in non-sleepable context" after r364296 -> r364341 Message-ID: <20200818140419.GB28906@raichu> In-Reply-To: <CAGudoHF0xTrw8h24iUOMr6YdL-z2Mce1z_ur0=bRvMQL1PD0Sg@mail.gmail.com> References: <20200818124419.GO1394@albert.catwhisker.org> <CAGudoHEEnZVaBwY1L-wjmDcrsHyP4pE3-0qyurh-kWsCO9Edhg@mail.gmail.com> <20200818125825.GP1394@albert.catwhisker.org> <CAGudoHHJLoQhU%2Bb4%2Btt_dmChYj=qXA%2BiVaC5mFW8sFxoqq54pw@mail.gmail.com> <20200818134900.GA28906@raichu> <CAGudoHF0xTrw8h24iUOMr6YdL-z2Mce1z_ur0=bRvMQL1PD0Sg@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Aug 18, 2020 at 03:55:15PM +0200, Mateusz Guzik wrote:
> On 8/18/20, Mark Johnston <markj@freebsd.org> wrote:
> > On Tue, Aug 18, 2020 at 03:24:52PM +0200, Mateusz Guzik wrote:
> >> Try this:
> >>
> >> diff --git a/sys/kern/kern_malloc.c b/sys/kern/kern_malloc.c
> >> index 46eb1c4347c..7b94ca7b880 100644
> >> --- a/sys/kern/kern_malloc.c
> >> +++ b/sys/kern/kern_malloc.c
> >> @@ -618,8 +618,8 @@ void *
> >> unsigned long osize = size;
> >> #endif
> >>
> >> - KASSERT((flags & M_WAITOK) == 0 || THREAD_CAN_SLEEP(),
> >> - ("malloc(M_WAITOK) in non-sleepable context"));
> >> + if ((flags & M_WAITOK) != 0)
> >> + WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, NULL,
> >> __func__);
> >>
> >> #ifdef MALLOC_DEBUG
> >> va = NULL;
> >> diff --git a/sys/vm/uma_core.c b/sys/vm/uma_core.c
> >> index 37d78354200..2e1267ec02f 100644
> >> --- a/sys/vm/uma_core.c
> >> +++ b/sys/vm/uma_core.c
> >> @@ -3355,8 +3355,8 @@ uma_zalloc_arg(uma_zone_t zone, void *udata, int
> >> flags)
> >> uma_cache_bucket_t bucket;
> >> uma_cache_t cache;
> >>
> >> - KASSERT((flags & M_WAITOK) == 0 || THREAD_CAN_SLEEP(),
> >> - ("uma_zalloc(M_WAITOK) in non-sleepable context"));
> >> + if ((flags & M_WAITOK) != 0)
> >> + WITNESS_WARN(WARN_GIANTOK | WARN_SLEEPOK, NULL,
> >> __func__);
> >>
> >> /* Enable entropy collection for RANDOM_ENABLE_UMA kernel option
> >> */
> >> random_harvest_fast_uma(&zone, sizeof(zone), RANDOM_UMA);
> >
> > Doesn't it only print a warning if non-sleepable locks are held?
> > THREAD_CAN_SLEEP() catches other cases, like epoch sections and worker
> > threads that are not allowed to sleep (CAM doneq threads in this case).
> > Otherwise uma_zalloc_debug() already checks with WITNESS.
> >
> > I'm inclined to simply revert the commit for now. Alternately,
> > disk_alloc() could be modified to handle M_NOWAIT/M_WAITOK flags, and
> > that could be used in daregister() and other CAM periph drivers.
> > daregister() already uses M_NOWAIT to allocate the driver softc itself.
> >
>
> If WITNESS_WARN(.., WARN_SLEEPOK) does not check for all possible
> blockers for going off CPU that's a bug. I do support reverting the
> patch for now until the dust settles. I don't propose the above hack
> as the final fix.
Well, IMO the bug is that we have no subroutine to perform all of these
checks (which includes those done by WITNESS_WARN(..., WARN_SLEEPOK)).
WITNESS' responsibilities are more narrow. I just meant that your patch
effectively reverts Gleb's commit.
> As for the culrpit at hand, given the backtrace:
> devfs_alloc() at devfs_alloc+0x28/frame 0xffffffff8218d570
> make_dev_sv() at make_dev_sv+0x97/frame 0xffffffff8218d600
> make_dev_s() at make_dev_s+0x3b/frame 0xffffffff8218d660
> passregister() at passregister+0x3e7/frame 0xffffffff8218d8b0
>
> I think this is missing wait flags, resulting in M_WAITOK later, i.e.:
Ah, I was looking at a different report. All the more reason to revert
for now.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20200818140419.GB28906>