Re: [PATCH v2] z3fold: add shrinker

[Vitaly Wool]

On Wed, 12 Oct 2016 09:52:06 +1100
Dave Chinner <david@xxxxxxxxxxxxx> wrote:

<snip>
> 
> > +static unsigned long z3fold_shrink_scan(struct shrinker *shrink,
> > +				struct shrink_control *sc)
> > +{
> > +	struct z3fold_pool *pool = container_of(shrink, struct z3fold_pool,
> > +						shrinker);
> > +	struct z3fold_header *zhdr;
> > +	int i, nr_to_scan = sc->nr_to_scan;
> > +
> > +	spin_lock(&pool->lock);
> 
> Do not do this. Shrinkers should not run entirely under a spin lock
> like this - it causes scheduling latency problems and when the
> shrinker is run concurrently on different CPUs it will simply burn
> CPU doing no useful work. Especially, in this case, as each call to
> z3fold_compact_page() may be copying a significant amount of data
> around and so there is potentially a /lot/ of work being done on
> each call to the shrinker.
> 
> If you need compaction exclusion for the shrinker invocation, then
> please use a sleeping lock to protect the compaction work.

Well, as far as I recall, spin_lock() will resolve to a sleeping lock
for PREEMPT_RT, so it is not that much of a problem for configurations
which do care much about latencies. Please also note that the time
spent in the loop is deterministic since we take not more than one entry
from every unbuddied list.

What I could do though is add the following piece of code at the end of
the loop, right after the /break/:
		spin_unlock(&pool->lock);
		cond_resched();
		spin_lock(&pool->lock);

Would that make sense for you?

> 
> >  *****************/
> > @@ -234,6 +335,13 @@ static struct z3fold_pool *z3fold_create_pool(gfp_t gfp,
> >  		INIT_LIST_HEAD(&pool->unbuddied[i]);
> >  	INIT_LIST_HEAD(&pool->buddied);
> >  	INIT_LIST_HEAD(&pool->lru);
> > +	pool->shrinker.count_objects = z3fold_shrink_count;
> > +	pool->shrinker.scan_objects = z3fold_shrink_scan;
> > +	pool->shrinker.seeks = DEFAULT_SEEKS;
> > +	if (register_shrinker(&pool->shrinker)) {
> > +		pr_warn("z3fold: could not register shrinker\n");
> > +		pool->no_shrinker = true;
> > +	} 
> 
> Just fail creation of the pool. If you can't register a shrinker,
> then much bigger problems are about to happen to your system, and
> running a new memory consumer that /can't be shrunk/ is not going to
> help anyone.

I don't have a strong opinion on this but it doesn't look fatal to me
in _this_ particular case (z3fold) since even without the shrinker, the
compression ratio will never be lower than the one of zbud, which
doesn't have a shrinker at all.

Best regards,
   Vitaly