Re: usb mem wait
On Mon, Nov 22, 2010 at 09:09:17AM -0500, Ted Unangst wrote:
> On Mon, Nov 22, 2010 at 12:06 AM, Jacob Meuser
> wrote:
> > On Sun, Nov 21, 2010 at 06:47:01PM -0500, Ted Unangst wrote:
> >> instead of faking an assertwaitok check, let's use the real thing. this
> >> is almost the opposite of progress on the whole bluetooth issue, but it
> >> shortens the stack trace considerably. the curproc check isn't terribly
> >> accurate, either, so it misses bugs.
> >>
> >> i don't see any reason to avoid sleeping once we've decided that waiting
> >> is ok, so i changed that too.
> >
> > where was it decided that waiting is ok? because it's not supposed to
> > be interrupt context? I dunno about that. this function makes it into
> > a lot of places, and I don't see any that wait.
>
> Well, it is waiting! running btconfig up will panic later down the
> chain, with this function on the call stack. Sorry, don't have the
> exact trace handy. And it's already checking for curproc. That's
> just a broken test for the same thing.
I don't understand the purpose of this change. do you plan to change
the rest of the stack (usb_allocmem, *hci_allocm, etc) that calls
this function to wait for memory? are you sure the only reason for
the M_NOWAITs is to not wait in interrupt context? I'm not saying
this change is necessarily wrong, but by itself, it raises questions.
> >
> >
> >> Index: usb_mem.c
> >> ===
> >> RCS file: /home/tedu/cvs/src/sys/dev/usb/usb_mem.c,v
> >> retrieving revision 1.22
> >> diff -u -r1.22 usb_mem.c
> >> --- usb_mem.c 29 Sep 2010 20:06:38 - 1.22
> >> +++ usb_mem.c 21 Nov 2010 23:15:26 -
> >> @@ -98,12 +98,7 @@
> >> DPRINTFN(5, ("usb_block_allocmem: size=%lu align=%lu\n",
> >>(u_long)size, (u_long)align));
> >>
> >> -#ifdef DIAGNOSTIC
> >> - if (!curproc) {
> >> - printf("usb_block_allocmem: in interrupt context,
> size=%lu\n",
> >> - (unsigned long) size);
> >> - }
> >> -#endif
> >> + assertwaitok();
> >>
> >> s = splusb();
> >> /* First check the free list. */
> >> @@ -120,17 +115,8 @@
> >> }
> >> splx(s);
> >>
> >> -#ifdef DIAGNOSTIC
> >> - if (!curproc) {
> >> - printf("usb_block_allocmem: in interrupt context,
> failed\n");
> >> - return (USBD_NOMEM);
> >> - }
> >> -#endif
> >> -
> >> DPRINTFN(6, ("usb_block_allocmem: no free\n"));
> >> - p = malloc(sizeof *p, M_USB, M_NOWAIT);
> >> - if (p == NULL)
> >> - return (USBD_NOMEM);
> >> + p = malloc(sizeof *p, M_USB, M_WAITOK);
> >>
> >> p->tag = tag;
> >> p->size = size;
> >
> > --
> > jake...@sdf.lonestar.org
> > SDF Public Access UNIX System - http://sdf.lonestar.org
--
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org
Re: usb mem wait
On Mon, Nov 22, 2010 at 12:06 AM, Jacob Meuser
wrote:
> On Sun, Nov 21, 2010 at 06:47:01PM -0500, Ted Unangst wrote:
>> instead of faking an assertwaitok check, let's use the real thing. this
>> is almost the opposite of progress on the whole bluetooth issue, but it
>> shortens the stack trace considerably. the curproc check isn't terribly
>> accurate, either, so it misses bugs.
>>
>> i don't see any reason to avoid sleeping once we've decided that waiting
>> is ok, so i changed that too.
>
> where was it decided that waiting is ok? because it's not supposed to
> be interrupt context? I dunno about that. this function makes it into
> a lot of places, and I don't see any that wait.
Well, it is waiting! running btconfig up will panic later down the
chain, with this function on the call stack. Sorry, don't have the
exact trace handy. And it's already checking for curproc. That's
just a broken test for the same thing.
>
>
>> Index: usb_mem.c
>> ===
>> RCS file: /home/tedu/cvs/src/sys/dev/usb/usb_mem.c,v
>> retrieving revision 1.22
>> diff -u -r1.22 usb_mem.c
>> --- usb_mem.c 29 Sep 2010 20:06:38 - 1.22
>> +++ usb_mem.c 21 Nov 2010 23:15:26 -
>> @@ -98,12 +98,7 @@
>> DPRINTFN(5, ("usb_block_allocmem: size=%lu align=%lu\n",
>>(u_long)size, (u_long)align));
>>
>> -#ifdef DIAGNOSTIC
>> - if (!curproc) {
>> - printf("usb_block_allocmem: in interrupt context,
size=%lu\n",
>> - (unsigned long) size);
>> - }
>> -#endif
>> + assertwaitok();
>>
>> s = splusb();
>> /* First check the free list. */
>> @@ -120,17 +115,8 @@
>> }
>> splx(s);
>>
>> -#ifdef DIAGNOSTIC
>> - if (!curproc) {
>> - printf("usb_block_allocmem: in interrupt context,
failed\n");
>> - return (USBD_NOMEM);
>> - }
>> -#endif
>> -
>> DPRINTFN(6, ("usb_block_allocmem: no free\n"));
>> - p = malloc(sizeof *p, M_USB, M_NOWAIT);
>> - if (p == NULL)
>> - return (USBD_NOMEM);
>> + p = malloc(sizeof *p, M_USB, M_WAITOK);
>>
>> p->tag = tag;
>> p->size = size;
>
> --
> jake...@sdf.lonestar.org
> SDF Public Access UNIX System - http://sdf.lonestar.org
Re: usb mem wait
On Sun, Nov 21, 2010 at 06:47:01PM -0500, Ted Unangst wrote:
> instead of faking an assertwaitok check, let's use the real thing. this
> is almost the opposite of progress on the whole bluetooth issue, but it
> shortens the stack trace considerably. the curproc check isn't terribly
> accurate, either, so it misses bugs.
>
> i don't see any reason to avoid sleeping once we've decided that waiting
> is ok, so i changed that too.
where was it decided that waiting is ok? because it's not supposed to
be interrupt context? I dunno about that. this function makes it into
a lot of places, and I don't see any that wait.
> Index: usb_mem.c
> ===
> RCS file: /home/tedu/cvs/src/sys/dev/usb/usb_mem.c,v
> retrieving revision 1.22
> diff -u -r1.22 usb_mem.c
> --- usb_mem.c 29 Sep 2010 20:06:38 - 1.22
> +++ usb_mem.c 21 Nov 2010 23:15:26 -
> @@ -98,12 +98,7 @@
> DPRINTFN(5, ("usb_block_allocmem: size=%lu align=%lu\n",
>(u_long)size, (u_long)align));
>
> -#ifdef DIAGNOSTIC
> - if (!curproc) {
> - printf("usb_block_allocmem: in interrupt context, size=%lu\n",
> - (unsigned long) size);
> - }
> -#endif
> + assertwaitok();
>
> s = splusb();
> /* First check the free list. */
> @@ -120,17 +115,8 @@
> }
> splx(s);
>
> -#ifdef DIAGNOSTIC
> - if (!curproc) {
> - printf("usb_block_allocmem: in interrupt context, failed\n");
> - return (USBD_NOMEM);
> - }
> -#endif
> -
> DPRINTFN(6, ("usb_block_allocmem: no free\n"));
> - p = malloc(sizeof *p, M_USB, M_NOWAIT);
> - if (p == NULL)
> - return (USBD_NOMEM);
> + p = malloc(sizeof *p, M_USB, M_WAITOK);
>
> p->tag = tag;
> p->size = size;
--
jake...@sdf.lonestar.org
SDF Public Access UNIX System - http://sdf.lonestar.org
usb mem wait
instead of faking an assertwaitok check, let's use the real thing. this
is almost the opposite of progress on the whole bluetooth issue, but it
shortens the stack trace considerably. the curproc check isn't terribly
accurate, either, so it misses bugs.
i don't see any reason to avoid sleeping once we've decided that waiting
is ok, so i changed that too.
Index: usb_mem.c
===
RCS file: /home/tedu/cvs/src/sys/dev/usb/usb_mem.c,v
retrieving revision 1.22
diff -u -r1.22 usb_mem.c
--- usb_mem.c 29 Sep 2010 20:06:38 - 1.22
+++ usb_mem.c 21 Nov 2010 23:15:26 -
@@ -98,12 +98,7 @@
DPRINTFN(5, ("usb_block_allocmem: size=%lu align=%lu\n",
(u_long)size, (u_long)align));
-#ifdef DIAGNOSTIC
- if (!curproc) {
- printf("usb_block_allocmem: in interrupt context, size=%lu\n",
- (unsigned long) size);
- }
-#endif
+ assertwaitok();
s = splusb();
/* First check the free list. */
@@ -120,17 +115,8 @@
}
splx(s);
-#ifdef DIAGNOSTIC
- if (!curproc) {
- printf("usb_block_allocmem: in interrupt context, failed\n");
- return (USBD_NOMEM);
- }
-#endif
-
DPRINTFN(6, ("usb_block_allocmem: no free\n"));
- p = malloc(sizeof *p, M_USB, M_NOWAIT);
- if (p == NULL)
- return (USBD_NOMEM);
+ p = malloc(sizeof *p, M_USB, M_WAITOK);
p->tag = tag;
p->size = size;
