On 6/7/19 8:45 AM, Hans Verkuil wrote: > On 6/7/19 8:11 AM, Tomasz Figa wrote: >> On Wed, May 22, 2019 at 7:56 PM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote: >>>> I share the same experience. Bitstream buffers are usually so small that >>>> you can always find a physically contiguous memory region for them and a >>>> memcpy() will be faster than the overhead of getting an IOMMU involved. >>>> This obviously depends on the specific hardware, but there's always some >>>> threshold before which mapping through an IOMMU just doesn't make sense >>>> from a fragmentation and/or performance point of view. >>>> >>>> I wonder, though, if it's not possible to keep userptr buffers around >>>> and avoid the constant mapping/unmapping. If we only performed cache >>>> maintenance on them as necessary, perhaps that could provide a viable, >>>> maybe even good, zero-copy mechanism. >>> >>> The vb2 framework will keep the mapping for a userptr as long as userspace >>> uses the same userptr for every buffer. >>> >>> I.e. the first time a buffer with index I is queued the userptr is mapped. >>> If that buffer is later dequeued and then requeued again with the same >>> userptr the vb2 core will reuse the old mapping. Otherwise it will unmap >>> and map again with the new userptr. >> >> That's a good point. I forgot that we've been seeing random memory >> corruptions (fortunately of the userptr memory only, not random system >> memory) because of this behavior and carrying a patch in all >> downstream branches to remove this caching. >> >> I can see that we keep references on the pages that corresponded to >> the user VMA at the time the buffer was queued, but are we guaranteed >> that the list of pages backing that VMA hasn't changed over time? > > Since you are seeing memory corruptions, the answer to this is perhaps 'no'? > > I think the (quite possibly faulty) reasoning was that while memory is mapped, > userspace can't do a free()/malloc() pair and end up with the same address. > > I suspect this might be a wrong assumption, and in that case we're better off > removing this check. > > But I'd like to have some confirmation that it is really wrong. I did some testing, and indeed, this doesn't work. A patch fixing this will be posted soon. Regards, Hans > > USERPTR isn't used very often, so it wouldn't surprise me if it is buggy. > > Regards, > > Hans > >> >>> >>> The same is done for dmabuf, BTW. So if userspace keeps changing dmabuf >>> fds for each buffer, then that is not optimal. >> >> We could possibly try to search through the other buffers and reuse >> the mapping if there is a match? >> >> Best regards, >> Tomasz >> >
