Re: [Linaro-mm-sig] [RFCv2 PATCH 2/9 - 4/4] v4l: vb2-dma-contig: update and code refactoring

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

-- 
Regards,

Laurent Pinchart