Tetsuo Handa wrote:
> Andrew Morton wrote:
> > On Tue, 21 Nov 2017 21:02:37 +0900 Tetsuo Handa <penguin-kernel@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > > There are users not checking for register_shrinker() failure.
> > > Continuing with ignoring failure can lead to later oops at
> > > unregister_shrinker().
> > >
> > > ...
> > >
> > > --- a/include/linux/shrinker.h
> > > +++ b/include/linux/shrinker.h
> > > @@ -75,6 +75,6 @@ struct shrinker {
> > > #define SHRINKER_NUMA_AWARE (1 << 0)
> > > #define SHRINKER_MEMCG_AWARE (1 << 1)
> > >
> > > -extern int register_shrinker(struct shrinker *);
> > > +extern __must_check int register_shrinker(struct shrinker *);
> > > extern void unregister_shrinker(struct shrinker *);
> > > #endif
> >
> > hm, well, OK, it's a small kmalloc(GFP_KERNEL). That won't be
> > failing.
>
> It failed by fault injection and resulted in a report at
> http://lkml.kernel.org/r/001a113f996099503a055e793dd3@xxxxxxxxxx .
Since kzalloc() can become > 32KB allocation if CONFIG_NODES_SHIFT > 12
(which might not be impossible in near future), register_shrinker() can
potentially become a costly allocation which might fail without invoking
the OOM killer. It is a good opportunity to think whether we should allow
register_shrinker() to fail.
>
> >
> > Affected code seems to be fs/xfs, fs/super.c, fs/quota,
> > arch/x86/kvm/mmu, drivers/gpu/drm/ttm, drivers/md and a bunch of
> > staging stuff.
> >
> > I'm not sure this is worth bothering about?
> >
>
> Continuing with failed register_shrinker() is almost always wrong.
> Though I don't know whether mm/zsmalloc.c case can make sense.
>
Thinking from the fact that register_shrinker() had been "void" until Linux 3.11
and we did not take appropriate precautions when changing to "int" in Linux 3.12,
we need to consider making register_shrinker() "void" again.
If we could agree with opening up the use of __GFP_NOFAIL for allocating a few
non-contiguous pages on large systems, we can make register_shrinker() "void"
again. (Draft patch is shown below. I choose array of kmalloc(PAGE_SIZE)
rather than kvmalloc() in order to use __GFP_NOFAIL.)
include/linux/shrinker.h | 4 +++-
mm/vmscan.c | 31 ++++++++++++++++++++++---------
2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h
index 388ff29..362a871 100644
--- a/include/linux/shrinker.h
+++ b/include/linux/shrinker.h
@@ -34,6 +34,8 @@ struct shrink_control {
};
#define SHRINK_STOP (~0UL)
+#define SHRINKER_SLOTS_PER_PAGE (PAGE_SIZE / sizeof(atomic_long_t))
+#define SHRINKER_SLOT_PAGES DIV_ROUND_UP(MAX_NUMNODES, SHRINKER_SLOTS_PER_PAGE)
/*
* A callback you can register to apply pressure to ageable caches.
*
@@ -67,7 +69,7 @@ struct shrinker {
/* These are for internal use */
struct list_head list;
/* objs pending delete, per node */
- atomic_long_t *nr_deferred;
+ atomic_long_t *nr_deferred[SHRINKER_SLOT_PAGES];
};
#define DEFAULT_SEEKS 2 /* A good number if you don't know better. */
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 1c1bc95..da1f633 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -276,14 +276,19 @@ unsigned long lruvec_lru_size(struct lruvec *lruvec, enum lru_list lru, int zone
*/
int register_shrinker(struct shrinker *shrinker)
{
- size_t size = sizeof(*shrinker->nr_deferred);
+ int i;
+ size_t size = sizeof(atomic_long_t);
if (shrinker->flags & SHRINKER_NUMA_AWARE)
size *= nr_node_ids;
- shrinker->nr_deferred = kzalloc(size, GFP_KERNEL);
- if (!shrinker->nr_deferred)
- return -ENOMEM;
+ for (i = 0; i < SHRINKER_SLOT_PAGES; i++) {
+ const size_t s = size >= PAGE_SIZE ? PAGE_SIZE : size;
+
+ size -= s;
+ shrinker->nr_deferred[i] = kzalloc(s,
+ GFP_KERNEL | __GFP_NOFAIL);
+ }
down_write(&shrinker_rwsem);
list_add_tail(&shrinker->list, &shrinker_list);
@@ -297,10 +302,12 @@ int register_shrinker(struct shrinker *shrinker)
*/
void unregister_shrinker(struct shrinker *shrinker)
{
+ int i;
down_write(&shrinker_rwsem);
list_del(&shrinker->list);
up_write(&shrinker_rwsem);
- kfree(shrinker->nr_deferred);
+ for (i = 0; i < SHRINKER_SLOT_PAGES; i++)
+ kfree(shrinker->nr_deferred[i]);
}
EXPORT_SYMBOL(unregister_shrinker);
@@ -321,17 +328,24 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
long batch_size = shrinker->batch ? shrinker->batch
: SHRINK_BATCH;
long scanned = 0, next_deferred;
+ atomic_long_t *nr_deferred;
freeable = shrinker->count_objects(shrinker, shrinkctl);
if (freeable == 0)
return 0;
+ if (SHRINKER_SLOT_PAGES > 1)
+ nr_deferred = &shrinker->nr_deferred
+ [nid / SHRINKER_SLOTS_PER_PAGE]
+ [nid % SHRINKER_SLOTS_PER_PAGE];
+ else
+ nr_deferred = &shrinker->nr_deferred[0][nid];
/*
* copy the current shrinker scan count into a local variable
* and zero it so that other concurrent shrinker invocations
* don't also do this scanning work.
*/
- nr = atomic_long_xchg(&shrinker->nr_deferred[nid], 0);
+ nr = atomic_long_xchg(nr_deferred, 0);
total_scan = nr;
delta = (4 * nr_scanned) / shrinker->seeks;
@@ -417,10 +431,9 @@ static unsigned long do_shrink_slab(struct shrink_control *shrinkctl,
* scan, there is no need to do an update.
*/
if (next_deferred > 0)
- new_nr = atomic_long_add_return(next_deferred,
- &shrinker->nr_deferred[nid]);
+ new_nr = atomic_long_add_return(next_deferred, nr_deferred);
else
- new_nr = atomic_long_read(&shrinker->nr_deferred[nid]);
+ new_nr = atomic_long_read(nr_deferred);
trace_mm_shrink_slab_end(shrinker, nid, freed, nr, new_nr, total_scan);
return freed;
--
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>
