Re: [PATCH 03/10] mm: shrinker: Add new stats for .to_text()

[Kent Overstreet <kent.overstreet@linux.dev>]

On Wed, Aug 28, 2024 at 12:36:08PM GMT, Dave Chinner wrote:
> Doing this inside the tight loop (adding 3 atomics and two calls to
> ktime_get_ns()) is total overkill. Such fine grained accounting
> doesn't given any extra insight into behaviour compared to
> accounting the entire loop once.
> 
> e.g. the actual time the shrinker takes to run is the time the whole
> loop takes to run, not only the individual shrinker->scan_objects
> call.
> 
> The shrinker code already calculates the total objects scanned and
> the total objects freed by the entire loop, so there is no reason to
> be calculating this again using much more expensive atomic
> operations.
> 
> And these are diagnostic stats - the do not need to be perfectly
> correct and so atomics are unnecessary overhead the vast majority of
> the time.  This code will have much lower impact on runtime overhead
> written like this:
> 
> 	start_time = ktime_get_ns();
> 
> 	while (total_scan >= batch_size ||
>                total_scan >= freeable) {
> 	.....
> 	}
> 
> 	shrinker->objects_requested_to_free += scanned;
> 	shrinker->objects_freed += freed;
> 
> 	end_time = ktime_get_ns();
> 	shrinker->ns_run += end_time - start_time;
> 	shrinker->last_scanned = end_time;
> 	if (freed)
> 		shrinker->last_freed = end_time;
> 
> And still give pretty much the exact same debug information without
> any additional runtime overhead....

*nod*

This is all going to be noise compared to the expense of ->scan()
itself, but there's no reason not to, I'll make the change...