Bug 12449
| Summary: | uvcvideo memory allocation fails | ||
|---|---|---|---|
| Product: | Drivers | Reporter: | Johannes Berg (johannes) |
| Component: | Other | Assignee: | Laurent Pinchart (laurent.pinchart+bugzilla-kernel) |
| Status: | RESOLVED CODE_FIX | ||
| Severity: | normal | CC: | akpm, laurent.pinchart+bugzilla-kernel, mchehab |
| Priority: | P1 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Kernel Version: | 2.6.29-rc1 | Subsystem: | |
| Regression: | Yes | Bisected commit-id: | |
| Attachments: |
patch to retry allocations
Retry URB buffers allocation Retry URB buffers allocation |
||
yeah, that sucks. Normally what code like this will do is to try successively smaller allocations until one works. But you do need to set __GFP_NOWARN when doing this, to avoid the kernel log spew. That would require changing uvc_alloc_urb_buffers() to take a gfp_t, but that's a good change anyway. Created attachment 19821 [details]
patch to retry allocations
Andrew, thanks for the advice. By the way, uvc_alloc_urb_buffers() is not required to take a gfp_t argument, as it will only have to allocate buffers when called with GFP_KERNEL (buffers are already allocated in the resume path that use GFP_NOIO). It wouldn't hurt, but that would be a bit pointless. Johannes, thanks for the patch. Is there a specific reason why you changed the number of packets from 40 to 64 instead of, say, 32 ? Warning: that patch is broken, but I haven't figured out what is uvc_init_video_isoc and uvc_init_video_bulk take a gfp_t argument, so either that needs to be removed or propagated through as in my patch. I changed the max number of packets so that I wouldn't allocate less, and I wanted to go to a power of two. If you think 32 is good, then I can do that. Created attachment 19873 [details]
Retry URB buffers allocation
Retry URB buffers allocation by dividing the number of packets by 2 until allocation succeeds.
This patch is based on Johannes patch and cleans up code and comments to match the new URB buffers allocation behaviour.
The gfp_t argument passed to uvc_init_video_{bulk,isoc} is either GFP_KERNEL when initialising the device or GFP_NOIO when resuming from suspend. As the buffers are not freed during suspend, uvc_alloc_urb_buffers will only allocate memory when called at initialisation time. The gfp_t argument can thus be hardcoded to GFP_KERNEL | __GFP_NOWARN.
Johannes, could you please try this new patch ?
I'll give it a try, though I think I'd prefer, code-wise, to have the gfp_t argument passed through so readers don't have to scratch their head and search for the above explanation, and there'll be no confusion if things ever change. But it's your code, of course. Give me a bit to try this. Works, better than my patch which made it crash the second time around when buffers were already allocated, thanks. I'm not sure my system has memory fragmentation problems yet though. Tested-by: Johannes Berg <johannes@sipsolutions.net> And since I just reviewed the patch too: Reviewed-by: Johannes Berg <johannes@sipsolutions.net> But I think your argument of GFP_KERNEL vs. GFP_NOIO doesn't add up, why does it need to do anything at all when resuming? Does it need to reallocate urbs, but not buffers? In any case, as I said above I think that should be changed or at least documented so casual readers of the code don't get confused. Changed to pass gfp_t through would probably be best, it doesn't change anything, doesn't cost anything but makes the code less prone to breaking with future changes. Created attachment 19881 [details]
Retry URB buffers allocation
Buffers are not freed when suspending, so they don't need to be reallocated when resuming.
Anyway, here's a patch that propagates the gfp_t argument to uvc_alloc_urb_buffers. I'll push it upstream through the V4L/DVB tree.
Is there a reason this patch is not in a kernel tree near me yet? It fixes a rather annoying bug and as such should be in .29 imho. I've sent a pull request to Mauro on 2009-01-22. This was too late for 2.6.29, so the patch is sitting in the v4l-dvb tree waiting for the 2.6.30 merge window to open. D'oh. Can this patch not just go through as a bugfix on its own? |
Latest working kernel version: n/a Earliest failing kernel version: 2.6.28 Distribution: debian/unstable Hardware Environment: macbook5,1 Every once a while I try using my webcam, I get a memory allocation failure because uvcvideo tries to do an order-5 allocation, and fails due to memory fragmentation. I "fixed" this with this patch which limits it, on my machine, to an order-2 allocation: --- a/drivers/media/video/uvc/uvc_video.c +++ b/drivers/media/video/uvc/uvc_video.c @@ -761,8 +761,8 @@ static int uvc_init_video_isoc(struct uvc_video_device *vide return -EINVAL; npackets = DIV_ROUND_UP(size, psize); - if (npackets > UVC_MAX_ISO_PACKETS) - npackets = UVC_MAX_ISO_PACKETS; + if (npackets > 4) + npackets = 4; size = npackets * psize;