From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932646AbcJLKJg (ORCPT ); Wed, 12 Oct 2016 06:09:36 -0400 Received: from mail-pf0-f176.google.com ([209.85.192.176]:33648 "EHLO mail-pf0-f176.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752865AbcJLKJ2 (ORCPT ); Wed, 12 Oct 2016 06:09:28 -0400 Date: Wed, 12 Oct 2016 10:26:34 +0200 From: Vitaly Wool To: Dave Chinner Cc: Linux-MM , linux-kernel@vger.kernel.org, Seth Jennings , Dan Streetman , Andrew Morton Subject: Re: [PATCH v2] z3fold: add shrinker Message-Id: <20161012102634.f32cb17648eff6b2fd452aea@gmail.com> In-Reply-To: <20161011225206.GJ23194@dastard> References: <20161012001827.53ae55723e67d1dee2a2f839@gmail.com> <20161011225206.GJ23194@dastard> X-Mailer: Sylpheed 3.4.1 (GTK+ 2.24.23; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 12 Oct 2016 09:52:06 +1100 Dave Chinner wrote: > > > +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