Re: [PATCH] i915: slab shrinker have to return -1 if it cant shrink any objects

[KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>]

(2011/07/13 16:41), Chris Wilson wrote:
> On Wed, 13 Jul 2011 09:19:24 +0900, KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
>> (2011/07/12 19:06), Chris Wilson wrote:
>>> On Tue, 12 Jul 2011 18:36:50 +0900, KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
>>>> Hi,
>>>>
>>>> sorry for the delay.
>>>>
>>>>> On Wed, 29 Jun 2011 20:53:54 -0700, Keith Packard <keithp@keithp.com> wrote:
>>>>>> On Fri, 24 Jun 2011 17:03:22 +0900, KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
>> 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?
> 
> 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.

__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.

> 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.

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.


>>> 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;
>>
> 
> 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.

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.