On 04/30/2013 06:40 PM, Mel Gorman wrote: > On Sat, Apr 27, 2013 at 03:19:02AM +0400, Glauber Costa wrote: >> From: Dave Chinner <dchinner@xxxxxxxxxx> >> >> The current shrinker callout API uses an a single shrinker call for >> multiple functions. To determine the function, a special magical >> value is passed in a parameter to change the behaviour. This >> complicates the implementation and return value specification for >> the different behaviours. >> >> Separate the two different behaviours into separate operations, one >> to return a count of freeable objects in the cache, and another to >> scan a certain number of objects in the cache for freeing. In >> defining these new operations, ensure the return values and >> resultant behaviours are clearly defined and documented. >> >> Modify shrink_slab() to use the new API and implement the callouts >> for all the existing shrinkers. >> >> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx> > > Glauber's signed-off appears to be missing. > I didn't sign all patches, just the ones I have changed. Should I sign them all ? > > As unreasonable as it is, it means that this API can no longer can handle > more than "long" objects. While we'd never hit the limit in practice > unless shrinkers are insane or the objects represent something that is > not stored in memory, it still looks odd to allow an API to potentially > say it has a negative number of objects and as as far as I can gather, > it's just so the shrinkers can return -1. > > Why not leave this as unsigned long and return SHRINK_UNCOUNTABLE > count_objects if the number of freeable items cannot be determined and > SHRINK_UNFREEABLE if scan_objects cannot free without risk of deadlock. > Underneath, SHRINK_* would be defined as ULONG_MAX. > I believe you have already saw the reason for that in the following patch. Do you still have a problem with this ? > >> diff --git a/mm/vmscan.c b/mm/vmscan.c >> index f9d2fba..ca3f690 100644 >> --- a/mm/vmscan.c >> +++ b/mm/vmscan.c >> @@ -205,19 +205,19 @@ static inline int do_shrinker_shrink(struct shrinker *shrinker, >> * >> * Returns the number of slab objects which we shrunk. >> */ >> -unsigned long shrink_slab(struct shrink_control *shrink, >> +unsigned long shrink_slab(struct shrink_control *sc, >> unsigned long nr_pages_scanned, >> unsigned long lru_pages) >> { > > In every other part of vmscan.c, sc is a scan_control but here it is a > shrink_control. That's an unfortunate reuse of a name that cause me to > scratch my head later when I looked at the tracepoint modification. > shrinkc? It's a crappy suggestion but if you think of a better name than > sc then a rename would be nice. > I am all in favor of being explicit. How about we rename sc to... shrink_control ? >> >> - max_pass = do_shrinker_shrink(shrinker, shrink, 0); >> + if (shrinker->scan_objects) { >> + max_pass = shrinker->count_objects(shrinker, sc); >> + WARN_ON(max_pass < 0); >> + } else >> + max_pass = do_shrinker_shrink(shrinker, sc, 0); >> if (max_pass <= 0) >> continue; >> > > This had me scratching my head but looking ahead you introduce the new API, > convert everything over and then kick this away. Presumably this is for > bisect safely and so the patch is not a megapatch of hurt. > Yes, it is. > Generally I saw no problems, this was mostly on the nit-picking end of > the spectrum and a count/scan pair of APIs is a lot nicer to look at > than the current interface. > Great! -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx"> email@xxxxxxxxx </a>