RE: [PATCH] lowmemorykiller: prevent multiple instances of lowmemory killer
From: Dolkow, Snild
Date: Wed Apr 24 2013 - 04:33:57 EST
> 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)
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,
289 if (shrink_ret == -1)
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.
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/