Hi Jerome,
On Tuesday 27 March 2012 12:45:23 Jerome Glisse wrote:
On Tue, Mar 27, 2012 at 11:01 AM, Laurent Pinchart wrote:
On Thursday 22 March 2012 16:58:27 Tomasz Stanislawski wrote:
On 03/22/2012 03:42 PM, Laurent Pinchart wrote:
On Thursday 22 March 2012 14:36:33 Tomasz Stanislawski wrote:
On 03/22/2012 11:50 AM, Laurent Pinchart wrote:
On Thursday 22 March 2012 11:02:23 Laurent Pinchart wrote:
[snip]
> + *copy_vma = vb2_get_vma(vma); > + if (!*copy_vma) { > + printk(KERN_ERR "failed to copy vma\n"); > + ret = -ENOMEM; > + goto cleanup; > + }
Do we really need to make a copy of the VMA ? The only reason why we store a pointer to it is to check the flags in vb2_dc_put_userptr(). We could store the flags instead and avoid vb2_get_dma()/vb2_put_dma() calls altogether.
I remember that there was a very good reason of copying this vma structure. You caught me on 'cargo-cult' programming. I will do some reverse engineering and try to answer it soon.
OK :-) I'm not copying the VMA in the OMAP3 ISP driver, which is why this caught my eyes. If you find the reason why copying it is needed, please add a comment to the code.
The reason of copying vma was that 'struct vma' has no reference counters. Therefore it could be deleted after mm lock is freed, ending with freeing its all pages belonging to vma. To prevent it, a copy of vma is created. Notice that inside vb2_get_vma the callback open is called for original vma, preventing memory from being released. On vb2_put_vma the complementary close is called.
Feel free to prove me wrong, but I think get_user_pages() is enough to prevent the pages from being freed, even if the VMA is deleted.
However, there's one subtle issue that we will need to deal with when we will implement cache management. It took me a lot of time to debug and fix it when I was working on the OMAP3 ISP driver, so I'll explain it in the hope that someone will find it later when dealing with the same problems :-)
When a buffer is queued, the OMAP3 ISP driver cleans up the cache using the userspace mapping addresses (for USERPTR buffers). This might be a bad thing, but that's the way we currently implement that.
A prior get_user_pages() call will make sure pages are not freed, but due to optimization in the lockless memory management algorithms the userspace mapping can disappear: the kernel might consider that a page can be freed, remove its userspace mapping, and then find out that the page is locked. It will then move on without restoring the userspace mapping, which will be restored when the next page fault occurs.
When cleaning the cache using the userspace mapping addresses, any page for which the userspace mapping has been removed will trigger a page fault. The page fault handler (do_page_fault() in arm/arch/mm/fault.c) will try to read_lock mmap_sem. If it fails, it will check if the page fault occured in userspace context, or from a known exception location. As neither condition is true, it will panic.
The solution I found to this issue was to lock the VMA. This ensured that the userspace mapping would stay in place. See isp_video_buffer_lock_vma() in drivers/media/video/omap3isp/ispqueue.c. You could use a similar approach here if you want to ensure that userspace mappings are not removed, but once again I don't think that's needed (until we get to cache handling) as get_user_pages() will ensure that the pages are not freed.
I think the proper solution is to not use any user allocated memory and always use dma-buf. Some evil process thread might unlock the vma behind your back and you back to the original issue.
The linux memory management is not designed to easily allow use of user allocated memory by a device to do dma to/from it, at least not for the usecase where dma operation might happen over long period of time.
I guess some VMA change might help but this might also be hurt full and i am not familiar enough with the whole memory management to venture a guess on what kind if implication there is.
I agree with you, we should move away from using user-allocated memory. I just wanted to explain what I did and why for reference purpose, as it took me lots of time to debug the issue. Until every driver moves to dma-buf (and for some time after as well) we will still need to support user-allocated memory in V4L2, even if the solution isn't completely bullet-proof.