Is my understanding correct?
Yes, of hand that sounds correct to me.
The other occasions should just be early bail out to optimize
things under memory pressure.
Christian.
Am 03.10.2018 um 22:31 schrieb Philip Yang:
Hi Christian,
Yes, I agree. I am working on patch 2 to replace get_user_page
with HMM. One problem is in current gfx path, we check if
mmu_invalidation multiple times in amdgpu_cs_ioctl() path after
get_user_page(), amdgpu_cs_parser_bos(),
amdgpu_cs_list_validate(), and amdgpu_cs_submit(). For HMM,
hmm_vma_range_done() has to be called once and only once after
hmm_vma_get_pfns()/hmm_vma_fault(), so I will call
hmm_vma_range_done() inside amdgpu_cs_submit after holding the
mn lock. Is my understanding correct?
Philip
On 2018-10-02 11:05 AM, Christian König wrote:
Checking more code and
documentation and thinking about it over my vacation I think I
have some new conclusions here.
Currently we are using get_user_pages() together with an MMU
notifier to guarantee coherent address space view, because
get_user_pages() works by grabbing a reference to the pages
and ignoring concurrent page table updates.
But HMM uses a different approach by checking the address
space for modifications using hmm_vma_range_done() and
re-trying when the address space has changed.
Now what you are trying to do is to change that into
get_user_pages() and HMM callback and this is what won't work.
We can either use get_user_pages() with MMU notifier or we can
use HMM for the work, but we can't mix and match.
So my initial guess was correct that we just need to change
both sides of the implementation at the same time.
Regards,
Christian.
Am 28.09.2018 um 17:13 schrieb Koenig, Christian:
No it definitely isn't.
We have literally worked month on this with
the core MM developers.
Making sure that we have a consistent page
array is absolutely vital for correct operation.
Please also check Jerome's presentation from
XDC it also perfectly explains why this approach won't
work correctly.
Christian.
For B path, we take
mm->mmap_sem, then call hmm_vma_get_pfns() or
get_user_pages(). This is obvious.
For A path, mmu notifier
mmu_notifier_invalidate_range_start()/mmu_notifier_invalidate_range_end()
is called in many places, and the calling path is quit
complicated inside mm, it's not obvious. I checked many of
the them, for example:
do_munmap()
down_write(&mm->mmap_sem)
arch_unmap()
mpx_notify_unmap()...
zap_bt_entries_mapping()
zap_page_range()
up_write(&mm->mmap_sem)
void zap_page_range(struct vm_area_struct *vma, unsigned
long start,
unsigned long size)
{
struct mm_struct *mm = vma->vm_mm;
struct mmu_gather tlb;
unsigned long end = start + size;
lru_add_drain();
tlb_gather_mmu(&tlb, mm, start, end);
update_hiwater_rss(mm);
mmu_notifier_invalidate_range_start(mm, start, end);
for ( ; vma && vma->vm_start < end; vma
= vma->vm_next)
unmap_single_vma(&tlb, vma, start, end, NULL);
mmu_notifier_invalidate_range_end(mm, start, end);
tlb_finish_mmu(&tlb, start, end);
}
So AFAIK it's okay without invalidate_range_end()
callback.
Regards,
Philip
On 2018-09-28 01:25 AM, Koenig, Christian wrote:
No, that is incorrect as well :)
The mmap_sem isn't necessary taken
during page table updates.
What you could do is replace
get_user_pages() directly with HMM. If I'm not
completely mistaken that should work as expected.
Christian.
I was trying to understand
the way how HMM handle this concurrent issue and how
we handle it in amdgpu_ttm_tt_userptr_needs_pages()
and amdgpu_ttm_tt_affect_userptr(). HMM uses
range->valid flag, we use gtt->mmu_invalidations
and gtt->last_set_pages. Both use the same lock
plus flag idea actually.
Thanks for the information, now I understand fence
ttm_eu_fence_buffer_objects() put to BOs will block
CPU page table update. This is another side of this
concurrent issue I didn't know.
I had same worry that it has issue without
invalidate_range_end() callback as the calling
sequence Felix lists. Now I think it's fine after
taking a look again today because of mm->mmap_sem
usage, this is my understanding:
A path:
down_write(&mm->mmap_sem);
mmu_notifier_invalidate_range_start()
take_lock()
release_lock()
CPU page table update
mmu_notifier_invalidate_range_end()
up_write(&mm->mmap_sem);
B path:
again:
down_read(&mm->mmap_sem);
hmm_vma_get_pfns()
up_read(&mm->mmap_sem);
....
....
take_lock()
if (!hmm_vma_range_done()) {
release_lock()
goto again
}
submit command job...
release_lock()
If you agree, I will submit patch v5 with some minor
changes, and submit another patch to replace
get_user_page() with HMM.
Regards,
Philip
On 2018-09-27 11:36 AM, Christian König wrote:
Yeah, I've read that as
well.
My best guess is that we just need to add a call to
hmm_vma_range_done() after taking the lock and also
replace get_user_pages() with hmm_vma_get_pfns().
But I'm still not 100% sure how all of that is
supposed to work together.
Regards,
Christian.
Am 27.09.2018 um 16:50 schrieb Kuehling, Felix:
I think the answer is
here:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/vm/hmm.rst#n216
Regards,
Felix
At least with
get_user_pages() that is perfectly possible.
For HMM it could
be that this is prevented somehow.
> In this case
you can end up accessing pages which are
invalidated while get_user_pages is in
process.
What’s the
sequence of events you have in mind?
Something like this?
- Page
table is updated and triggers MMU notifier
- amdgpu
MMU notifier runs and waits for pending CS
to finish while holding the read lock
- New
CS starts just after
invalidate_range_start MMU notifier
finishes but before the page table update
is done
- get_user_pages
returns outdated physical addresses
I hope that’s not
actually possible and that get_user_pages
or hmm_vma_fault would block until the
page table update is done. That is,
invalidate_range_start marks the start of
a page table update, and while that update
is in progress, get_user_pages or
hmm_vma_fault block. Jerome, can you
comment on that?
Thanks,
Felix
Yeah I
understand that, but again that won't
work.
In this case
you can end up accessing pages which
are invalidated while get_user_pages
is in process.
> I’m not
planning to change that. I don’t think
there is any need to change it.
>
> Yeah, but when HMM doesn't provide
both the start and the end hock of the
invalidation this way won't work any
more.
>
> So we need to find a solution for
this,
> Christian.
My whole argument is
that you don’t need to hold the read
lock until the invalidate_range_end.
Just read_lock and read_unlock in the
invalidate_range_start function.
Regards,
Felix
Am 27.09.2018 um
15:18 schrieb Kuehling, Felix:
> The problem is
here:
>
>
ttm_eu_fence_buffer_objects(&p->ticket,
&p->validated, p->fence);
>
amdgpu_mn_unlock(p->mn);
>
> We need to hold the lock until
the fence is added to the reservation
object.
>
> Otherwise somebody could have
changed the page tables just in the
moment between the check of
amdgpu_ttm_tt_userptr_needs_pages()
and adding the fence to the
reservation object.
I’m not
planning to change that. I don’t
think there is any need to change
it.
Yeah, but when HMM doesn't provide both
the start and the end hock of the
invalidation this way won't work any
more.
So we need to find a solution for this,
Christian.
Regards,
Felix
Am 27.09.2018 um
13:08 schrieb Kuehling, Felix:
> We double
check that there wasn't any page
table modification while we prepared
the submission and restart the whole
process when there actually was some
update.
>
> The reason why we need to do
this is here:
>
>
ttm_eu_fence_buffer_objects(&p->ticket,
&p->validated, p->fence);
>
amdgpu_mn_unlock(p->mn);
>
> Only after the new fence is
added to the buffer object we can
release the lock so that any
invalidation will now block on our
command submission to finish before
it modifies the page table.
I don’t
see why this requires holding the
read-lock until
invalidate_range_end.
amdgpu_ttm_tt_affect_userptr gets
called while the mn read-lock is
held in invalidate_range_start
notifier.
That's not related to
amdgpu_ttm_tt_affect_userptr(), this
function could actually be called
outside the lock.
The problem is here:
ttm_eu_fence_buffer_objects(&p->ticket,
&p->validated, p->fence);
amdgpu_mn_unlock(p->mn);
We need to hold the lock until the
fence is added to the reservation
object.
Otherwise somebody could have changed
the page tables just in the moment
between the check of
amdgpu_ttm_tt_userptr_needs_pages()
and adding the fence to the
reservation object.
Regards,
Christian.
Regards,
Felix
That
is correct, but take a look what
we do when after calling the
amdgpu_mn_read_lock():
/* No
memory allocation is allowed
while holding the mn lock */
amdgpu_mn_lock(p->mn);
amdgpu_bo_list_for_each_userptr_entry(e,
p->bo_list) {
struct amdgpu_bo
*bo =
ttm_to_amdgpu_bo(e->tv.bo);
if
(amdgpu_ttm_tt_userptr_needs_pages(bo->tbo.ttm))
{
r =
-ERESTARTSYS;
goto
error_abort;
}
}
We double check that there wasn't
any page table modification while
we prepared the submission and
restart the whole process when
there actually was some update.
The reason why we need to do this
is here:
ttm_eu_fence_buffer_objects(&p->ticket,
&p->validated,
p->fence);
amdgpu_mn_unlock(p->mn);
Only after the new fence is added
to the buffer object we can
release the lock so that any
invalidation will now block on our
command submission to finish
before it modifies the page table.
The only other option would be to
add the fence first and then check
if there was any update to the
page tables.
The issue with that approach is
that adding a fence can't be made
undone, so if we find that there
actually was an update to the page
tables we would need to somehow
turn the CS into a dummy (e.g.
overwrite all IBs with NOPs or
something like that) and still
submit it.
Not sure if that is actually
possible.
Regards,
Christian.
Am 27.09.2018 um 10:47 schrieb
Kuehling, Felix:
So back to my
previous question:
>> But
do we really need another lock for
this? Wouldn't the
>>
re-validation of userptr BOs
(currently calling get_user_pages)
force
>>
synchronization with the ongoing
page table invalidation through
the
>>
mmap_sem or other MM locks?
>
> No and
yes. We don't hold any other locks
while doing command submission,
but I expect that HMM has its own
mechanism to prevent that.
>
> Since
we don't modify
amdgpu_mn_lock()/amdgpu_mn_unlock()
we are certainly not using this
mechanism correctly.
The existing
amdgpu_mn_lock/unlock should block
the MMU notifier while a command
submission is in progress. It
should also block command
submission while an MMU notifier
is in progress.
What we lose
with HMM is the ability to hold a
read-lock for the entire duration
of the invalidate_range_start
until invalidate_range_end. As I
understand it, that lock is meant
to prevent new command submissions
while the page tables are being
updated by the kernel. But my
point is, that get_user_pages or
hmm_vma_fault should do the same
kind of thing. Before the command
submission can go ahead, it needs
to update the userptr addresses.
If the page tables are still being
updated, it will block there even
without holding the
amdgpu_mn_read_lock.
Regards,
Felix
No, that
won't work. We would still run
into lock inversion problems.
What we
could do with the scheduler
is to turn submissions into
dummies if we find that the
page tables are now
outdated.
But that
would be really hacky and
I'm not sure if that would
really work in all cases.
I had a chat
with Jerome yesterday. He
pointed out that the new
blockable parameter can be used
to infer whether the MMU
notifier is being called in a
reclaim operation. So if
blockable==true, it should even
be safe to take the BO
reservation lock without
problems. I think with that we
should be able to remove the
read-write locking completely
and go back to locking (or
try-locking for
blockable==false) the
reservation locks in the MMU
notifier?
Regards,
Felix
-----Original Message-----
From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx>
On Behalf Of Christian König
Sent: Saturday, September 15,
2018 3:47 AM
To: Kuehling, Felix <Felix.Kuehling@xxxxxxx>;
Yang, Philip <Philip.Yang@xxxxxxx>;
amd-gfx@xxxxxxxxxxxxxxxxxxxxx;
Jerome Glisse <j.glisse@xxxxxxxxx>
Subject: Re: [PATCH] drm/amdgpu:
use HMM mirror callback to
replace mmu notifier v4
Am 14.09.2018 um 22:21 schrieb
Felix Kuehling:
> On 2018-09-14 01:52 PM,
Christian König wrote:
>> Am 14.09.2018 um 19:47
schrieb Philip Yang:
>>> On 2018-09-14 03:51
AM, Christian König wrote:
>>>> Am 13.09.2018
um 23:51 schrieb Felix Kuehling:
>>>>> On
2018-09-13 04:52 PM, Philip Yang
wrote:
>>>>> [SNIP]
>>>>>> +
amdgpu_mn_read_unlock(amn);
>>>>>> +
>>>>>
amdgpu_mn_read_lock/unlock
support recursive locking for
multiple
>>>>> overlapping
or nested invalidation ranges.
But if you'r locking
>>>>> and
unlocking in the same function.
Is that still a concern?
>>> I don't understand
the possible recursive case, but
>>>
amdgpu_mn_read_lock() still
support recursive locking.
>>>> Well the real
problem is that unlocking them
here won't work.
>>>>
>>>> We need to hold
the lock until we are sure that
the operation which
>>>> updates the
page tables is completed.
>>>>
>>> The reason for this
change is because hmm mirror has
>>> invalidate_start
callback, no invalidate_end
callback
>>>
>>> Check
mmu_notifier.c and hmm.c again,
below is entire logic to
>>> update CPU page
tables and callback:
>>>
>>> mn lock
amn->lock is used to protect
interval tree access because
>>> user may
submit/register new userptr
anytime.
>>> This is same for
old and new way.
>>>
>>> step 2 guarantee
the GPU operation is done before
updating CPU page
>>> table.
>>>
>>> So I think the
change is safe. We don't need
hold mn lock until the
>>> CPU page tables
update is completed.
>> No, that isn't even
remotely correct. The lock
doesn't protects the
>> interval tree.
>>
>>> Old:
>>> 1.
down_read_non_owner(&amn->lock)
>>> 2. loop to
handle BOs from node->bos
through interval tree
>>> amn->object
nodes
>>> gfx: wait
for pending BOs fence operation
done, mark user
>>> pages dirty
>>> kfd: evict
user queues of the process, wait
for queue
>>> unmap/map operation
done
>>> 3. update CPU
page tables
>>> 4.
up_read(&amn->lock)
>>>
>>> New, switch step 3
and 4
>>> 1.
down_read_non_owner(&amn->lock)
>>> 2. loop to
handle BOs from node->bos
through interval tree
>>> amn->object
nodes
>>> gfx: wait
for pending BOs fence operation
done, mark user
>>> pages dirty
>>> kfd: evict
user queues of the process, wait
for queue
>>> unmap/map operation
done
>>> 3.
up_read(&amn->lock)
>>> 4. update CPU
page tables
>> The lock is there to
make sure that we serialize page
table updates
>> with command
submission.
> As I understand it, the
idea is to prevent command
submission (adding
> new fences to BOs) while a
page table invalidation is in
progress.
Yes, exactly.
> But do we really need
another lock for this? Wouldn't
the
> re-validation of userptr
BOs (currently calling
get_user_pages) force
> synchronization with the
ongoing page table invalidation
through the
> mmap_sem or other MM locks?
No and yes. We don't hold any
other locks while doing command
submission, but I expect that
HMM has its own mechanism to
prevent that.
Since we don't modify
amdgpu_mn_lock()/amdgpu_mn_unlock()
we are certainly not using this
mechanism correctly.
Regards,
Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
|
