RE: [PATCH] lowmemorykiller: prevent multiple instances of lowmemory killer

[Dolkow, Snild]

> No, it's not.  This is controlled higher in shrink_slab() by this:
>
>	max_pass = do_shrinker_shrink(shrinker, shrink, 0);
>	if (max_pass <= 0)
>		continue;
>

Yes, but the later calls will still not handle other negative values as failures, and there is a chance that more than one thread will get past that first check. 

286		nr_before = do_shrinker_shrink(shrinker, shrink, 0);
287		shrink_ret = do_shrinker_shrink(shrinker, shrink,
288						batch_size);
289		if (shrink_ret == -1)
290			break;
291		if (shrink_ret < nr_before)
292			ret += nr_before - shrink_ret;

If, for example, nr_before happens to be -2 and shrink_ret happens to be -1000 here, we're going to erroneously increase ret by 998.

> and your patch is implemented incorrectly, i.e. it does not return 
> LMK_BUSY if the spinlock is contended which needlessly recalls the 
> shrinker later.

It's worth noting that the LMK has a fastpath for the nr_to_scan=0 case, like the shrinker.h comment recommends. And nr_to_scan=0 is used to query the cache size, so it seems like a good idea to return successfully whenever we can.

> You have a couple of options:
>
>  - return -1 when the spinlock is contended immediately when
>    !sc->nr_to_scan (although it should really be a cmpxchg since a
>    spinlock isn't needed), or

This comes with the risk of nr_before being -1, and shrink_ret being positive. In that case, we will have sent a kill signal, but we're not increasing ret. Not a catastrophe, AFAICT, but not fantastic either.

>  - protect the for_each_process() loop in lowmem_shrink() with an
>    actual spinlock that will detect any previously killed process
>    since it will have the TIF_MEMDIE bit set.

We expect that killing one process will be enough, so spinning seems like a waste of time. If one process wasn't enough, the LMK will trigger again soon.

//Snild
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/