> 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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
