[PATCH v2 2/7] drm/ttm: add TTM_PAGE_FLAG_SHMEM

[Matthew Auld]

On Tue, 14 Sept 2021 at 10:03, Christian König <christian.koenig at amd.com> wrote:
>
> Am 14.09.21 um 10:50 schrieb Matthew Auld:
> > Add new flag to indicate special shmem based tt, which can directly
> > handle swapping itself, and should be visible to some shrinker.
> >
> > As part of this we should skip the ttm_pages_allocated accounting, since
> > such tt objects should already be reachable, and potentially reclaimable
> > by some shrinker, if under memory pressure, and so shouldn't directly
> > count towards the swap "watermark" level.
> >
> > We also need to stop touching the page->mapping and page->index for such
> > objects, like in ttm_tt_add_mapping, since shmem already uses these.
> > Some drivers seems to depend on the tt mapping/index behaviour for their
> > own purposes, so directly using shmem tt likely won't be usable there
> > as-is.
> >
> > Signed-off-by: Matthew Auld <matthew.auld at intel.com>
> > Cc: Thomas Hellström <thomas.hellstrom at linux.intel.com>
> > Cc: Christian König <christian.koenig at amd.com>
> > ---
> >   drivers/gpu/drm/ttm/ttm_bo_vm.c |  4 ++--
> >   drivers/gpu/drm/ttm/ttm_tt.c    | 10 +++++-----
> >   include/drm/ttm/ttm_tt.h        |  1 +
> >   3 files changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> > index f56be5bc0861..e2131c73dcb6 100644
> > --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
> > +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
> > @@ -346,8 +346,8 @@ vm_fault_t ttm_bo_vm_fault_reserved(struct vm_fault *vmf,
> >                       } else if (unlikely(!page)) {
> >                               break;
> >                       }
> > -                     page->index = drm_vma_node_start(&bo->base.vma_node) +
> > -                             page_offset;
> > +                     if (!(bo->ttm->page_flags & TTM_PAGE_FLAG_SHMEM))
> > +                             page->index = drm_vma_node_start(&bo->base.vma_node) + page_offset;
>
> I still have a rather bad feeling about that.
>
> This should either not be necessary any more in general or the shmemfile
> approach doesn't work correctly.
>
> Please send a patch to remove this for everybody instead and we will see
> if that really works or not.
>
> >                       pfn = page_to_pfn(page);
> >               }
> >
> > diff --git a/drivers/gpu/drm/ttm/ttm_tt.c b/drivers/gpu/drm/ttm/ttm_tt.c
> > index dae52433beeb..cc4815c1f505 100644
> > --- a/drivers/gpu/drm/ttm/ttm_tt.c
> > +++ b/drivers/gpu/drm/ttm/ttm_tt.c
> > @@ -293,7 +293,7 @@ static void ttm_tt_add_mapping(struct ttm_device *bdev, struct ttm_tt *ttm)
> >   {
> >       pgoff_t i;
> >
> > -     if (ttm->page_flags & TTM_PAGE_FLAG_SG)
> > +     if (ttm->page_flags & (TTM_PAGE_FLAG_SG | TTM_PAGE_FLAG_SHMEM))
>
> Maybe you should re-use the TTM_PAGE_FLAG_SG for this and/or rename the
> flag to better describe what it does.
>
> Something like TTM_PAGE_FLAG_EXTERNAL or similar? The only other use
> case for TTM_PAGE_FLAG_SG which comes to my mind is controlling if the
> pages array is allocated or not.

This seems slightly tricky. We still want ttm_bo_vm_reserve() to
behave normally when seeing shmem_tt, and yet it still needs to return
SIGBUS or so for FLAG_SG, as per the existing behaviour. Throwing in
bo->type == type_sg seems maybe plausible, but at least amdgpu is
manually setting FLAG_SG for userptr objects, so I presume bo->type !=
type_sg here?

Otherwise maybe just s/SHMEM/EXTERNAL and leave FLAG_SG as-is?

>
> Christian.
>
> >               return;
> >
> >       for (i = 0; i < ttm->num_pages; ++i)
> > @@ -311,7 +311,7 @@ int ttm_tt_populate(struct ttm_device *bdev,
> >       if (ttm_tt_is_populated(ttm))
> >               return 0;
> >
> > -     if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) {
> > +     if (!(ttm->page_flags & (TTM_PAGE_FLAG_SG | TTM_PAGE_FLAG_SHMEM))) {
> >               atomic_long_add(ttm->num_pages, &ttm_pages_allocated);
> >               if (bdev->pool.use_dma32)
> >                       atomic_long_add(ttm->num_pages,
> > @@ -349,7 +349,7 @@ int ttm_tt_populate(struct ttm_device *bdev,
> >       return 0;
> >
> >   error:
> > -     if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) {
> > +     if (!(ttm->page_flags & (TTM_PAGE_FLAG_SG | TTM_PAGE_FLAG_SHMEM))) {
> >               atomic_long_sub(ttm->num_pages, &ttm_pages_allocated);
> >               if (bdev->pool.use_dma32)
> >                       atomic_long_sub(ttm->num_pages,
> > @@ -364,7 +364,7 @@ static void ttm_tt_clear_mapping(struct ttm_tt *ttm)
> >       pgoff_t i;
> >       struct page **page = ttm->pages;
> >
> > -     if (ttm->page_flags & TTM_PAGE_FLAG_SG)
> > +     if (ttm->page_flags & (TTM_PAGE_FLAG_SG | TTM_PAGE_FLAG_SHMEM))
> >               return;
> >
> >       for (i = 0; i < ttm->num_pages; ++i) {
> > @@ -384,7 +384,7 @@ void ttm_tt_unpopulate(struct ttm_device *bdev, struct ttm_tt *ttm)
> >       else
> >               ttm_pool_free(&bdev->pool, ttm);
> >
> > -     if (!(ttm->page_flags & TTM_PAGE_FLAG_SG)) {
> > +     if (!(ttm->page_flags & (TTM_PAGE_FLAG_SG | TTM_PAGE_FLAG_SHMEM))) {
> >               atomic_long_sub(ttm->num_pages, &ttm_pages_allocated);
> >               if (bdev->pool.use_dma32)
> >                       atomic_long_sub(ttm->num_pages,
> > diff --git a/include/drm/ttm/ttm_tt.h b/include/drm/ttm/ttm_tt.h
> > index 89b15d673b22..20d550185065 100644
> > --- a/include/drm/ttm/ttm_tt.h
> > +++ b/include/drm/ttm/ttm_tt.h
> > @@ -42,6 +42,7 @@ struct ttm_operation_ctx;
> >   #define TTM_PAGE_FLAG_ZERO_ALLOC      (1 << 6)
> >   #define TTM_PAGE_FLAG_SG              (1 << 8)
> >   #define TTM_PAGE_FLAG_NO_RETRY            (1 << 9)
> > +#define TTM_PAGE_FLAG_SHMEM        (1 << 10)
> >
> >   #define TTM_PAGE_FLAG_PRIV_POPULATED  (1 << 31)
> >
>