[PATCH 1/6] drm/ttm: Add unampping of the entire device address space
Daniel Vetter
daniel at ffwll.ch
Fri Jun 12 06:52:35 UTC 2020
On Thu, Jun 11, 2020 at 11:15:42AM -0400, Andrey Grodzovsky wrote:
>
> On 6/10/20 5:16 PM, Daniel Vetter wrote:
> > On Wed, Jun 10, 2020 at 10:30 PM Thomas Hellström (Intel)
> > <thomas_os at shipmail.org> wrote:
> > >
> > > On 6/10/20 5:30 PM, Daniel Vetter wrote:
> > > > On Wed, Jun 10, 2020 at 04:05:04PM +0200, Christian König wrote:
> > > > > Am 10.06.20 um 15:54 schrieb Andrey Grodzovsky:
> > > > > > On 6/10/20 6:15 AM, Thomas Hellström (Intel) wrote:
> > > > > > > On 6/9/20 7:21 PM, Koenig, Christian wrote:
> > > > > > > > Am 09.06.2020 18:37 schrieb "Grodzovsky, Andrey"
> > > > > > > > <Andrey.Grodzovsky at amd.com>:
> > > > > > > >
> > > > > > > >
> > > > > > > > On 6/5/20 2:40 PM, Christian König wrote:
> > > > > > > > > Am 05.06.20 um 16:29 schrieb Andrey Grodzovsky:
> > > > > > > > >>
> > > > > > > > >> On 5/11/20 2:45 AM, Christian König wrote:
> > > > > > > > >>> Am 09.05.20 um 20:51 schrieb Andrey Grodzovsky:
> > > > > > > > >>>> Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com>
> > > > > > > > >>>> ---
> > > > > > > > >>>> drivers/gpu/drm/ttm/ttm_bo.c | 22 +++++++++++++++++++++-
> > > > > > > > >>>> include/drm/ttm/ttm_bo_driver.h | 2 ++
> > > > > > > > >>>> 2 files changed, 23 insertions(+), 1 deletion(-)
> > > > > > > > >>>>
> > > > > > > > >>>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c
> > > > > > > > >>>> b/drivers/gpu/drm/ttm/ttm_bo.c
> > > > > > > > >>>> index c5b516f..eae61cc 100644
> > > > > > > > >>>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
> > > > > > > > >>>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
> > > > > > > > >>>> @@ -1750,9 +1750,29 @@ void ttm_bo_unmap_virtual(struct
> > > > > > > > >>>> ttm_buffer_object *bo)
> > > > > > > > >>>> ttm_bo_unmap_virtual_locked(bo);
> > > > > > > > >>>> ttm_mem_io_unlock(man);
> > > > > > > > >>>> }
> > > > > > > > >>>> +EXPORT_SYMBOL(ttm_bo_unmap_virtual);
> > > > > > > > >>>> +void ttm_bo_unmap_virtual_address_space(struct
> > > > > > > > ttm_bo_device *bdev)
> > > > > > > > >>>> +{
> > > > > > > > >>>> + struct ttm_mem_type_manager *man;
> > > > > > > > >>>> + int i;
> > > > > > > > >>>> -EXPORT_SYMBOL(ttm_bo_unmap_virtual);
> > > > > > > > >>>
> > > > > > > > >>>> + for (i = 0; i < TTM_NUM_MEM_TYPES; i++) {
> > > > > > > > >>>> + man = &bdev->man[i];
> > > > > > > > >>>> + if (man->has_type && man->use_type)
> > > > > > > > >>>> + ttm_mem_io_lock(man, false);
> > > > > > > > >>>> + }
> > > > > > > > >>>
> > > > > > > > >>> You should drop that it will just result in a deadlock
> > > > > > > > warning for
> > > > > > > > >>> Nouveau and has no effect at all.
> > > > > > > > >>>
> > > > > > > > >>> Apart from that looks good to me,
> > > > > > > > >>> Christian.
> > > > > > > > >>
> > > > > > > > >>
> > > > > > > > >> As I am considering to re-include this in V2 of the
> > > > > > > > patchsets, can
> > > > > > > > >> you clarify please why this will have no effect at all ?
> > > > > > > > >
> > > > > > > > > The locks are exclusive for Nouveau to allocate/free the io
> > > > > > > > address
> > > > > > > > > space.
> > > > > > > > >
> > > > > > > > > Since we don't do this here we don't need the locks.
> > > > > > > > >
> > > > > > > > > Christian.
> > > > > > > >
> > > > > > > >
> > > > > > > > So basically calling unmap_mapping_range doesn't require any extra
> > > > > > > > locking around it and whatever locks are taken within the function
> > > > > > > > should be enough ?
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > I think so, yes.
> > > > > > > >
> > > > > > > > Christian.
> > > > > > > Yes, that's true. However, without the bo reservation, nothing stops
> > > > > > > a PTE from being immediately re-faulted back again. Even while
> > > > > > > unmap_mapping_range() is running.
> > > > > > >
> > > > > > Can you explain more on this - specifically, which function to reserve
> > > > > > the BO, why BO reservation would prevent re-fault of the PTE ?
> > > > > >
> > > > > Thomas is talking about ttm_bo_reserver()/ttm_bo_unreserve(), but we don't
> > > > > need this because we unmap everything because the whole device is gone and
> > > > > not just manipulate a single BO.
> > > > >
> > > > > > > So the device removed flag needs to be advertized before this
> > > > > > > function is run,
> > > > > > >
> > > > > > I indeed intend to call this right after calling drm_dev_unplug from
> > > > > > amdgpu_pci_remove while adding drm_dev_enter/exit in ttm_bo_vm_fault (or
> > > > > > in amdgpu specific wrapper since I don't see how can I access struct
> > > > > > drm_device from ttm_bo_vm_fault) and this in my understanding should
> > > > > > stop a PTE from being re-faulted back as you pointed out - so again I
> > > > > > don't see how bo reservation would prevent it so it looks like I am
> > > > > > missing something...
> > > > > >
> > > > > >
> > > > > > > (perhaps with a memory barrier pair).
> > > > > > >
> > > > > > drm_dev_unplug and drm_dev_enter/exit are RCU synchronized and so I
> > > > > > don't think require any extra memory barriers for visibility of the
> > > > > > removed flag being set
> > > > > >
> > > > > As far as I can see that should be perfectly sufficient.
> > > > Only if you have a drm_dev_enter/exit pair in your fault handler.
> > > > Otherwise you're still open to the races Thomas described. But aside from
> > > > that the drm_dev_unplug stuff has all the barriers and stuff to make sure
> > > > nothing escapes.
> > > >
> > > > Failure to drm_dev_enter could then also trigger the special case where we
> > > > put a dummy page in place.
> > > > -Daniel
> > > Hmm, Yes, indeed advertizing the flag before the call to
> > > unmap_mapping_range isn't enough, since there might be fault handlers
> > > running that haven't picked up the flag when unmap_mapping_range is
> > > launched.
> > Hm ... Now I'm not sure drm_dev_enter/exit is actually good enough. I
> > guess if you use vmf_insert_pfn within the drm_dev_enter/exit critical
> > section, it should be fine. But I think you can also do fault handlers
> > that just return the struct page and then let core handle the pte
> > wrangling, those would indeed race and we can't have that I think.
> >
> > I think we should try and make sure (as much as possible) that this is
> > done all done in helpers and not some open coded stuff in drivers, or
> > we'll just get it all wrong in the details.
>
>
> Can you please clarify this last paragraph ? Where in your opinion should I
> place the drm_dev_enter/exit and the zero page setting to faulting VA's
> PTEs ? I was planning to do it in amdgpu specific .fault handler which in
> turn calls to ttm_bo_vm_fault.
Nah, I think this should be done in ttm_bo_vm_fault. Reinventing this
wheel in every driver is going to be horrible. Rough control flow:
if (!drm_dev_enter()) {
/* insert dummy page pfn, the hw is gone */
return;
}
/* old page fault handling code with vm_insert_pfn and all the
* same locking as before */
drm_dev_exit();
Cheers, Daniel
>
> Andrey
>
>
> >
> > > For the special case of syncing a full address-space
> > > unmap_mapping_range() with fault handlers regardless of the reason for
> > > the full address-space unmap_mapping_range() one could either traverse
> > > the address space (drm_vma_manager) and grab *all* bo reservations
> > > around the unmap_mapping_range(), or grab the i_mmap_lock in read mode
> > > in the fault handler. (It's taken in write mode in unmap_mapping_range).
> > > While the latter may seem like a simple solution, one should probably
> > > consider the overhead both in run-time and scaling ability.
> > drm_dev_enter/exit uses srcu internally, so afaik should scale
> > ridiculously well and be dirt cheap on the read side. It's horrible on
> > the flush side in drm_dev_unplug, but hey no one cares about that :-)
> > -Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the amd-gfx
mailing list