From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f51.google.com (mail-pa0-f51.google.com [209.85.220.51]) by kanga.kvack.org (Postfix) with ESMTP id F25436B0253 for ; Fri, 10 Jul 2015 22:53:45 -0400 (EDT) Received: by pachj5 with SMTP id hj5so1693925pac.3 for ; Fri, 10 Jul 2015 19:53:45 -0700 (PDT) Received: from mail-pd0-x22c.google.com (mail-pd0-x22c.google.com. [2607:f8b0:400e:c02::22c]) by mx.google.com with ESMTPS id dk4si16403593pbb.219.2015.07.10.19.53.43 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 10 Jul 2015 19:53:44 -0700 (PDT) Received: by pdrg1 with SMTP id g1so61444387pdr.2 for ; Fri, 10 Jul 2015 19:53:43 -0700 (PDT) From: Sergey Senozhatsky Subject: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Date: Sat, 11 Jul 2015 11:51:53 +0900 Message-Id: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> Sender: owner-linux-mm@kvack.org List-ID: To: Andrew Morton Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Sergey Senozhatsky , Sergey Senozhatsky Hello, Shrinker API does not handle nicely unregister_shrinker() on a not-registered ->shrinker. Looking at shrinker users, they all have to (a) carry on some sort of a flag to make sure that "unregister_shrinker()" will not blow up later (b) be fishy (potentially can Oops) (c) access private members `struct shrinker' (e.g. `shrink.list.next') Change unregister_shrinker() to consider all-zeroes shrinker as 'initialized, but not registered' shrinker, so we can avoid NULL dereference when unregister_shrinker() accidentally receives such a struct. Introduce init_shrinker() function to init `critical' shrinkers members when the entire shrinker cannot be, for some reason, zeroed out. This also helps to avoid Oops in unregister_shrinker() in some cases (when unregister_shrinker() receives not initialized and not registered shrinker). Sergey Senozhatsky (2): mm/shrinker: do not NULL dereference uninitialized shrinker mm/shrinker: add init_shrinker() function include/linux/shrinker.h | 1 + mm/vmscan.c | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) -- 2.4.5 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pd0-f177.google.com (mail-pd0-f177.google.com [209.85.192.177]) by kanga.kvack.org (Postfix) with ESMTP id 6ED486B0253 for ; Fri, 10 Jul 2015 22:53:49 -0400 (EDT) Received: by pdrg1 with SMTP id g1so61445321pdr.2 for ; Fri, 10 Jul 2015 19:53:49 -0700 (PDT) Received: from mail-pa0-x22e.google.com (mail-pa0-x22e.google.com. [2607:f8b0:400e:c03::22e]) by mx.google.com with ESMTPS id sz10si17158966pab.68.2015.07.10.19.53.47 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 10 Jul 2015 19:53:47 -0700 (PDT) Received: by pachj5 with SMTP id hj5so1694189pac.3 for ; Fri, 10 Jul 2015 19:53:47 -0700 (PDT) From: Sergey Senozhatsky Subject: [PATCH 1/2] mm/shrinker: do not NULL dereference uninitialized shrinker Date: Sat, 11 Jul 2015 11:51:54 +0900 Message-Id: <1436583115-6323-2-git-send-email-sergey.senozhatsky@gmail.com> In-Reply-To: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> Sender: owner-linux-mm@kvack.org List-ID: To: Andrew Morton Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Sergey Senozhatsky , Sergey Senozhatsky Consider 'all zeroes' shrinker as 'initialized, but not registered', and, thus, don't unregister such a shrinker. This helps to avoid accidental NULL pointer dereferences, when a zeroed shrinker struct is getting passed to unregister_shrinker() in error handing path, for example. Signed-off-by: Sergey Senozhatsky --- mm/vmscan.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/mm/vmscan.c b/mm/vmscan.c index c8d8282..cadc8a2 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -254,6 +254,12 @@ EXPORT_SYMBOL(register_shrinker); */ void unregister_shrinker(struct shrinker *shrinker) { + /* + * All-zeroes is 'initialized, but not registered' shrinker. + */ + if (unlikely(!shrinker->list.next)) + return; + down_write(&shrinker_rwsem); list_del(&shrinker->list); up_write(&shrinker_rwsem); -- 2.4.5 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pd0-f181.google.com (mail-pd0-f181.google.com [209.85.192.181]) by kanga.kvack.org (Postfix) with ESMTP id 7E00D9003C8 for ; Fri, 10 Jul 2015 22:53:52 -0400 (EDT) Received: by pdjr16 with SMTP id r16so32076626pdj.3 for ; Fri, 10 Jul 2015 19:53:52 -0700 (PDT) Received: from mail-pd0-x229.google.com (mail-pd0-x229.google.com. [2607:f8b0:400e:c02::229]) by mx.google.com with ESMTPS id hu9si17106208pdb.252.2015.07.10.19.53.51 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 10 Jul 2015 19:53:51 -0700 (PDT) Received: by pdrg1 with SMTP id g1so61445695pdr.2 for ; Fri, 10 Jul 2015 19:53:51 -0700 (PDT) From: Sergey Senozhatsky Subject: [PATCH 2/2] mm/shrinker: add init_shrinker() function Date: Sat, 11 Jul 2015 11:51:55 +0900 Message-Id: <1436583115-6323-3-git-send-email-sergey.senozhatsky@gmail.com> In-Reply-To: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> Sender: owner-linux-mm@kvack.org List-ID: To: Andrew Morton Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Sergey Senozhatsky , Sergey Senozhatsky All zeroes shrinker is now treated as 'initialized, but not registered'. If, for some reason, you can't zero your shrinker struct (or don't want to) then use init_shrinker() function. Otherwise, in some cases, unregister_shrinker() may Oops. Signed-off-by: Sergey Senozhatsky --- include/linux/shrinker.h | 1 + mm/vmscan.c | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h index 4fcacd9..bffb660 100644 --- a/include/linux/shrinker.h +++ b/include/linux/shrinker.h @@ -67,6 +67,7 @@ struct shrinker { #define SHRINKER_NUMA_AWARE (1 << 0) #define SHRINKER_MEMCG_AWARE (1 << 1) +extern void init_shrinker(struct shrinker *); extern int register_shrinker(struct shrinker *); extern void unregister_shrinker(struct shrinker *); #endif diff --git a/mm/vmscan.c b/mm/vmscan.c index cadc8a2..4bbcfcf 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -221,6 +221,18 @@ static unsigned long get_lru_size(struct lruvec *lruvec, enum lru_list lru) } /* + * All-zeroes shrinker considered to be initialized. Use this + * function if you can't (don't want to) zero out your shrinker + * structure. + */ +void init_shrinker(struct shrinker *shrinker) +{ + shrinker->nr_deferred = NULL; + INIT_LIST_HEAD(&shrinker->list); +} +EXPORT_SYMBOL(init_shrinker); + +/* * Add a shrinker callback to be called from the vm. */ int register_shrinker(struct shrinker *shrinker) -- 2.4.5 -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f43.google.com (mail-pa0-f43.google.com [209.85.220.43]) by kanga.kvack.org (Postfix) with ESMTP id E91586B0253 for ; Sat, 11 Jul 2015 06:02:38 -0400 (EDT) Received: by pactm7 with SMTP id tm7so180737779pac.2 for ; Sat, 11 Jul 2015 03:02:38 -0700 (PDT) Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id xs6si18567952pab.214.2015.07.11.03.02.34 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 11 Jul 2015 03:02:34 -0700 (PDT) Date: Sat, 11 Jul 2015 03:02:32 -0700 From: Christoph Hellwig Subject: Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Message-ID: <20150711100232.GA4607@infradead.org> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> Sender: owner-linux-mm@kvack.org List-ID: To: Sergey Senozhatsky Cc: Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Sergey Senozhatsky On Sat, Jul 11, 2015 at 11:51:53AM +0900, Sergey Senozhatsky wrote: > Hello, > > Shrinker API does not handle nicely unregister_shrinker() on a not-registered > ->shrinker. Looking at shrinker users, they all have to > (a) carry on some sort of a flag to make sure that "unregister_shrinker()" > will not blow up later > (b) be fishy (potentially can Oops) > (c) access private members `struct shrinker' (e.g. `shrink.list.next') Ayone who does that is broken. You just need to have clear init (with proper unwinding) and exit functions and order things properly. It works like most register/unregister calls and should stay that way. Maye you you should ty to explain what practical problem you're seeing to start with. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pd0-f172.google.com (mail-pd0-f172.google.com [209.85.192.172]) by kanga.kvack.org (Postfix) with ESMTP id 765C86B0253 for ; Sat, 11 Jul 2015 22:48:24 -0400 (EDT) Received: by pdjr16 with SMTP id r16so43478721pdj.3 for ; Sat, 11 Jul 2015 19:48:24 -0700 (PDT) Received: from mail-pd0-x22b.google.com (mail-pd0-x22b.google.com. [2607:f8b0:400e:c02::22b]) by mx.google.com with ESMTPS id x10si21548330pdr.182.2015.07.11.19.48.23 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 11 Jul 2015 19:48:23 -0700 (PDT) Received: by pdbep18 with SMTP id ep18so205408943pdb.1 for ; Sat, 11 Jul 2015 19:48:23 -0700 (PDT) Date: Sun, 12 Jul 2015 11:47:32 +0900 From: Sergey Senozhatsky Subject: Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Message-ID: <20150712024732.GA787@swordfish> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> <20150711100232.GA4607@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150711100232.GA4607@infradead.org> Sender: owner-linux-mm@kvack.org List-ID: To: Christoph Hellwig Cc: Sergey Senozhatsky , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Sergey Senozhatsky Hello Christoph, On (07/11/15 03:02), Christoph Hellwig wrote: > > Shrinker API does not handle nicely unregister_shrinker() on a not-registered > > ->shrinker. Looking at shrinker users, they all have to > > (a) carry on some sort of a flag to make sure that "unregister_shrinker()" > > will not blow up later > > (b) be fishy (potentially can Oops) > > (c) access private members `struct shrinker' (e.g. `shrink.list.next') > > Ayone who does that is broken. You just need to have clear init (with > proper unwinding) and exit functions and order things properly. It > works like most register/unregister calls and should stay that way. > > Maye you you should ty to explain what practical problem you're seeing > to start with. Yes, but the main difference here is that it seems that shrinker users don't tend to treat shrinker registration failures as fatal errors and just continue with shrinker functionality disabled. And it makes sense. (copy paste from https://lkml.org/lkml/2015/7/9/751) > Ayone who does that is broken I'm afraid, in that case we almost don't have not-broken shrinker users. -- ignoring register_shrinker() error : int ldlm_pools_init(void) : { : int rc; : : rc = ldlm_pools_thread_start(); : if (rc == 0) { : register_shrinker(&ldlm_pools_srv_shrinker); : register_shrinker(&ldlm_pools_cli_shrinker); : } : return rc; : } : EXPORT_SYMBOL(ldlm_pools_init); : : void ldlm_pools_fini(void) : { : unregister_shrinker(&ldlm_pools_srv_shrinker); : unregister_shrinker(&ldlm_pools_cli_shrinker); : ldlm_pools_thread_stop(); : } : EXPORT_SYMBOL(ldlm_pools_fini); -- and here :void i915_gem_shrinker_init(struct drm_i915_private *dev_priv) :{ : dev_priv->mm.shrinker.scan_objects = i915_gem_shrinker_scan; : dev_priv->mm.shrinker.count_objects = i915_gem_shrinker_count; : dev_priv->mm.shrinker.seeks = DEFAULT_SEEKS; : register_shrinker(&dev_priv->mm.shrinker); : : dev_priv->mm.oom_notifier.notifier_call = i915_gem_shrinker_oom; : register_oom_notifier(&dev_priv->mm.oom_notifier); :} -- and here :int __init gfs2_glock_init(void) :{ : unsigned i; ... : register_shrinker(&glock_shrinker); : : return 0; :} : :void gfs2_glock_exit(void) :{ : unregister_shrinker(&glock_shrinker); : destroy_workqueue(glock_workqueue); : destroy_workqueue(gfs2_delete_workqueue); :} -- and here :static int __init lowmem_init(void) :{ : register_shrinker(&lowmem_shrinker); : return 0; :} : :static void __exit lowmem_exit(void) :{ : unregister_shrinker(&lowmem_shrinker); :} -- accessing private member 'c->shrink.list.next' to distinguish between 'register_shrinker() was successful and need to unregister it' and 'register_shrinker() failed, don't unregister_shrinker() because it may Oops' :struct cache_set { : ... : struct shrinker shrink; : ... :}; : : ... : : void bch_btree_cache_free(struct cache_set *c) : { : struct btree *b; : struct closure cl; : closure_init_stack(&cl); : : if (c->shrink.list.next) : unregister_shrinker(&c->shrink); -- and here :int bch_btree_cache_alloc(struct cache_set *c) :{ ... : register_shrinker(&c->shrink); : : ... : :void bch_btree_cache_free(struct cache_set *c) :{ : struct btree *b; : struct closure cl; : closure_init_stack(&cl); : : if (c->shrink.list.next) : unregister_shrinker(&c->shrink); : And so on and on. In fact, 'git grep = register_shrinker' gives only $ git grep '= register_shrinker' fs/ext4/extents_status.c: err = register_shrinker(&sbi->s_es_shrinker); fs/nfsd/nfscache.c: status = register_shrinker(&nfsd_reply_cache_shrinker); fs/ubifs/super.c: err = register_shrinker(&ubifs_shrinker_info); mm/huge_memory.c: err = register_shrinker(&huge_zero_page_shrinker); mm/workingset.c: ret = register_shrinker(&workingset_shadow_shrinker); The rest is 'broken'. -ss -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f45.google.com (mail-pa0-f45.google.com [209.85.220.45]) by kanga.kvack.org (Postfix) with ESMTP id E59766B0253 for ; Mon, 13 Jul 2015 02:33:44 -0400 (EDT) Received: by pactm7 with SMTP id tm7so201966462pac.2 for ; Sun, 12 Jul 2015 23:33:44 -0700 (PDT) Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id jf2si20737898pbd.115.2015.07.12.23.33.43 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 12 Jul 2015 23:33:43 -0700 (PDT) Date: Sun, 12 Jul 2015 23:33:41 -0700 From: Christoph Hellwig Subject: Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Message-ID: <20150713063341.GA24167@infradead.org> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> <20150711100232.GA4607@infradead.org> <20150712024732.GA787@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150712024732.GA787@swordfish> Sender: owner-linux-mm@kvack.org List-ID: To: Sergey Senozhatsky Cc: Christoph Hellwig , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Sergey Senozhatsky On Sun, Jul 12, 2015 at 11:47:32AM +0900, Sergey Senozhatsky wrote: > Yes, but the main difference here is that it seems that shrinker users > don't tend to treat shrinker registration failures as fatal errors and > just continue with shrinker functionality disabled. And it makes sense. > > (copy paste from https://lkml.org/lkml/2015/7/9/751) > I hearily disagree. It's not any less critical than other failures. The right way forward is to handle register failure properly. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f53.google.com (mail-pa0-f53.google.com [209.85.220.53]) by kanga.kvack.org (Postfix) with ESMTP id 7CD8A6B0253 for ; Mon, 13 Jul 2015 02:52:23 -0400 (EDT) Received: by pachj5 with SMTP id hj5so26941448pac.3 for ; Sun, 12 Jul 2015 23:52:23 -0700 (PDT) Received: from mail-pa0-x22b.google.com (mail-pa0-x22b.google.com. [2607:f8b0:400e:c03::22b]) by mx.google.com with ESMTPS id g6si10548832pdn.197.2015.07.12.23.52.22 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sun, 12 Jul 2015 23:52:22 -0700 (PDT) Received: by pachj5 with SMTP id hj5so26941228pac.3 for ; Sun, 12 Jul 2015 23:52:22 -0700 (PDT) Date: Mon, 13 Jul 2015 15:52:53 +0900 From: Sergey Senozhatsky Subject: Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Message-ID: <20150713065253.GA811@swordfish> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> <20150711100232.GA4607@infradead.org> <20150712024732.GA787@swordfish> <20150713063341.GA24167@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150713063341.GA24167@infradead.org> Sender: owner-linux-mm@kvack.org List-ID: To: Christoph Hellwig Cc: Sergey Senozhatsky , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Sergey Senozhatsky On (07/12/15 23:33), Christoph Hellwig wrote: > On Sun, Jul 12, 2015 at 11:47:32AM +0900, Sergey Senozhatsky wrote: > > Yes, but the main difference here is that it seems that shrinker users > > don't tend to treat shrinker registration failures as fatal errors and > > just continue with shrinker functionality disabled. And it makes sense. > > > > (copy paste from https://lkml.org/lkml/2015/7/9/751) > > > > I hearily disagree. It's not any less critical than other failures. Why? In some sense, shrinker callbacks are just a way to be nice. No one writes a driver just to be able to handle shrinker calls. An ability to react to those calls is just additional option; it does not directly affect or limit driver's functionality (at least, it really should not). > The right way forward is to handle register failure properly. In other words, to (a) keep a flag to signify that register was not successful or (b) look at ->shrinker.list.next or ->nr_deferred or (c) treat register failures as critical errors. (I sort of disagree with you here). -ss -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f54.google.com (mail-pa0-f54.google.com [209.85.220.54]) by kanga.kvack.org (Postfix) with ESMTP id A80966B0253 for ; Mon, 13 Jul 2015 05:04:01 -0400 (EDT) Received: by pactm7 with SMTP id tm7so204176781pac.2 for ; Mon, 13 Jul 2015 02:04:01 -0700 (PDT) Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id ox1si27428831pdb.28.2015.07.13.02.04.00 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 13 Jul 2015 02:04:00 -0700 (PDT) Date: Mon, 13 Jul 2015 02:03:58 -0700 From: Christoph Hellwig Subject: Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Message-ID: <20150713090358.GA28901@infradead.org> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> <20150711100232.GA4607@infradead.org> <20150712024732.GA787@swordfish> <20150713063341.GA24167@infradead.org> <20150713065253.GA811@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150713065253.GA811@swordfish> Sender: owner-linux-mm@kvack.org List-ID: To: Sergey Senozhatsky Cc: Sergey Senozhatsky , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org On Mon, Jul 13, 2015 at 03:52:53PM +0900, Sergey Senozhatsky wrote: > Why? In some sense, shrinker callbacks are just a way to be nice. > No one writes a driver just to be able to handle shrinker calls. An > ability to react to those calls is just additional option; it does > not directly affect or limit driver's functionality (at least, it > really should not). No, they are not just nice. They are a fundamental part of memory management and required to reclaim (often large) amounts of memory. Nevermind that we don't ignore any other registration time error in the kernel. > > The right way forward is to handle register failure properly. > > In other words, to > (a) keep a flag to signify that register was not successful > or > (b) look at ->shrinker.list.next or ->nr_deferred > or > (c) treat register failures as critical errors. (I sort of > disagree with you here). The only important part is here is (c). -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f45.google.com (mail-pa0-f45.google.com [209.85.220.45]) by kanga.kvack.org (Postfix) with ESMTP id 64A216B0253 for ; Mon, 13 Jul 2015 05:34:11 -0400 (EDT) Received: by pachj5 with SMTP id hj5so29320271pac.3 for ; Mon, 13 Jul 2015 02:34:11 -0700 (PDT) Received: from mail-pa0-x230.google.com (mail-pa0-x230.google.com. [2607:f8b0:400e:c03::230]) by mx.google.com with ESMTPS id ol11si27581457pab.5.2015.07.13.02.34.10 for (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 13 Jul 2015 02:34:10 -0700 (PDT) Received: by padck2 with SMTP id ck2so39993677pad.0 for ; Mon, 13 Jul 2015 02:34:10 -0700 (PDT) Date: Mon, 13 Jul 2015 18:34:42 +0900 From: Sergey Senozhatsky Subject: Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Message-ID: <20150713092531.GA578@swordfish> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> <20150711100232.GA4607@infradead.org> <20150712024732.GA787@swordfish> <20150713063341.GA24167@infradead.org> <20150713065253.GA811@swordfish> <20150713090358.GA28901@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150713090358.GA28901@infradead.org> Sender: owner-linux-mm@kvack.org List-ID: To: Christoph Hellwig Cc: Sergey Senozhatsky , Sergey Senozhatsky , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org On (07/13/15 02:03), Christoph Hellwig wrote: > On Mon, Jul 13, 2015 at 03:52:53PM +0900, Sergey Senozhatsky wrote: > > Why? In some sense, shrinker callbacks are just a way to be nice. > > No one writes a driver just to be able to handle shrinker calls. An > > ability to react to those calls is just additional option; it does > > not directly affect or limit driver's functionality (at least, it > > really should not). > > No, they are not just nice. They are a fundamental part of memory > management and required to reclaim (often large) amounts of memory. Yes. 'Nice' used in a sense that drivers have logic to release the memory anyway; mm asks volunteers (the drivers that have registered shrinker callbacks) to release some spare/wasted/etc. when things are getting tough (the drivers are not aware of that in general). This is surely important to mm, not to the driver though -- it just agrees to be 'nice', but even not expected to release any memory at all (IOW, this is not a contract). -ss -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-f41.google.com (mail-pa0-f41.google.com [209.85.220.41]) by kanga.kvack.org (Postfix) with ESMTP id 544596B0255 for ; Tue, 14 Jul 2015 03:18:08 -0400 (EDT) Received: by padck2 with SMTP id ck2so1071551pad.0 for ; Tue, 14 Jul 2015 00:18:08 -0700 (PDT) Received: from bombadil.infradead.org (bombadil.infradead.org. [2001:1868:205::9]) by mx.google.com with ESMTPS id q5si165290pdj.240.2015.07.14.00.18.03 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 14 Jul 2015 00:18:03 -0700 (PDT) Date: Tue, 14 Jul 2015 00:17:59 -0700 From: Christoph Hellwig Subject: Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Message-ID: <20150714071759.GD31117@infradead.org> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> <20150711100232.GA4607@infradead.org> <20150712024732.GA787@swordfish> <20150713063341.GA24167@infradead.org> <20150713065253.GA811@swordfish> <20150713090358.GA28901@infradead.org> <20150713092531.GA578@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150713092531.GA578@swordfish> Sender: owner-linux-mm@kvack.org List-ID: To: Sergey Senozhatsky Cc: Christoph Hellwig , Sergey Senozhatsky , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org On Mon, Jul 13, 2015 at 06:34:42PM +0900, Sergey Senozhatsky wrote: > Yes. 'Nice' used in a sense that drivers have logic to release the > memory anyway; mm asks volunteers (the drivers that have registered > shrinker callbacks) to release some spare/wasted/etc. when things > are getting tough (the drivers are not aware of that in general). > This is surely important to mm, not to the driver though -- it just > agrees to be 'nice', but even not expected to release any memory at > all (IOW, this is not a contract). Not registering the shrinker is a plain and simple memory leak. Just like a missing free your driver will appear to work fine for a while, but eventually the leaks will bring down the whole system including your driver. -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753570AbbGKGs1 (ORCPT ); Sat, 11 Jul 2015 02:48:27 -0400 Received: from mail-pa0-f42.google.com ([209.85.220.42]:34946 "EHLO mail-pa0-f42.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753332AbbGKGsW (ORCPT ); Sat, 11 Jul 2015 02:48:22 -0400 From: Sergey Senozhatsky To: Andrew Morton Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Sergey Senozhatsky , Sergey Senozhatsky Subject: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Date: Sat, 11 Jul 2015 11:51:53 +0900 Message-Id: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> X-Mailer: git-send-email 2.4.5 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello, Shrinker API does not handle nicely unregister_shrinker() on a not-registered ->shrinker. Looking at shrinker users, they all have to (a) carry on some sort of a flag to make sure that "unregister_shrinker()" will not blow up later (b) be fishy (potentially can Oops) (c) access private members `struct shrinker' (e.g. `shrink.list.next') Change unregister_shrinker() to consider all-zeroes shrinker as 'initialized, but not registered' shrinker, so we can avoid NULL dereference when unregister_shrinker() accidentally receives such a struct. Introduce init_shrinker() function to init `critical' shrinkers members when the entire shrinker cannot be, for some reason, zeroed out. This also helps to avoid Oops in unregister_shrinker() in some cases (when unregister_shrinker() receives not initialized and not registered shrinker). Sergey Senozhatsky (2): mm/shrinker: do not NULL dereference uninitialized shrinker mm/shrinker: add init_shrinker() function include/linux/shrinker.h | 1 + mm/vmscan.c | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) -- 2.4.5 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752812AbbGKGml (ORCPT ); Sat, 11 Jul 2015 02:42:41 -0400 Received: from mail-pd0-f178.google.com ([209.85.192.178]:34429 "EHLO mail-pd0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751602AbbGKGmk (ORCPT ); Sat, 11 Jul 2015 02:42:40 -0400 From: Sergey Senozhatsky To: Andrew Morton Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Sergey Senozhatsky , Sergey Senozhatsky Subject: [PATCH 1/2] mm/shrinker: do not NULL dereference uninitialized shrinker Date: Sat, 11 Jul 2015 11:51:54 +0900 Message-Id: <1436583115-6323-2-git-send-email-sergey.senozhatsky@gmail.com> X-Mailer: git-send-email 2.4.5 In-Reply-To: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Consider 'all zeroes' shrinker as 'initialized, but not registered', and, thus, don't unregister such a shrinker. This helps to avoid accidental NULL pointer dereferences, when a zeroed shrinker struct is getting passed to unregister_shrinker() in error handing path, for example. Signed-off-by: Sergey Senozhatsky --- mm/vmscan.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/mm/vmscan.c b/mm/vmscan.c index c8d8282..cadc8a2 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -254,6 +254,12 @@ EXPORT_SYMBOL(register_shrinker); */ void unregister_shrinker(struct shrinker *shrinker) { + /* + * All-zeroes is 'initialized, but not registered' shrinker. + */ + if (unlikely(!shrinker->list.next)) + return; + down_write(&shrinker_rwsem); list_del(&shrinker->list); up_write(&shrinker_rwsem); -- 2.4.5 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752742AbbGKGhU (ORCPT ); Sat, 11 Jul 2015 02:37:20 -0400 Received: from mail-pd0-f169.google.com ([209.85.192.169]:35479 "EHLO mail-pd0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751809AbbGKGhS (ORCPT ); Sat, 11 Jul 2015 02:37:18 -0400 From: Sergey Senozhatsky To: Andrew Morton Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Sergey Senozhatsky , Sergey Senozhatsky Subject: [PATCH 2/2] mm/shrinker: add init_shrinker() function Date: Sat, 11 Jul 2015 11:51:55 +0900 Message-Id: <1436583115-6323-3-git-send-email-sergey.senozhatsky@gmail.com> X-Mailer: git-send-email 2.4.5 In-Reply-To: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org All zeroes shrinker is now treated as 'initialized, but not registered'. If, for some reason, you can't zero your shrinker struct (or don't want to) then use init_shrinker() function. Otherwise, in some cases, unregister_shrinker() may Oops. Signed-off-by: Sergey Senozhatsky --- include/linux/shrinker.h | 1 + mm/vmscan.c | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/include/linux/shrinker.h b/include/linux/shrinker.h index 4fcacd9..bffb660 100644 --- a/include/linux/shrinker.h +++ b/include/linux/shrinker.h @@ -67,6 +67,7 @@ struct shrinker { #define SHRINKER_NUMA_AWARE (1 << 0) #define SHRINKER_MEMCG_AWARE (1 << 1) +extern void init_shrinker(struct shrinker *); extern int register_shrinker(struct shrinker *); extern void unregister_shrinker(struct shrinker *); #endif diff --git a/mm/vmscan.c b/mm/vmscan.c index cadc8a2..4bbcfcf 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -221,6 +221,18 @@ static unsigned long get_lru_size(struct lruvec *lruvec, enum lru_list lru) } /* + * All-zeroes shrinker considered to be initialized. Use this + * function if you can't (don't want to) zero out your shrinker + * structure. + */ +void init_shrinker(struct shrinker *shrinker) +{ + shrinker->nr_deferred = NULL; + INIT_LIST_HEAD(&shrinker->list); +} +EXPORT_SYMBOL(init_shrinker); + +/* * Add a shrinker callback to be called from the vm. */ int register_shrinker(struct shrinker *shrinker) -- 2.4.5 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753788AbbGKKCg (ORCPT ); Sat, 11 Jul 2015 06:02:36 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:44526 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753426AbbGKKCf (ORCPT ); Sat, 11 Jul 2015 06:02:35 -0400 Date: Sat, 11 Jul 2015 03:02:32 -0700 From: Christoph Hellwig To: Sergey Senozhatsky Cc: Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Message-ID: <20150711100232.GA4607@infradead.org> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-SRS-Rewrite: SMTP reverse-path rewritten from by bombadil.infradead.org See http://www.infradead.org/rpr.html Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Jul 11, 2015 at 11:51:53AM +0900, Sergey Senozhatsky wrote: > Hello, > > Shrinker API does not handle nicely unregister_shrinker() on a not-registered > ->shrinker. Looking at shrinker users, they all have to > (a) carry on some sort of a flag to make sure that "unregister_shrinker()" > will not blow up later > (b) be fishy (potentially can Oops) > (c) access private members `struct shrinker' (e.g. `shrink.list.next') Ayone who does that is broken. You just need to have clear init (with proper unwinding) and exit functions and order things properly. It works like most register/unregister calls and should stay that way. Maye you you should ty to explain what practical problem you're seeing to start with. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752574AbbGLCsZ (ORCPT ); Sat, 11 Jul 2015 22:48:25 -0400 Received: from mail-pd0-f177.google.com ([209.85.192.177]:34959 "EHLO mail-pd0-f177.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752427AbbGLCsX (ORCPT ); Sat, 11 Jul 2015 22:48:23 -0400 Date: Sun, 12 Jul 2015 11:47:32 +0900 From: Sergey Senozhatsky To: Christoph Hellwig Cc: Sergey Senozhatsky , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Message-ID: <20150712024732.GA787@swordfish> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> <20150711100232.GA4607@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150711100232.GA4607@infradead.org> User-Agent: Mutt/1.5.23+89 (0255b37be491) (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Christoph, On (07/11/15 03:02), Christoph Hellwig wrote: > > Shrinker API does not handle nicely unregister_shrinker() on a not-registered > > ->shrinker. Looking at shrinker users, they all have to > > (a) carry on some sort of a flag to make sure that "unregister_shrinker()" > > will not blow up later > > (b) be fishy (potentially can Oops) > > (c) access private members `struct shrinker' (e.g. `shrink.list.next') > > Ayone who does that is broken. You just need to have clear init (with > proper unwinding) and exit functions and order things properly. It > works like most register/unregister calls and should stay that way. > > Maye you you should ty to explain what practical problem you're seeing > to start with. Yes, but the main difference here is that it seems that shrinker users don't tend to treat shrinker registration failures as fatal errors and just continue with shrinker functionality disabled. And it makes sense. (copy paste from https://lkml.org/lkml/2015/7/9/751) > Ayone who does that is broken I'm afraid, in that case we almost don't have not-broken shrinker users. -- ignoring register_shrinker() error : int ldlm_pools_init(void) : { : int rc; : : rc = ldlm_pools_thread_start(); : if (rc == 0) { : register_shrinker(&ldlm_pools_srv_shrinker); : register_shrinker(&ldlm_pools_cli_shrinker); : } : return rc; : } : EXPORT_SYMBOL(ldlm_pools_init); : : void ldlm_pools_fini(void) : { : unregister_shrinker(&ldlm_pools_srv_shrinker); : unregister_shrinker(&ldlm_pools_cli_shrinker); : ldlm_pools_thread_stop(); : } : EXPORT_SYMBOL(ldlm_pools_fini); -- and here :void i915_gem_shrinker_init(struct drm_i915_private *dev_priv) :{ : dev_priv->mm.shrinker.scan_objects = i915_gem_shrinker_scan; : dev_priv->mm.shrinker.count_objects = i915_gem_shrinker_count; : dev_priv->mm.shrinker.seeks = DEFAULT_SEEKS; : register_shrinker(&dev_priv->mm.shrinker); : : dev_priv->mm.oom_notifier.notifier_call = i915_gem_shrinker_oom; : register_oom_notifier(&dev_priv->mm.oom_notifier); :} -- and here :int __init gfs2_glock_init(void) :{ : unsigned i; ... : register_shrinker(&glock_shrinker); : : return 0; :} : :void gfs2_glock_exit(void) :{ : unregister_shrinker(&glock_shrinker); : destroy_workqueue(glock_workqueue); : destroy_workqueue(gfs2_delete_workqueue); :} -- and here :static int __init lowmem_init(void) :{ : register_shrinker(&lowmem_shrinker); : return 0; :} : :static void __exit lowmem_exit(void) :{ : unregister_shrinker(&lowmem_shrinker); :} -- accessing private member 'c->shrink.list.next' to distinguish between 'register_shrinker() was successful and need to unregister it' and 'register_shrinker() failed, don't unregister_shrinker() because it may Oops' :struct cache_set { : ... : struct shrinker shrink; : ... :}; : : ... : : void bch_btree_cache_free(struct cache_set *c) : { : struct btree *b; : struct closure cl; : closure_init_stack(&cl); : : if (c->shrink.list.next) : unregister_shrinker(&c->shrink); -- and here :int bch_btree_cache_alloc(struct cache_set *c) :{ ... : register_shrinker(&c->shrink); : : ... : :void bch_btree_cache_free(struct cache_set *c) :{ : struct btree *b; : struct closure cl; : closure_init_stack(&cl); : : if (c->shrink.list.next) : unregister_shrinker(&c->shrink); : And so on and on. In fact, 'git grep = register_shrinker' gives only $ git grep '= register_shrinker' fs/ext4/extents_status.c: err = register_shrinker(&sbi->s_es_shrinker); fs/nfsd/nfscache.c: status = register_shrinker(&nfsd_reply_cache_shrinker); fs/ubifs/super.c: err = register_shrinker(&ubifs_shrinker_info); mm/huge_memory.c: err = register_shrinker(&huge_zero_page_shrinker); mm/workingset.c: ret = register_shrinker(&workingset_shadow_shrinker); The rest is 'broken'. -ss From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751493AbbGMGdp (ORCPT ); Mon, 13 Jul 2015 02:33:45 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:51455 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751345AbbGMGdo (ORCPT ); Mon, 13 Jul 2015 02:33:44 -0400 Date: Sun, 12 Jul 2015 23:33:41 -0700 From: Christoph Hellwig To: Sergey Senozhatsky Cc: Christoph Hellwig , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Message-ID: <20150713063341.GA24167@infradead.org> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> <20150711100232.GA4607@infradead.org> <20150712024732.GA787@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150712024732.GA787@swordfish> User-Agent: Mutt/1.5.23 (2014-03-12) X-SRS-Rewrite: SMTP reverse-path rewritten from by bombadil.infradead.org See http://www.infradead.org/rpr.html Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Jul 12, 2015 at 11:47:32AM +0900, Sergey Senozhatsky wrote: > Yes, but the main difference here is that it seems that shrinker users > don't tend to treat shrinker registration failures as fatal errors and > just continue with shrinker functionality disabled. And it makes sense. > > (copy paste from https://lkml.org/lkml/2015/7/9/751) > I hearily disagree. It's not any less critical than other failures. The right way forward is to handle register failure properly. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751481AbbGMGwY (ORCPT ); Mon, 13 Jul 2015 02:52:24 -0400 Received: from mail-pa0-f44.google.com ([209.85.220.44]:35253 "EHLO mail-pa0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751354AbbGMGwW (ORCPT ); Mon, 13 Jul 2015 02:52:22 -0400 Date: Mon, 13 Jul 2015 15:52:53 +0900 From: Sergey Senozhatsky To: Christoph Hellwig Cc: Sergey Senozhatsky , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org, Sergey Senozhatsky Subject: Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Message-ID: <20150713065253.GA811@swordfish> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> <20150711100232.GA4607@infradead.org> <20150712024732.GA787@swordfish> <20150713063341.GA24167@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150713063341.GA24167@infradead.org> User-Agent: Mutt/1.5.23+89 (0255b37be491) (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On (07/12/15 23:33), Christoph Hellwig wrote: > On Sun, Jul 12, 2015 at 11:47:32AM +0900, Sergey Senozhatsky wrote: > > Yes, but the main difference here is that it seems that shrinker users > > don't tend to treat shrinker registration failures as fatal errors and > > just continue with shrinker functionality disabled. And it makes sense. > > > > (copy paste from https://lkml.org/lkml/2015/7/9/751) > > > > I hearily disagree. It's not any less critical than other failures. Why? In some sense, shrinker callbacks are just a way to be nice. No one writes a driver just to be able to handle shrinker calls. An ability to react to those calls is just additional option; it does not directly affect or limit driver's functionality (at least, it really should not). > The right way forward is to handle register failure properly. In other words, to (a) keep a flag to signify that register was not successful or (b) look at ->shrinker.list.next or ->nr_deferred or (c) treat register failures as critical errors. (I sort of disagree with you here). -ss From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751122AbbGMJED (ORCPT ); Mon, 13 Jul 2015 05:04:03 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:58698 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750787AbbGMJEB (ORCPT ); Mon, 13 Jul 2015 05:04:01 -0400 Date: Mon, 13 Jul 2015 02:03:58 -0700 From: Christoph Hellwig To: Sergey Senozhatsky Cc: Sergey Senozhatsky , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Message-ID: <20150713090358.GA28901@infradead.org> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> <20150711100232.GA4607@infradead.org> <20150712024732.GA787@swordfish> <20150713063341.GA24167@infradead.org> <20150713065253.GA811@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150713065253.GA811@swordfish> User-Agent: Mutt/1.5.23 (2014-03-12) X-SRS-Rewrite: SMTP reverse-path rewritten from by bombadil.infradead.org See http://www.infradead.org/rpr.html Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jul 13, 2015 at 03:52:53PM +0900, Sergey Senozhatsky wrote: > Why? In some sense, shrinker callbacks are just a way to be nice. > No one writes a driver just to be able to handle shrinker calls. An > ability to react to those calls is just additional option; it does > not directly affect or limit driver's functionality (at least, it > really should not). No, they are not just nice. They are a fundamental part of memory management and required to reclaim (often large) amounts of memory. Nevermind that we don't ignore any other registration time error in the kernel. > > The right way forward is to handle register failure properly. > > In other words, to > (a) keep a flag to signify that register was not successful > or > (b) look at ->shrinker.list.next or ->nr_deferred > or > (c) treat register failures as critical errors. (I sort of > disagree with you here). The only important part is here is (c). From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752573AbbGMJeO (ORCPT ); Mon, 13 Jul 2015 05:34:14 -0400 Received: from mail-pa0-f48.google.com ([209.85.220.48]:35403 "EHLO mail-pa0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751667AbbGMJeK (ORCPT ); Mon, 13 Jul 2015 05:34:10 -0400 Date: Mon, 13 Jul 2015 18:34:42 +0900 From: Sergey Senozhatsky To: Christoph Hellwig Cc: Sergey Senozhatsky , Sergey Senozhatsky , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Message-ID: <20150713092531.GA578@swordfish> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> <20150711100232.GA4607@infradead.org> <20150712024732.GA787@swordfish> <20150713063341.GA24167@infradead.org> <20150713065253.GA811@swordfish> <20150713090358.GA28901@infradead.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150713090358.GA28901@infradead.org> User-Agent: Mutt/1.5.23+89 (0255b37be491) (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On (07/13/15 02:03), Christoph Hellwig wrote: > On Mon, Jul 13, 2015 at 03:52:53PM +0900, Sergey Senozhatsky wrote: > > Why? In some sense, shrinker callbacks are just a way to be nice. > > No one writes a driver just to be able to handle shrinker calls. An > > ability to react to those calls is just additional option; it does > > not directly affect or limit driver's functionality (at least, it > > really should not). > > No, they are not just nice. They are a fundamental part of memory > management and required to reclaim (often large) amounts of memory. Yes. 'Nice' used in a sense that drivers have logic to release the memory anyway; mm asks volunteers (the drivers that have registered shrinker callbacks) to release some spare/wasted/etc. when things are getting tough (the drivers are not aware of that in general). This is surely important to mm, not to the driver though -- it just agrees to be 'nice', but even not expected to release any memory at all (IOW, this is not a contract). -ss From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752672AbbGNHSG (ORCPT ); Tue, 14 Jul 2015 03:18:06 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:59732 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751265AbbGNHSE (ORCPT ); Tue, 14 Jul 2015 03:18:04 -0400 Date: Tue, 14 Jul 2015 00:17:59 -0700 From: Christoph Hellwig To: Sergey Senozhatsky Cc: Christoph Hellwig , Sergey Senozhatsky , Andrew Morton , linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 0/2] mm/shrinker: make unregister_shrinker() less fragile Message-ID: <20150714071759.GD31117@infradead.org> References: <1436583115-6323-1-git-send-email-sergey.senozhatsky@gmail.com> <20150711100232.GA4607@infradead.org> <20150712024732.GA787@swordfish> <20150713063341.GA24167@infradead.org> <20150713065253.GA811@swordfish> <20150713090358.GA28901@infradead.org> <20150713092531.GA578@swordfish> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150713092531.GA578@swordfish> User-Agent: Mutt/1.5.23 (2014-03-12) X-SRS-Rewrite: SMTP reverse-path rewritten from by bombadil.infradead.org See http://www.infradead.org/rpr.html Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Jul 13, 2015 at 06:34:42PM +0900, Sergey Senozhatsky wrote: > Yes. 'Nice' used in a sense that drivers have logic to release the > memory anyway; mm asks volunteers (the drivers that have registered > shrinker callbacks) to release some spare/wasted/etc. when things > are getting tough (the drivers are not aware of that in general). > This is surely important to mm, not to the driver though -- it just > agrees to be 'nice', but even not expected to release any memory at > all (IOW, this is not a contract). Not registering the shrinker is a plain and simple memory leak. Just like a missing free your driver will appear to work fine for a while, but eventually the leaks will bring down the whole system including your driver.