Am 16.04.24 um 15:08 schrieb Thomas Hellström:
Hi, Christian,
On Tue, 2024-04-16 at 13:55 +0200, Christian König wrote:
While patches 1-4 look good from a high level I still think it needs
some prerequisite and re-ordering.
First of all make all the cleanups separate patches. In other words
that
ttm_resource_manager_next() takes only the cursor as argument, adding
ttm_resource_cursor_fini()/ttm_resource_cursor_fini_locked() as one
patch and then ttm_lru_bulk_move_init()/ttm_lru_bulk_move_fini() as
second.
Yes, I can take a look at that. I think the shortening of the argument
list of ttm_resource_manager_next() makes sense as a separate cleanup.
The other two are needed because of the changes introduced in the
respective patches. I could of course add stubs of these functions
before the patch that currently introduce them if needed, but don't
really see the point. What do you think.
Na, stubs doesn't make sense.
I was under the expression that you would have something to do for them
even without the LRU patch. If that's not the case then just skip that.
With that done I think we should first switch over TTM and all
drivers
using it to drm_exec as part of it's context object.
So are you ok with adding an optional drm_exec pointer in the
ttm_operation_ctx for this? (That was my plan moving forward).
Yeah, perfectly valid. My thinking was just to do that first.
However, when that has been added, I think it makes sense to leave to
the driver author to port their validation loops and bo allocation over
to using drm_exec. While we made sure the drm_exec object was indeed
passed to the validation helper in the drm_gpuvm code, I'm not sure
everybody actually includes their validation and bo allocation (for
example page-table-bos) in their drm_exec while_not_all_locked() loop,
and I think it's reasonable to require the "port the driver over" to be
an optional but strongly recommended driver effort. If the driver sets
ctx->drm_exec to NULL, it will fallback to current behaviour.
For the intermediate case I think it makes sense to have this optional,
but in the long run we should make it mandatory.
Then I would switch over to using LRU hitches for both swapping and
eviction.
And when that's finally done we can take a look into the partial
shmem
swapping :)
And Felix is really (and mean *really*) looking forward to the
partial
shmem swapping as well.
While the LRU walker helper introduced in patch 8 has drm_exec support,
shrinkers don't require it, since they are always trylocking. (However
being able to "evict" system to swap directly in the validation stage
using drm_exec locking is probably something we should support).
That's why I opted for implementing shrinking before exhaustive
eviction. But if you insist we can do it the other way around. Most of
what's needed is already in the patches.
I don't care about the order in which things are implemented, but I
think we should have at least some eviction prototype as well for testing.
Eviction is just the much easier to exercise use case.
Regards,
Christian.
/Thomas
Regards,
Christian.
Am 16.04.24 um 12:07 schrieb Thomas Hellström:
This series implements TTM shrinker / eviction helpers and an xe bo
shrinker. It builds on two previous series, *and obsoletes these*.
First
https://www.mail-archive.com/dri-devel@xxxxxxxxxxxxxxxxxxxxx/msg484425.html
for patch 1-4, which IMO still could be reviewed and pushed as a
separate series.
Second the previous TTM shrinker series
https://lore.kernel.org/linux-mm/b7491378-defd-4f1c-31e2-29e4c77e2d67@xxxxxxx/T/
Where the comment about layering
https://lore.kernel.org/linux-mm/b7491378-defd-4f1c-31e2-29e4c77e2d67@xxxxxxx/T/#ma918844aa8a6efe8768fdcda0c6590d5c93850c9
now addressed, and this version also implements shmem objects for
backup
rather than direct swap-cache insertions, which was used in the
previuos
series. It turns out that with per-page backup / shrinking, shmem
objects
appears to work just as well as direct swap-cache insertions with
the
added benefit that was introduced in the previous TTM shrinker
series to
avoid running out of swap entries isn't really needed.
In any case, patch 1-4 are better described in their separate
series.
(RFC is removed for those).
Patch 5 could in theory be skipped but introduces a possibility to
easily
add or test multiple backup backends, like the direct swap-cache
insertion or even files into fast dedicated nvme storage for for
example.
Patch 6 introduces helpers in the ttm_pool code for page-by-page
shrinking
and recovery. It avoids having to temporarily allocate a huge
amount of
memory to be able to shrink a buffer object. It also introduces the
possibility to immediately write-back pages if needed, since that
tends
to be a bit delayed when left to kswapd.
Patch 7 Adds a simple error injection to the above code to help
increase
test coverage.
Patch 8 introduces a LRU walk helper for eviction and shrinking.
It's
currently xe-only but not xe-specific and can easily be moved to
TTM when
used by more than one driver or when eviction is implemented using
it.
Patch 9 introduces a helper callback for shrinking (Also ready to
be
moved to TTM) and an xe-specific shrinker implementation. It also
adds a kunit test to test the shrinker functionality by trying to
allocate twice the available amount of RAM as buffer objects. If
there
is no swap-space available, the buffer objects are marked
purgeable.
v2:
- Squash obsolete revision history in the patch commit messages.
- Fix a couple of review comments by Christian
- Don't store the mem_type in the TTM managers but in the
resource cursor.
- Rename introduced TTM *back_up* function names to *backup*
- Add ttm pool recovery fault injection.
- Shrinker xe kunit test
- Various bugfixes
Cc: Somalapuram Amaranath <Amaranath.Somalapuram@xxxxxxx>
Cc: Christian König <christian.koenig@xxxxxxx>
Cc: <dri-devel@xxxxxxxxxxxxxxxxxxxxx>
Thomas Hellström (8):
drm/ttm: Allow TTM LRU list nodes of different types
drm/ttm: Use LRU hitches
drm/ttm, drm/amdgpu, drm/xe: Consider hitch moves within bulk
sublist
moves
drm/ttm: Allow continued swapout after -ENOSPC falure
drm/ttm: Add a virtual base class for graphics memory backup
drm/ttm/pool: Provide a helper to shrink pages.
drm/xe, drm/ttm: Provide a generic LRU walker helper
drm/xe: Add a shrinker for xe bos
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 +
drivers/gpu/drm/ttm/Makefile | 2 +-
drivers/gpu/drm/ttm/ttm_backup_shmem.c | 137 +++++++++
drivers/gpu/drm/ttm/ttm_bo.c | 1 +
drivers/gpu/drm/ttm/ttm_device.c | 33 ++-
drivers/gpu/drm/ttm/ttm_pool.c | 391
++++++++++++++++++++++++-
drivers/gpu/drm/ttm/ttm_resource.c | 231 ++++++++++++---
drivers/gpu/drm/ttm/ttm_tt.c | 34 +++
drivers/gpu/drm/xe/Makefile | 2 +
drivers/gpu/drm/xe/xe_bo.c | 123 ++++++--
drivers/gpu/drm/xe/xe_bo.h | 3 +
drivers/gpu/drm/xe/xe_device.c | 8 +
drivers/gpu/drm/xe/xe_device_types.h | 2 +
drivers/gpu/drm/xe/xe_shrinker.c | 237 +++++++++++++++
drivers/gpu/drm/xe/xe_shrinker.h | 18 ++
drivers/gpu/drm/xe/xe_ttm_helpers.c | 224 ++++++++++++++
drivers/gpu/drm/xe/xe_ttm_helpers.h | 63 ++++
drivers/gpu/drm/xe/xe_vm.c | 4 +
include/drm/ttm/ttm_backup.h | 136 +++++++++
include/drm/ttm/ttm_device.h | 2 +
include/drm/ttm/ttm_pool.h | 4 +
include/drm/ttm/ttm_resource.h | 96 +++++-
include/drm/ttm/ttm_tt.h | 19 ++
23 files changed, 1683 insertions(+), 91 deletions(-)
create mode 100644 drivers/gpu/drm/ttm/ttm_backup_shmem.c
create mode 100644 drivers/gpu/drm/xe/xe_shrinker.c
create mode 100644 drivers/gpu/drm/xe/xe_shrinker.h
create mode 100644 drivers/gpu/drm/xe/xe_ttm_helpers.c
create mode 100644 drivers/gpu/drm/xe/xe_ttm_helpers.h
create mode 100644 include/drm/ttm/ttm_backup.h
