On 16/05/2025 15:03, Christian König wrote:
On 5/16/25 15:41, Tvrtko Ursulin wrote:
But because TTM shrinker does not currently update shrinkerctl->nr_scanned,
shrinker core assumes TTM looked at full SHRINK_BATCH pages with every call, and
adds and decrements that value to the counters it uses to determine when to stop
trying the TTM shrinker and move to the next one.
Yeah that was intentional as well.
What was the reason do you remember?
Uff I think it hat something to do with fairness between the different pool
types.
Therefore the exit condition can trigger too early in relation to the number of
shrinkable pages in TTM pools. Or it can require too many calls to
ttm_pool_shrinker_scan.
Oh, that is indeed problematic.
Depending on the distribution of freeable pages per pools.
No, that should be completely unrelated.
This patch:
1. Fixes the TTM shrinker to correctly report shrinkctl->nr_scanned.
No it doesn't. The nr_scanned for the TTM pool is not related to the number of
pool types scanned.
Not sure what you mean here.
Ttm_pool_shrinker_count() reports in number of pages which shrinker core then
uses to set nr_to_scan. But as ttm_pool_shrinker_scan() does not update
nr_scanned the core shrinker logic is foiled. If that is by design I would like
to understand why.
But as nr_scanned is currently not updated at all, it is obviously not related
to pools, or anything really.
And in my patch it is still not related to the pools. I made it report the
number of _pages_ scanned to align with ttm_pool_shrinker_count() and what the
shrinker core expects.
Exactly that's the point, you are not using the number of pages scanned, but
rather the order of the pool type. And that is clearly incorrect.
Ugh, you are absolutely correct.
I don't know what happened here.. well I know to an extent. I was
working on some other bug and saw some weird shrinker behaviour.
Misunderstood how it worked, "fixed it", saw the trace points showing a
lot less activity which made me think "yeah this is better" and moved
on. Until I remembered this patch and sent it out.
I think there is a little bit that could be improved, but this version
clearly goes into the trash.
Sorry for the trouble and thanks a lot for persisting with explanations!
Regards,
Tvrtko
This way the break condition in shrinker core works correctly.
2. Makes TTM shrinker actually scan as much as the core requested from it
(respecting shrinkerctl->nr_to_scan).
This avoids only scanning say one page when shrinker core asked to scan 128.
This reduces the number of calls into the TTM shrinker from the core to free
same amount of pages.
That should be easy, you just need to adjust the while loop in
ttm_pool_shrinker_scan().
Yes I think this part would be simple on its own.
3. Tunes the batch size away from the default 128 (SHRINK_BATCH) into a value
based on the median size of the TTM pools.
This ensures that on average TTM shrinker tries to actually free _something_
more often than it does now. Otherwise it can happen to try a few pools
(depending LRU order) and give up, while the freeable pages are actually in the
pools not yet looked at. By tuning the batch size it ensures that on average
more pools are looked at.
That doesn't make sense. The number of pools looked at is completely irrelevant
for the shrinker.
Nr_to_scan the shrinker core asks for is default at most 128 pages.
This means TTM shrinker can look at the few small order pools, find nothing,
and exit.
No, that is not what the code does. See here:
do
num_freed += ttm_pool_shrink();
while (!num_freed && atomic_long_read(&allocated_pages));
We scan until we find at least one page to free or the global allocated pages
counter says that we don't have any pages to free.
Looking at empty pools actually doesn't make much sense, so they are just
skipped until we find the one which contains the pages.
Shrinker core may then call into it again to try again. Or maybe it can even
move on the the next shrinker (away from TTM) if the distribution of free pages
between pools is right.
The exact behaviour here will depend on whether TTM shrinker starts respecting
nr_to_scan and reporting nr_scanned so it is a bit premature to discuss in
detail until those two are settled.
Then it is probably a good idea to separate that.
Anyway, by tweaking the batch_size from 128 to 352 we can make the same effect
in fewer iterations.
We should just look at all pools round robin until the number of objects/pages
the shrinker wants to free has been freed.
I thought that was what the patch does.
Although on a second look I think am missing an exit condition for the case
where requested amount cannot be freed.
Now mind you I looked at this two months ago, which is when I first sent this
patch out, but haven't went through this today again, it still makes sense to
me. So I am curious why you think it totally does not. It looks obvious to me
it is not respecting the contract with the core so I would be surprised if I
was missing some obvious gotcha.
It sounds like you are assuming that the pool type structure are the objects to
scan and we abort when we have scanned a certain amount. But that isn't true,
we abort when we can't find any page to free any more.
Maybe I am blind but AFAICT it currently exits as soon as it frees at least one
page:
static unsigned long ttm_pool_shrinker_scan(struct shrinker *shrink,
struct shrink_control *sc)
{
unsigned long num_freed = 0;
do
num_freed += ttm_pool_shrink();
while (!num_freed && atomic_long_read(&allocated_pages));
return num_freed;
}
Or you mean from the point of view of the core shrinker? Again, that one is
complicated since it currently always decrements total_scan by SHRINK_BATCH. And
total_scan is set to at most half of pool pages. So it can scan order0 find nothing
-> total_scan -= 128, order1 find nothing -> total_scan -= 128, etc, and it
soon accumulates a false positive large number of scanned pages and exits from TTM
shrinker if freeable is low, but actually shrinkable pages are in the high order pool
it did not even get to try.
That is not what happens. See the loop tries to free pages from all pools until
it finds at least one.
After one page is freed the pool type is put at the end of the LRU, so with
each new call to ttm_pool_shrink() we will keep going over different pool types.
See this here:
pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list);
list_move_tail(&pt->shrinker_list, &shrinker_list);
Regards,
Christian.
Regards,
Tvrtko
/* Return the allocation order based for a page */
@@ -881,10 +880,12 @@ int ttm_pool_restore_and_alloc(struct ttm_pool *pool,
struct ttm_tt *tt,
*/
void ttm_pool_free(struct ttm_pool *pool, struct ttm_tt *tt)
{
+ unsigned long nr_scanned = 0;
+
ttm_pool_free_range(pool, tt, tt->caching, 0, tt->num_pages);
while (atomic_long_read(&allocated_pages) > page_pool_size)
- ttm_pool_shrink();
+ ttm_pool_shrink(&nr_scanned);
}
EXPORT_SYMBOL(ttm_pool_free);
@@ -1132,17 +1133,21 @@ void ttm_pool_fini(struct ttm_pool *pool)
}
EXPORT_SYMBOL(ttm_pool_fini);
-/* As long as pages are available make sure to release at least one */
static unsigned long ttm_pool_shrinker_scan(struct shrinker *shrink,
struct shrink_control *sc)
{
- unsigned long num_freed = 0;
+ unsigned long to_scan, freed = 0;
- do
- num_freed += ttm_pool_shrink();
- while (!num_freed && atomic_long_read(&allocated_pages));
+ sc->nr_scanned = 0;
+ to_scan = min_t(unsigned long,
+ sc->nr_to_scan, atomic_long_read(&allocated_pages));
+ while (freed < to_scan) {
+ freed += ttm_pool_shrink(&sc->nr_scanned);
+ to_scan = min_t(unsigned long,
+ to_scan, atomic_long_read(&allocated_pages));
+ }
- return num_freed;
+ return sc->nr_scanned ? freed : SHRINK_STOP;
That again doesn't make sense. That we only find pool types which don't have
pages doesn't mean we have scanned them.
As far as I can see the existing code was correct after all.
}
/* Return the number of pages available or SHRINK_EMPTY if we have none */
@@ -1266,7 +1271,10 @@ EXPORT_SYMBOL(ttm_pool_debugfs);
/* Test the shrinker functions and dump the result */
static int ttm_pool_debugfs_shrink_show(struct seq_file *m, void *data)
{
- struct shrink_control sc = { .gfp_mask = GFP_NOFS };
+ struct shrink_control sc = {
+ .gfp_mask = GFP_NOFS,
+ .nr_to_scan = 1,
+ };
fs_reclaim_acquire(GFP_KERNEL);
seq_printf(m, "%lu/%lu\n", ttm_pool_shrinker_count(mm_shrinker, &sc),
@@ -1324,6 +1332,7 @@ int ttm_pool_mgr_init(unsigned long num_pages)
mm_shrinker->count_objects = ttm_pool_shrinker_count;
mm_shrinker->scan_objects = ttm_pool_shrinker_scan;
+ mm_shrinker->batch = (1 << (MAX_PAGE_ORDER / 2)) * NR_PAGE_ORDERS;
Since we install only one global shrinker for all pool types, which might
contain everything from 1 page till 512 pages, this seems to not make sense at
all either.
What exactly are you trying to do here?
Regards,
Christian.
mm_shrinker->seeks = 1;
shrinker_register(mm_shrinker);
