"panic: malloc(M_WAITOK) in non-sleepable context" after r364296 -> r364341

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|

"panic: malloc(M_WAITOK) in non-sleepable context" after r364296 -> r364341

David Wolfskill
Just did a src update from r364296 -> r364341 on amd64 on my bild
machine (laptop is still working -- it built firefox earlier, which
takes some time).  On initial reboot, the build machine panicked:

umass0:  SCSI over Bulk-Only; quirks = 0x4000
umass0:6:0: Attached to scbus6
(probe0:umass-sim0:0:0:0): Down reving Protocol Version from 2 to 0?
panic: malloc(M_WAITOK) in non-sleepable context
cpuid = 1
time = 20
KDB: stack backtrace:
db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame 0xffffffff8218d440
vpanic() at vpanic+0x182/frame 0xffffffff8218d490
panic() at panic+0x43/frame 0xffffffff8218d4f0
malloc() at malloc+0x20c/frame 0xffffffff8218d540
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
cam_periph_alloc() at cam_periph_alloc+0x57b/frame 0xffffffff8218d980
passasync() at passasync+0x5c/frame 0xffffffff8218d9c0
xpt_async_process_dev() at xpt_async_process_dev+0x152/frame 0xffffffff8218da10
xpt_async_process() at xpt_async_process+0x334/frame 0xffffffff8218db20
xpt_done_process() at xpt_done_process+0x3a3/frame 0xffffffff8218db60
xpt_done_td() at xpt_done_td+0xf5/frame 0xffffffff8218dbb0
fork_exit() at fork_exit+0x80/frame 0xffffffff8218dbf0
fork_trampoline() at fork_trampoline+0xe/frame 0xffffffff8218dbf0
--- trap 0, rip = 0, rsp = 0, rbp = 0 ---
KDB: enter: panic
[ thread pid 16 tid 100068 ]
Stopped at      kdb_enter+0x37: movq    $0,0x10b70f6(%rip)
db>


So -- it's at the debugger prompt; given sufficient hints, I'm happy
to poke at it & report back.

Yesterday's (r364296) verbose dmesg.boot is at
http://www.catwhisker.org/~david/FreeBSD/history/freebeast.13_dmesg.txt
(in case that's useful).

Peace,
david
--
David H. Wolfskill [hidden email]
Donald Trump is either ignorant of how to govern or is refusing to do so.

See http://www.catwhisker.org/~david/publickey.gpg for my public key.

signature.asc (631 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: "panic: malloc(M_WAITOK) in non-sleepable context" after r364296 -> r364341

Mateusz Guzik
see https://reviews.freebsd.org/D26027

On 8/18/20, David Wolfskill <[hidden email]> wrote:

> Just did a src update from r364296 -> r364341 on amd64 on my bild
> machine (laptop is still working -- it built firefox earlier, which
> takes some time).  On initial reboot, the build machine panicked:
>
> umass0:  SCSI over Bulk-Only; quirks = 0x4000
> umass0:6:0: Attached to scbus6
> (probe0:umass-sim0:0:0:0): Down reving Protocol Version from 2 to 0?
> panic: malloc(M_WAITOK) in non-sleepable context
> cpuid = 1
> time = 20
> KDB: stack backtrace:
> db_trace_self_wrapper() at db_trace_self_wrapper+0x2b/frame
> 0xffffffff8218d440
> vpanic() at vpanic+0x182/frame 0xffffffff8218d490
> panic() at panic+0x43/frame 0xffffffff8218d4f0
> malloc() at malloc+0x20c/frame 0xffffffff8218d540
> 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
> cam_periph_alloc() at cam_periph_alloc+0x57b/frame 0xffffffff8218d980
> passasync() at passasync+0x5c/frame 0xffffffff8218d9c0
> xpt_async_process_dev() at xpt_async_process_dev+0x152/frame
> 0xffffffff8218da10
> xpt_async_process() at xpt_async_process+0x334/frame 0xffffffff8218db20
> xpt_done_process() at xpt_done_process+0x3a3/frame 0xffffffff8218db60
> xpt_done_td() at xpt_done_td+0xf5/frame 0xffffffff8218dbb0
> fork_exit() at fork_exit+0x80/frame 0xffffffff8218dbf0
> fork_trampoline() at fork_trampoline+0xe/frame 0xffffffff8218dbf0
> --- trap 0, rip = 0, rsp = 0, rbp = 0 ---
> KDB: enter: panic
> [ thread pid 16 tid 100068 ]
> Stopped at      kdb_enter+0x37: movq    $0,0x10b70f6(%rip)
> db>
>
>
> So -- it's at the debugger prompt; given sufficient hints, I'm happy
> to poke at it & report back.
>
> Yesterday's (r364296) verbose dmesg.boot is at
> http://www.catwhisker.org/~david/FreeBSD/history/freebeast.13_dmesg.txt
> (in case that's useful).
>
> Peace,
> david
> --
> David H. Wolfskill [hidden email]
> Donald Trump is either ignorant of how to govern or is refusing to do so.
>
> See http://www.catwhisker.org/~david/publickey.gpg for my public key.
>


--
Mateusz Guzik <mjguzik gmail.com>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: "panic: malloc(M_WAITOK) in non-sleepable context" after r364296 -> r364341

David Wolfskill
On Tue, Aug 18, 2020 at 02:48:47PM +0200, Mateusz Guzik wrote:
> see https://reviews.freebsd.org/D26027
> ....

Right; I was just reviewing the list of updated files and noted
src/sys/kern/kern_malloc.c, and its svn log showed:

------------------------------------------------------------------------
r364310 | glebius | 2020-08-17 08:37:08 -0700 (Mon, 17 Aug 2020) | 8
lines

With INVARIANTS panic immediately if M_WAITOK is requested in a
non-sleepable context.  Previously only _sleep() would panic.
This will catch misuse of M_WAITOK at development stage rather
than at stress load stage.

Reviewed by:    markj
Differential Revision:  https://reviews.freebsd.org/D26027

------------------------------------------------------------------------

And if we were still going in to the office, I'd discuss it with Gleb.  :-)

But in the mean time: I'm not a committer; I didn't cause the error.
Seems I can't try running head now with a GENERIC kernel.

Peace,
david
--
David H. Wolfskill [hidden email]
Donald Trump is either ignorant of how to govern or is refusing to do so.

See http://www.catwhisker.org/~david/publickey.gpg for my public key.

signature.asc (631 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: "panic: malloc(M_WAITOK) in non-sleepable context" after r364296 -> r364341

Mateusz Guzik
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);


On 8/18/20, David Wolfskill <[hidden email]> wrote:

> On Tue, Aug 18, 2020 at 02:48:47PM +0200, Mateusz Guzik wrote:
>> see https://reviews.freebsd.org/D26027
>> ....
>
> Right; I was just reviewing the list of updated files and noted
> src/sys/kern/kern_malloc.c, and its svn log showed:
>
> ------------------------------------------------------------------------
> r364310 | glebius | 2020-08-17 08:37:08 -0700 (Mon, 17 Aug 2020) | 8
> lines
>
> With INVARIANTS panic immediately if M_WAITOK is requested in a
> non-sleepable context.  Previously only _sleep() would panic.
> This will catch misuse of M_WAITOK at development stage rather
> than at stress load stage.
>
> Reviewed by:    markj
> Differential Revision:  https://reviews.freebsd.org/D26027
>
> ------------------------------------------------------------------------
>
> And if we were still going in to the office, I'd discuss it with Gleb.  :-)
>
> But in the mean time: I'm not a committer; I didn't cause the error.
> Seems I can't try running head now with a GENERIC kernel.
>
> Peace,
> david
> --
> David H. Wolfskill [hidden email]
> Donald Trump is either ignorant of how to govern or is refusing to do so.
>
> See http://www.catwhisker.org/~david/publickey.gpg for my public key.
>


--
Mateusz Guzik <mjguzik gmail.com>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: "panic: malloc(M_WAITOK) in non-sleepable context" after r364296 -> r364341

David Wolfskill
On Tue, Aug 18, 2020 at 03:24:52PM +0200, Mateusz Guzik wrote:
> Try this:
> [replacing new KASSERT() with WITNESS_WARN()...]
> ....

Thanks; on it.  Should take care of it, I think.

Peace,
david
--
David H. Wolfskill [hidden email]
Donald Trump is either ignorant of how to govern or is refusing to do so.

See http://www.catwhisker.org/~david/publickey.gpg for my public key.

signature.asc (631 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: "panic: malloc(M_WAITOK) in non-sleepable context" after r364296 -> r364341

Mateusz Guzik
So the previous patch should just print a warning.

Does this take care of the problem in general? I don't have means to
test the patch.

diff --git a/sys/cam/scsi/scsi_pass.c b/sys/cam/scsi/scsi_pass.c
index b299fbddd84..c6d6a5da403 100644
--- a/sys/cam/scsi/scsi_pass.c
+++ b/sys/cam/scsi/scsi_pass.c
@@ -652,6 +652,7 @@ passregister(struct cam_periph *periph, void *arg)
        args.mda_gid = GID_OPERATOR;
        args.mda_mode = 0600;
        args.mda_si_drv1 = periph;
+       args.mda_flags = MAKEDEV_NOWAIT;
        error = make_dev_s(&args, &softc->dev, "%s%d", periph->periph_name,
            periph->unit_number);
        if (error != 0) {


On 8/18/20, David Wolfskill <[hidden email]> wrote:

> On Tue, Aug 18, 2020 at 03:24:52PM +0200, Mateusz Guzik wrote:
>> Try this:
>> [replacing new KASSERT() with WITNESS_WARN()...]
>> ....
>
> Thanks; on it.  Should take care of it, I think.
>
> Peace,
> david
> --
> David H. Wolfskill [hidden email]
> Donald Trump is either ignorant of how to govern or is refusing to do so.
>
> See http://www.catwhisker.org/~david/publickey.gpg for my public key.
>


--
Mateusz Guzik <mjguzik gmail.com>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: "panic: malloc(M_WAITOK) in non-sleepable context" after r364296 -> r364341

Mark Johnston-2
In reply to this post by Mateusz Guzik
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.

diff --git a/sys/cam/scsi/scsi_da.c b/sys/cam/scsi/scsi_da.c
index d648d511b1a2..d940e7db24e0 100644
--- a/sys/cam/scsi/scsi_da.c
+++ b/sys/cam/scsi/scsi_da.c
@@ -2767,18 +2767,23 @@ daregister(struct cam_periph *periph, void *arg)
  return(CAM_REQ_CMP_ERR);
  }
 
- softc = (struct da_softc *)malloc(sizeof(*softc), M_DEVBUF,
-    M_NOWAIT|M_ZERO);
-
+ softc = malloc(sizeof(*softc), M_DEVBUF, M_NOWAIT | M_ZERO);
  if (softc == NULL) {
  printf("daregister: Unable to probe new device. "
        "Unable to allocate softc\n");
  return(CAM_REQ_CMP_ERR);
  }
+ softc->disk = disk_alloc_flags(M_NOWAIT);
+ if (softc->disk == NULL) {
+ printf("daregister: Unable to probe new device. "
+       "Unable to allocate disk structure\n");
+ return (CAM_REQ_CMP_ERR);
+ }
 
  if (cam_iosched_init(&softc->cam_iosched, periph) != 0) {
  printf("daregister: Unable to probe new device. "
        "Unable to allocate iosched memory\n");
+ disk_destroy(softc->disk);
  free(softc, M_DEVBUF);
  return(CAM_REQ_CMP_ERR);
  }
@@ -2898,7 +2903,6 @@ daregister(struct cam_periph *periph, void *arg)
  /*
  * Register this media as a disk.
  */
- softc->disk = disk_alloc();
  softc->disk->d_devstat = devstat_new_entry(periph->periph_name,
   periph->unit_number, 0,
   DEVSTAT_BS_UNAVAILABLE,
diff --git a/sys/geom/geom_disk.c b/sys/geom/geom_disk.c
index eaba770828d0..c07494ab4ec0 100644
--- a/sys/geom/geom_disk.c
+++ b/sys/geom/geom_disk.c
@@ -850,15 +850,22 @@ g_disk_ident_adjust(char *ident, size_t size)
 }
 
 struct disk *
-disk_alloc(void)
+disk_alloc_flags(int mflag)
 {
  struct disk *dp;
 
- dp = g_malloc(sizeof(struct disk), M_WAITOK | M_ZERO);
- LIST_INIT(&dp->d_aliases);
+ dp = g_malloc(sizeof(struct disk), mflag | M_ZERO);
+ if (dp != NULL)
+ LIST_INIT(&dp->d_aliases);
  return (dp);
 }
 
+struct disk *
+disk_alloc(void)
+{
+ return (disk_alloc_flags(M_WAITOK));
+}
+
 void
 disk_create(struct disk *dp, int version)
 {
diff --git a/sys/geom/geom_disk.h b/sys/geom/geom_disk.h
index 8e2282a09a3a..794ce2cc38bd 100644
--- a/sys/geom/geom_disk.h
+++ b/sys/geom/geom_disk.h
@@ -137,6 +137,7 @@ struct disk {
 #define DISKFLAG_WRITE_PROTECT 0x0100
 
 struct disk *disk_alloc(void);
+struct disk *disk_alloc_flags(int mflag);
 void disk_create(struct disk *disk, int version);
 void disk_destroy(struct disk *disk);
 void disk_gone(struct disk *disk);
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: "panic: malloc(M_WAITOK) in non-sleepable context" after r364296 -> r364341

Mateusz Guzik
On 8/18/20, Mark Johnston <[hidden email]> 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.

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.:
diff --git a/sys/cam/scsi/scsi_pass.c b/sys/cam/scsi/scsi_pass.c
index b299fbddd84..c6d6a5da403 100644
--- a/sys/cam/scsi/scsi_pass.c
+++ b/sys/cam/scsi/scsi_pass.c
@@ -652,6 +652,7 @@ passregister(struct cam_periph *periph, void *arg)
        args.mda_gid = GID_OPERATOR;
        args.mda_mode = 0600;
        args.mda_si_drv1 = periph;
+       args.mda_flags = MAKEDEV_NOWAIT;
        error = make_dev_s(&args, &softc->dev, "%s%d", periph->periph_name,
            periph->unit_number);
        if (error != 0) {


> diff --git a/sys/cam/scsi/scsi_da.c b/sys/cam/scsi/scsi_da.c
> index d648d511b1a2..d940e7db24e0 100644
> --- a/sys/cam/scsi/scsi_da.c
> +++ b/sys/cam/scsi/scsi_da.c
> @@ -2767,18 +2767,23 @@ daregister(struct cam_periph *periph, void *arg)
>   return(CAM_REQ_CMP_ERR);
>   }
>
> - softc = (struct da_softc *)malloc(sizeof(*softc), M_DEVBUF,
> -    M_NOWAIT|M_ZERO);
> -
> + softc = malloc(sizeof(*softc), M_DEVBUF, M_NOWAIT | M_ZERO);
>   if (softc == NULL) {
>   printf("daregister: Unable to probe new device. "
>         "Unable to allocate softc\n");
>   return(CAM_REQ_CMP_ERR);
>   }
> + softc->disk = disk_alloc_flags(M_NOWAIT);
> + if (softc->disk == NULL) {
> + printf("daregister: Unable to probe new device. "
> +       "Unable to allocate disk structure\n");
> + return (CAM_REQ_CMP_ERR);
> + }
>
>   if (cam_iosched_init(&softc->cam_iosched, periph) != 0) {
>   printf("daregister: Unable to probe new device. "
>         "Unable to allocate iosched memory\n");
> + disk_destroy(softc->disk);
>   free(softc, M_DEVBUF);
>   return(CAM_REQ_CMP_ERR);
>   }
> @@ -2898,7 +2903,6 @@ daregister(struct cam_periph *periph, void *arg)
>   /*
>   * Register this media as a disk.
>   */
> - softc->disk = disk_alloc();
>   softc->disk->d_devstat = devstat_new_entry(periph->periph_name,
>    periph->unit_number, 0,
>    DEVSTAT_BS_UNAVAILABLE,
> diff --git a/sys/geom/geom_disk.c b/sys/geom/geom_disk.c
> index eaba770828d0..c07494ab4ec0 100644
> --- a/sys/geom/geom_disk.c
> +++ b/sys/geom/geom_disk.c
> @@ -850,15 +850,22 @@ g_disk_ident_adjust(char *ident, size_t size)
>  }
>
>  struct disk *
> -disk_alloc(void)
> +disk_alloc_flags(int mflag)
>  {
>   struct disk *dp;
>
> - dp = g_malloc(sizeof(struct disk), M_WAITOK | M_ZERO);
> - LIST_INIT(&dp->d_aliases);
> + dp = g_malloc(sizeof(struct disk), mflag | M_ZERO);
> + if (dp != NULL)
> + LIST_INIT(&dp->d_aliases);
>   return (dp);
>  }
>
> +struct disk *
> +disk_alloc(void)
> +{
> + return (disk_alloc_flags(M_WAITOK));
> +}
> +
>  void
>  disk_create(struct disk *dp, int version)
>  {
> diff --git a/sys/geom/geom_disk.h b/sys/geom/geom_disk.h
> index 8e2282a09a3a..794ce2cc38bd 100644
> --- a/sys/geom/geom_disk.h
> +++ b/sys/geom/geom_disk.h
> @@ -137,6 +137,7 @@ struct disk {
>  #define DISKFLAG_WRITE_PROTECT 0x0100
>
>  struct disk *disk_alloc(void);
> +struct disk *disk_alloc_flags(int mflag);
>  void disk_create(struct disk *disk, int version);
>  void disk_destroy(struct disk *disk);
>  void disk_gone(struct disk *disk);
>


--
Mateusz Guzik <mjguzik gmail.com>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: "panic: malloc(M_WAITOK) in non-sleepable context" after r364296 -> r364341

Mark Johnston-2
On Tue, Aug 18, 2020 at 03:55:15PM +0200, Mateusz Guzik wrote:

> On 8/18/20, Mark Johnston <[hidden email]> 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.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: "panic: malloc(M_WAITOK) in non-sleepable context" after r364296 -> r364341

Mateusz Guzik
On 8/18/20, Mark Johnston <[hidden email]> wrote:

> On Tue, Aug 18, 2020 at 03:55:15PM +0200, Mateusz Guzik wrote:
>> On 8/18/20, Mark Johnston <[hidden email]> 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.
>

not exactly, as it still adds something for malloc.

I argued for a dedicated routine to perform all the relevant checks.
Note a dedicated routine instead of a handrolled call to
witness/panic/whatever can also report the exact blockers (if it knows
them), in this case what lock was taken and where. I would not mind
any extras (e.g., critnest).

>> 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.
>


--
Mateusz Guzik <mjguzik gmail.com>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-current
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: "panic: malloc(M_WAITOK) in non-sleepable context" after r364296 -> r364341

David Wolfskill
In reply to this post by Mateusz Guzik
On Tue, Aug 18, 2020 at 03:37:50PM +0200, Mateusz Guzik wrote:
> So the previous patch should just print a warning.
>
> Does this take care of the problem in general? I don't have means to
> test the patch.
> ....

Sorry for the delay, but finished patching & rebuilding, and
(essentially), yes.  (The difference is that I replaced all 3 of the
KASSERT()s from r364310 with the WITNESS_WARN() constructs, rather
than only 2.)

FreeBSD freebeast.catwhisker.org 13.0-CURRENT FreeBSD 13.0-CURRENT #1004 r364296M/364296: Mon Aug 17 06:04:40 PDT 2020     [hidden email]:/common/S4/obj/usr/src/amd64.amd64/sys/GENERIC  amd64 1300109 1300109

FreeBSD freebeast.catwhisker.org 13.0-CURRENT FreeBSD 13.0-CURRENT #1006 r364341M/364341: Tue Aug 18 06:59:10 PDT 2020     [hidden email]:/common/S4/obj/usr/src/amd64.amd64/sys/GENERIC  amd64 1300110 1300110

Peace,
david
--
David H. Wolfskill [hidden email]
Donald Trump is either ignorant of how to govern or is refusing to do so.

See http://www.catwhisker.org/~david/publickey.gpg for my public key.

patch (1K) Download Attachment
signature.asc (631 bytes) Download Attachment