[Adding Intel folks as they might be interested in this discussion]
On Wed, Aug 30, 2017 at 05:51:52PM -0400, Felix Kuehling wrote:
> Hi Jérôme,
>
> I have some questions about the potential range-start-end race you
> mentioned.
>
> On 2017-08-29 07:54 PM, Jérôme Glisse wrote:
> > Note that a lot of existing user feels broken in respect to range_start/
> > range_end. Many user only have range_start() callback but there is nothing
> > preventing them to undo what was invalidated in their range_start() callback
> > after it returns but before any CPU page table update take place.
> >
> > The code pattern use in kvm or umem odp is an example on how to properly
> > avoid such race. In a nutshell use some kind of sequence number and active
> > range invalidation counter to block anything that might undo what the
> > range_start() callback did.
> What happens when we start monitoring an address range after
> invaligate_range_start was called? Sounds like we have to keep track of
> all active invalidations for just such a case, even in address ranges
> that we don't currently care about.
>
> What are the things we cannot do between invalidate_range_start and
> invalidate_range_end? amdgpu calls get_user_pages to re-validate our
> userptr mappings after the invalidate_range_start notifier invalidated
> it. Do we have to wait for invalidate_range_end before we can call
> get_user_pages safely?
Well the whole userptr bo object is somewhat broken from the start.
You never defined the semantic of it ie what is expected. I can
think of 2 differents semantics:
A) a uptr buffer object is a snapshot of a memory at the time of
uptr buffer object creation
B) a uptr buffer object allow GPU to access a range of virtual
address of a process an share coherent view of that range
between CPU and GPU
As it was implemented it is more inline with B but it is not defined
anywhere AFAICT.
Anyway getting back to your questions, it kind of doesn't matter as
you are using GUP ie you are pinning pages except for one scenario
(at least i can only think of one).
Problematic case is race between CPU write to zero page or COW and
GPU driver doing read only GUP:
CPU thread 1 | CPU thread 2
---------------------------------------------------------------------
|
| uptr covering addr A read only
| .... do stuff with A
write fault to addr A |
invalidate_range_start([A, A+1]) | unbind_ttm -> unpin
| validate bo -> GUP -> zero page
lock page table |
replace zero pfn/COW with new page |
unlock page table |
invalidate_range_end([A, A+1]) |
So here the GPU would be using wrong page for the address. How bad
is it is undefined as the semantic of uptr is undefine. Given how it
as been use so far this race is unlikely (i don't think we have many
userspace that use that feature and do fork).
So i would first define the semantic of uptr bo and then i would fix
accordingly the code. Semantic A is easier to implement and you could
just drop the whole mmu_notifier. Maybe it is better to create uptr
buffer object everytime you want to snapshot a range of address. I
don't think the overhead of buffer creation would matter.
If you want to close the race for COW and zero page in case of read
only GUP there is no other way than what KVM or ODP is doing. I had
patchset to simplify all this but i need to bring it back to life.
Note that other thing might race but as you pin the pages they do
not matter. It just mean that if you GUP after range_start() but
before range_end() and before CPU page table update then you pinned
the same old page again and nothing will happen (migrate will fail,
MADV_FREE will nop, ...). So you just did the range_start() callback
for nothing in those cases.
(Sorry for taking so long to answer i forgot your mail yesterday with
all the other discussion going on).
Cheers,
Jérôme
