i915: slab shrinker have to return -1 if it can't shrink any objects
Subsystems (2)
PATCH 1/1 i915: slab shrinker have to return -1 if it can't shrink any objects
Now, i915_gem_inactive_shrink() should return -1 instead of 0 if it can't take a lock. Otherwise, vmscan is getting a lot of confusing because vmscan can't distinguish "can't take a lock temporary" and "we've shrank all of i915 objects".
---
I've found this issue by reviewing. I hope i915 developers confirm this. Thx.
drivers/gpu/drm/i915/i915_gem.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/gpu/drm/i915/i915_gem.c +1 -1
@@ -4104,7 +4104,7 @@ i915_gem_inactive_shrink(struct shrinker *shrinker, struct shrink_control *sc)int cnt;if (!mutex_trylock (&dev->struct_mutex)) -return 0 ; +return nr_to_scan ?-1 :0 ;/* "fast-path" to count number of available objects */ if (nr_to_scan ==0 ) {
Signature · 1 line
1.7.3.1
Quoted 4 lines · KOSAKI Motohiro
Now,
i915_gem_inactive_shrink()should return -1 instead of 0 if it can't take a lock. Otherwise, vmscan is getting a lot of confusing because vmscan can't distinguish "can't take a lock temporary" and "we've shrank all of i915 objects".
This doesn't look like the cleanest change possible. I think it would be better if the shrink function could uniformly return an error indication so that we wouldn't need the weird looking conditional return.
Signature · 1 line
[email protected]
_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] i915: slab shrinker have to return -1 if it cant shrink any objects
Quoted 10 lines · Keith Packard
Quoted 4 lines · KOSAKI Motohiro
Now,
i915_gem_inactive_shrink()should return -1 instead of 0 if it can't take a lock. Otherwise, vmscan is getting a lot of confusing because vmscan can't distinguish "can't take a lock temporary" and "we've shrank all of i915 objects".This doesn't look like the cleanest change possible. I think it would be better if the shrink function could uniformly return an error indication so that we wouldn't need the weird looking conditional return.
Unless I am mistaken, and there are more patches in flight, the return code from i915_gem_inactive_shrink() is promoted to unsigned long and then used in the calculation of how may objects to evict...
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH] i915: slab shrinker have to return -1 if it cant shrink any objects
Hi,
sorry for the delay.
Quoted 11 lines · Keith Packard
Quoted 10 lines · KOSAKI Motohiro
Quoted 5 lines
Now,
i915_gem_inactive_shrink()should return -1 instead of 0 if it can't take a lock. Otherwise, vmscan is getting a lot of confusing because vmscan can't distinguish "can't take a lock temporary" and "we've shrank all of i915 objects".This doesn't look like the cleanest change possible. I think it would be better if the shrink function could uniformly return an error indication so that we wouldn't need the weird looking conditional return.
shrink_icache_memory() is good sample code.
It doesn't take a lock if sc->nr_to_scan==0. i915_gem_inactive_shrink() should do it too, ideally.
My patch only take a first-aid.
Plus, if I understand correctly, i915_gem_inactive_shrink() have more fundamental issue. actually, shrinker code shouldn't use mutex. Instead, use spinlock. IOW, Don't call kmalloc(GFP_KERNEL) while taking dev->struct_mutex. Otherwise, vmscan in its call path completely fail to shrink i915 cache and it makes big memory reclaim confusing if i915 have a lot of shrinkable pages.
Quoted 3 lines
Unless I am mistaken, and there are more patches in flight, the return code from
i915_gem_inactive_shrink()is promoted to unsigned long and then used in the calculation of how may objects to evict...
shrinker->shrink has int type value. you can't change i915_gem_inactive_shrink() unless generic shrinker code.
Do you really want to change it?
Re: [PATCH] i915: slab shrinker have to return -1 if it cant shrink any objects
Quoted 16 lines · KOSAKI Motohiro
Hi,
sorry for the delay.
Quoted 11 lines · Keith Packard
Quoted 10 lines · KOSAKI Motohiro
Quoted 5 lines
Now,
i915_gem_inactive_shrink()should return -1 instead of 0 if it can't take a lock. Otherwise, vmscan is getting a lot of confusing because vmscan can't distinguish "can't take a lock temporary" and "we've shrank all of i915 objects".This doesn't look like the cleanest change possible. I think it would be better if the shrink function could uniformly return an error indication so that we wouldn't need the weird looking conditional return.
Quoted 8 lines
shrink_icache_memory()is good sample code.
It doesn't take a lock if sc->nr_to_scan==0.i915_gem_inactive_shrink()should do it too, ideally.My patch only take a first-aid.
Plus, if I understand correctly,
i915_gem_inactive_shrink()have more fundamental issue. actually, shrinker code shouldn't use mutex. Instead, use spinlock.
Why? The shrinker code is run in a non-atomic context that is explicitly allowed to wait, or so I thought. Where's the caveat that prevents mutex? Why doesn't the kernel complain?
Quoted 3 lines
IOW, Don't call
kmalloc(GFP_KERNEL)while takingdev->struct_mutex. Otherwise, vmscan in its call path completely fail to shrink i915 cache and it makes big memory reclaim confusing if i915 have a lot of shrinkable pages.
i915 can have several GiB of shrinkable pages. Of which 2 GiB may be tied up in the GTT upon which we have to wait for the GPU to release. In the future, we will be able to tie up all of physical memory.
There is only a single potential kmalloc in the shrinker path, for which we could preallocate a request so that we always have one available here.
Quoted 7 lines
Quoted 3 lines
Unless I am mistaken, and there are more patches in flight, the return code from
i915_gem_inactive_shrink()is promoted to unsigned long and then used in the calculation of how may objects to evict...
shrinker->shrinkhas int type value. you can't changei915_gem_inactive_shrink()unless generic shrinker code.
Do you really want to change it?
No, just pointing out that the patch causes warnings from the shrinker code as it tries to process (unsigned long)-1 objects. shrink_slab() does not use <0 as an error code!
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH] i915: slab shrinker have to return -1 if it cant shrink any objects
Quoted 21 lines · Chris Wilson
Quoted 20 lines · KOSAKI Motohiro
Hi,
sorry for the delay.
Quoted 11 lines · Keith Packard
Quoted 10 lines · KOSAKI Motohiro
Quoted 5 lines
Now,
i915_gem_inactive_shrink()should return -1 instead of 0 if it can't take a lock. Otherwise, vmscan is getting a lot of confusing because vmscan can't distinguish "can't take a lock temporary" and "we've shrank all of i915 objects".This doesn't look like the cleanest change possible. I think it would be better if the shrink function could uniformly return an error indication so that we wouldn't need the weird looking conditional return.
shrink_icache_memory()is good sample code.
It doesn't take a lock if sc->nr_to_scan==0.i915_gem_inactive_shrink()should do it too, ideally.
Quoted 7 lines
Quoted 4 lines
My patch only take a first-aid.
Plus, if I understand correctly,
i915_gem_inactive_shrink()have more fundamental issue. actually, shrinker code shouldn't use mutex. Instead, use spinlock.Why? The shrinker code is run in a non-atomic context that is explicitly allowed to wait, or so I thought. Where's the caveat that prevents mutex? Why doesn't the kernel complain?
The matter is not in contention. The problem is happen if the mutex is taken by shrink_slab calling thread. i915_gem_inactive_shrink() have no way to shink objects. How do you detect such case?
Quoted 10 lines
Quoted 3 lines
IOW, Don't call
kmalloc(GFP_KERNEL)while takingdev->struct_mutex. Otherwise, vmscan in its call path completely fail to shrink i915 cache and it makes big memory reclaim confusing if i915 have a lot of shrinkable pages.i915 can have several GiB of shrinkable pages. Of which 2 GiB may be tied up in the GTT upon which we have to wait for the GPU to release. In the future, we will be able to tie up all of physical memory.
There is only a single potential kmalloc in the shrinker path, for which we could preallocate a request so that we always have one available here.
Again, waiting is no problem if it is enough little time. btw, I think preallocation must be implemented, otherwise shrinker have no guarantee to shrink.
thanks.
Quoted 11 lines
Quoted 7 lines
Quoted 3 lines
Unless I am mistaken, and there are more patches in flight, the return code from
i915_gem_inactive_shrink()is promoted to unsigned long and then used in the calculation of how may objects to evict...
shrinker->shrinkhas int type value. you can't changei915_gem_inactive_shrink()unless generic shrinker code.
Do you really want to change it?No, just pointing out that the patch causes warnings from the shrinker code as it tries to process (unsigned long)-1 objects.
shrink_slab()does not use <0 as an error code!
Look.
unsigned long shrink_slab(struct shrink_control *shrink,
unsigned long nr_pages_scanned, unsigned long lru_pages)
{
(snip)
while (total_scan >= SHRINK_BATCH) {
long this_scan = SHRINK_BATCH;
int shrink_ret;
int nr_before;
nr_before = do_shrinker_shrink(shrinker, shrink, 0);
shrink_ret = do_shrinker_shrink(shrinker, shrink,
this_scan);
if (shrink_ret == -1)
break;Re: [PATCH] i915: slab shrinker have to return -1 if it cant shrink any objects
Quoted 11 lines · KOSAKI Motohiro
Quoted 7 lines · Chris Wilson
Quoted 6 lines · KOSAKI Motohiro
Hi,
sorry for the delay.
Quoted 2 lines · Keith Packard
Quoted 1 line · KOSAKI Motohiro
The matter is not in contention. The problem is happen if the mutex is taken by
shrink_slabcalling thread.i915_gem_inactive_shrink()have no way to shink objects. How do you detect such case?
In the primary allocator for the backing pages whilst the mutex is held we do __NORETRY and a manual shrinkage of our buffers before failing. That's the largest allocator, all the others are tiny and short-lived by comparison and left to fail.
For a second process to hit shrink_slab whilst the driver is blocked on the GPU, that is... unfortunate. Dropping that lock across that wait is achievable, just very complicated.
Quoted 11 lines
Quoted 3 lines
No, just pointing out that the patch causes warnings from the shrinker code as it tries to process (unsigned long)-1 objects.
shrink_slab()does not use <0 as an error code!Look.
unsigned long shrink_slab(struct
shrink_control*shrink,
unsigned longnr_pages_scanned, unsigned longlru_pages)
{
(snip)
Quoted 11 lines
while (total_scan >= SHRINK_BATCH) {
long this_scan = SHRINK_BATCH;
int shrink_ret;
int nr_before;
nr_before = do_shrinker_shrink(shrinker, shrink, 0);
shrink_ret = do_shrinker_shrink(shrinker, shrink,
this_scan);
if (shrink_ret == -1)
break;And fifteen lines above that you have:
unsigned long max_pass = do_shrinker_shrink(shrinker, shrinker, 0);
...
shrinker->nr += f(max_pass);
if (shrinker->nr < 0) printk(KERN_ERR "...");
That's the *error* I hit when I originally returned -1. -Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH] i915: slab shrinker have to return -1 if it cant shrink any objects
Quoted 9 lines · Chris Wilson
Quoted 8 lines · KOSAKI Motohiro
Quoted 7 lines · Chris Wilson
Quoted 6 lines · KOSAKI Motohiro
Hi,
sorry for the delay.
Quoted 2 lines · Keith Packard
Quoted 1 line · KOSAKI Motohiro
Quoted 8 lines
Quoted 3 lines
The matter is not in contention. The problem is happen if the mutex is taken by
shrink_slabcalling thread.i915_gem_inactive_shrink()have no way to shink objects. How do you detect such case?In the primary allocator for the backing pages whilst the mutex is held we do
__NORETRYand a manual shrinkage of our buffers before failing. That's the largest allocator, all the others are tiny and short-lived by comparison and left to fail.
__NORETRY perhaps might help to avoid false positive oom. But, __NORETRY still makes full page reclaim and may drop a lot of innocent page cache, and then system may become slow down.
Of course, you don't meet such worst case scenario so easy. But you may need to think worst case if you touch memory management code.
Quoted 3 lines
For a second process to hit
shrink_slabwhilst the driver is blocked on the GPU, that is... unfortunate. Dropping that lock across that wait is achievable, just very complicated.
I think that's no problem. waiting and complicated slow path have no matter if it's only exceptional case. That don't makes false positive memory starvation.
thx.
Quoted 23 lines
Quoted 11 lines
Quoted 3 lines
No, just pointing out that the patch causes warnings from the shrinker code as it tries to process (unsigned long)-1 objects.
shrink_slab()does not use <0 as an error code!Look.
unsigned long shrink_slab(struct
shrink_control*shrink,
unsigned longnr_pages_scanned, unsigned longlru_pages)
{
(snip)Quoted 11 lines
while (total_scan >= SHRINK_BATCH) { long this_scan = SHRINK_BATCH; int shrink_ret; int nr_before; nr_before = do_shrinker_shrink(shrinker, shrink, 0); shrink_ret = do_shrinker_shrink(shrinker, shrink, this_scan); if (shrink_ret == -1) break;
Quoted 7 lines
And fifteen lines above that you have:
unsigned longmax_pass=do_shrinker_shrink(shrinker, shrinker, 0);
...
shrinker->nr+=f(max_pass);
if (shrinker->nr< 0)printk(KERN_ERR "...");That's the *error* I hit when I originally returned -1.
You misunderstand the code. The third argument is critically important. Only if it's 0 (ie sc->nr_to_scan==0), shrinker must not return negative. Thus, my patch checked nr_to_scan argument. and I've suggested look at shrink_icache_memory().
If you are thinking the shrinker protocol is too complicated, doc update patch is really welcome.
Re: [PATCH] i915: slab shrinker have to return -1 if it cant shrink any objects
Quoted 14 lines · KOSAKI Motohiro
Quoted 13 lines · Chris Wilson
Quoted 11 lines · KOSAKI Motohiro
Quoted 7 lines · Chris Wilson
Quoted 6 lines · KOSAKI Motohiro
Hi,
sorry for the delay.
Quoted 2 lines · Keith Packard
Quoted 1 line · KOSAKI Motohiro
The matter is not in contention. The problem is happen if the mutex is taken by
shrink_slabcalling thread.i915_gem_inactive_shrink()have no way to shink objects. How do you detect such case?
Quoted 8 lines
Quoted 4 lines
In the primary allocator for the backing pages whilst the mutex is held we do
__NORETRYand a manual shrinkage of our buffers before failing. That's the largest allocator, all the others are tiny and short-lived by comparison and left to fail.
__NORETRYperhaps might help to avoid false positive oom. But,__NORETRYstill makes full page reclaim and may drop a lot of innocent page cache, and then system may become slow down.
But in this context, that is memory the user has requested to be used with the GPU, so the page cache is sacrificed to meet the allocation, if possible.
Quoted 2 lines
Of course, you don't meet such worst case scenario so easy. But you may need to think worst case if you touch memory management code.
Actually we'd much rather you took us into account when designing the mm.
Quoted 6 lines
Quoted 1 line
That's the *error* I hit when I originally returned -1.
You misunderstand the code. The third argument is critically important. Only if it's 0 (ie sc->nr_to_scan==0), shrinker must not return negative. Thus, my patch checked
nr_to_scanargument. and I've suggested look atshrink_icache_memory().
Ok.
Quoted 2 lines
If you are thinking the shrinker protocol is too complicated, doc update patch is really welcome.
What I don't understand is the disconnect between objects to shrink and the number of pages released. We may have tens of thousands of single page objects that are expensive to free in comparison to a few 10-100MiB objects that are just sitting idle. Would it be better to report the estimated number of shrinkable pages instead?
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Re: [PATCH] i915: slab shrinker have to return -1 if it cant shrink any objects
Quoted 24 lines · Chris Wilson
Quoted 14 lines · KOSAKI Motohiro
Quoted 13 lines · Chris Wilson
Quoted 11 lines · KOSAKI Motohiro
Quoted 7 lines · Chris Wilson
Quoted 6 lines · KOSAKI Motohiro
Hi,
sorry for the delay.
Quoted 2 lines · Keith Packard
Quoted 1 line · KOSAKI Motohiro
The matter is not in contention. The problem is happen if the mutex is taken by
shrink_slabcalling thread.i915_gem_inactive_shrink()have no way to shink objects. How do you detect such case?Quoted 8 lines
Quoted 4 lines
In the primary allocator for the backing pages whilst the mutex is held we do
__NORETRYand a manual shrinkage of our buffers before failing. That's the largest allocator, all the others are tiny and short-lived by comparison and left to fail.
__NORETRYperhaps might help to avoid false positive oom. But,__NORETRYstill makes full page reclaim and may drop a lot of innocent page cache, and then system may become slow down.
Quoted 8 lines
But in this context, that is memory the user has requested to be used with the GPU, so the page cache is sacrificed to meet the allocation, if possible.
Quoted 2 lines
Of course, you don't meet such worst case scenario so easy. But you may need to think worst case if you touch memory management code.
Actually we'd much rather you took us into account when designing the mm.
Heh. Now where have I heard that before? :/
Quoted 8 lines
Quoted 2 lines
If you are thinking the shrinker protocol is too complicated, doc update patch is really welcome.
What I don't understand is the disconnect between objects to shrink and the number of pages released. We may have tens of thousands of single page objects that are expensive to free in comparison to a few 10-100MiB objects that are just sitting idle. Would it be better to report the estimated number of shrinkable pages instead?
Maybe. Then again, maybe not.
The shrinker API is designed for slab caches which have a fixed object size, not a variable amount of memory per object, so if you report the number of shrinkable pages, you can make whatever decision you want as to the object(s) you free to free up that many pages.
However, that means that if you have 1000 reclaimable pages, and the dentry cache has 1000 reclaimable dentries, the same shrinker calls will free 1000 pages from your cache, but maybe none from the dentry cache due to slab fragmentation. Hence your cache could end up being blown to pieces by light memory pressure by telling the shrinker how many shrinkable pages you have cached. In that case, you want to report a much smaller number so the cache is harder to reclaim under light memory pressure, or don't reclaim as much as the shrinker is asked to reclaim.
This is one of the issues I faced when converting the XFS buffer cache to use an internal LRU and a shrinker to reclaim buffers that hold one or more pages. We used to cache the metadata in the page cache and let the VM reclaim from there, but that was a crap-shoot because page reclaim kept trashing the working set of metadata pages and it was simply not fixable. Hence I changed the lifecycle of buffers to include a priority based LRU for reclaiming buffer objects and moved away from using the page cache from holding cached metadata.
I let the shrinker know how many reclaimable buffers there are, but it has no idea how much memory each buffer pіns. I don't even keep track of it because from a performance perspective it is irrelevant; what matters is maintaining a minimial working set of metadata buffers under memory pressure.
In most cases the buffers hold only one or two pages, but because of the reclaim reference count it can take up to 7 attempts to free a buffer before it is finally reclaimed. Hence the buffer cache tends to hold onto a critical working set quite well under different levels of memory pressure because buffers more likely to be reused are much harder to reclaim than those that are likely to be used ony once.
As a result, the LRU resists aggressive reclaim to maintain the necessary working set of buffers quite well. The working set gets smaller as memory pressure goes up, but the shrinker is not able to completely trash the cache like the previous page-cache based version did. It's a very specific solution to the problem of tuning a shrinker for good system behaviour, but it's the only way I found that works....
Oh, and using a mutex to single thread cache reclaim rather than spinlocks is usually a good idea from a scalability point of view because your shrinker can be called simultaneously on every CPU. Spinlocks really, really hurt when that happens, and performance will plummet when it happens because you burn CPU on locks rather than reclaimıng objects. Single threaded object reclaim is still the fastest way to do reclaim if you have global lists and locks.
What I'm trying to say is that how you solve the shrinker balance problem for you subsystem will be specific to how you need to hold pages under memory pressure to maintain performance. Sorry I can't give you a better answer than that, but that's what my experience with caches and tuning shrinker behaviour indicates.
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH] i915: slab shrinker have to return -1 if it cant shrink any objects
In reply to KOSAKI MotohiroQuoted 20 lines · KOSAKI Motohiro
Quoted 19 lines · Chris Wilson
Quoted 12 lines
(snip)
while (total_scan >= SHRINK_BATCH) { long this_scan = SHRINK_BATCH; int shrink_ret; int nr_before; nr_before = do_shrinker_shrink(shrinker, shrink, 0); shrink_ret = do_shrinker_shrink(shrinker, shrink, this_scan); if (shrink_ret == -1) break;And fifteen lines above that you have:
unsigned longmax_pass=do_shrinker_shrink(shrinker, shrinker, 0);
...
shrinker->nr+=f(max_pass);
if (shrinker->nr< 0)printk(KERN_ERR "...");
Quoted 4 lines
Quoted 1 line
That's the *error* I hit when I originally returned -1.
You misunderstand the code. The third argument is critically important. Only if it's 0 (ie sc->nr_to_scan==0), shrinker must not return negative.
And once again the shitty shrinker API bites a user.
Quoted 2 lines
Thus, my patch checked
nr_to_scanargument. and I've suggested look atshrink_icache_memory().
Which is going away real soon - it's not the model of perfection that you make it out to be. ;)
Quoted 2 lines
If you are thinking the shrinker protocol is too complicated, doc update patch is really welcome.
Slab shrinkers have a nasty, crappy interface and the shrink_slab() code is full of bugs. Rather that telling people to "update the documentation" because it's too complex, how about we fix the interface and the bugs?
Indeed, how hard is it to require a subsystem to supply two shrinker methods, one to return the count of reclaimable objects, the other to scan the reclaimable objects to reclaim them? After all, that's exactly the interface I'm exposing to filesystems underneath the shrinker API in the per-sb shrinker patchset that gets rid of shrink_icache_memory() rather than propagating the insanity....
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Re: [PATCH] i915: slab shrinker have to return -1 if it cant shrink any objects
Quoted 7 lines
Quoted 2 lines
If you are thinking the shrinker protocol is too complicated, doc update patch is really welcome.
Slab shrinkers have a nasty, crappy interface and the
shrink_slab()code is full of bugs. Rather that telling people to "update the documentation" because it's too complex, how about we fix the interface and the bugs?
If you can take your time for that, I don't hesitate to help you. ;)
Re: [PATCH] i915: slab shrinker have to return -1 if it cant shrink any objects
Quoted 9 lines · KOSAKI Motohiro
Quoted 7 lines
Quoted 2 lines
If you are thinking the shrinker protocol is too complicated, doc update patch is really welcome.
Slab shrinkers have a nasty, crappy interface and the
shrink_slab()code is full of bugs. Rather that telling people to "update the documentation" because it's too complex, how about we fix the interface and the bugs?If you can take your time for that, I don't hesitate to help you. ;)
Its on my list of things to do if nobody else steps before I get there ;)
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]