Hello,
RFC
Proposed by Andrew Morton: https://lkml.org/lkml/2015/6/8/583
The existing pools' destroy() functions do not allow NULL pool pointers;
instead, every destructor() caller forced to check if pool is not NULL,
which:
a) requires additional attention from developers/reviewers
b) may lead to a NULL pointer dereferences if (a) didn't work
First 3 patches tweak
- kmem_cache_destroy()
- mempool_destroy()
- dma_pool_destroy()
to handle NULL pointers.
Basically, this patch set will:
1) Can prevent us from still undiscovered NULL pointer dereferences.
(like the one that was addressed in https://lkml.org/lkml/2015/6/5/262)
2) Make a cleanup possible. Things like:
[..]
if (xhci->segment_pool)
dma_pool_destroy(xhci->segment_pool);
..
if (xhci->device_pool)
dma_pool_destroy(xhci->device_pool);
..
if (xhci->small_streams_pool)
dma_pool_destroy(xhci->small_streams_pool);
..
if (xhci->medium_streams_pool)
dma_pool_destroy(xhci->medium_streams_pool);
[..]
or
[..]
fail_dma_pool:
if (IS_QLA82XX(ha) || ql2xenabledif) {
dma_pool_destroy(ha->fcp_cmnd_dma_pool);
ha->fcp_cmnd_dma_pool = NULL;
}
fail_dl_dma_pool:
if (IS_QLA82XX(ha) || ql2xenabledif) {
dma_pool_destroy(ha->dl_dma_pool);
ha->dl_dma_pool = NULL;
}
fail_s_dma_pool:
dma_pool_destroy(ha->s_dma_pool);
ha->s_dma_pool = NULL;
[..]
may now be simplified.
0004 and 0005 are not so necessary, simply because there are not
so many users of these two (added for pool's destroy() functions consistency):
-- zpool_destroy_pool()
-- zs_destroy_pool()
So, 0004 and 0005 can be dropped.
- zbud does kfree() in zbud_destroy_pool(), so I didn't touch it.
Sergey Senozhatsky (5):
mm/slab_common: allow NULL cache pointer in kmem_cache_destroy()
mm/mempool: allow NULL `pool' pointer in mempool_destroy()
mm/dmapool: allow NULL `pool' pointer in dma_pool_destroy()
mm/zpool: allow NULL `zpool' pointer in zpool_destroy_pool()
mm/zsmalloc: allow NULL `pool' pointer in zs_destroy_pool()
mm/dmapool.c | 3 +++
mm/mempool.c | 3 +++
mm/slab_common.c | 3 +++
mm/zpool.c | 3 +++
mm/zsmalloc.c | 3 +++
5 files changed, 15 insertions(+)
--
2.4.3.368.g7974889
kmem_cache_destroy() does not tolerate a NULL kmem_cache pointer
argument and performs a NULL-pointer dereference. This requires
additional attention and effort from developers/reviewers and
forces all kmem_cache_destroy() callers (200+ as of 4.1) to do
a NULL check
if (cache)
kmem_cache_destroy(cache);
Or, otherwise, be invalid kmem_cache_destroy() users.
Tweak kmem_cache_destroy() and NULL-check the pointer there.
Proposed by Andrew Morton.
Signed-off-by: Sergey Senozhatsky <[email protected]>
Reported-by: Andrew Morton <[email protected]>
LKML-reference: https://lkml.org/lkml/2015/6/8/583
---
mm/slab_common.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 8873985..ea69b13 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -641,6 +641,9 @@ void kmem_cache_destroy(struct kmem_cache *s)
bool need_rcu_barrier = false;
bool busy = false;
+ if (unlikely(!s))
+ return;
+
BUG_ON(!is_root_cache(s));
get_online_cpus();
--
2.4.3.368.g7974889
mempool_destroy() does not tolerate a NULL mempool_t pointer
argument and performs a NULL-pointer dereference. This requires
additional attention and effort from developers/reviewers and
forces all mempool_destroy() callers to do a NULL check
if (pool)
mempool_destroy(pool);
Or, otherwise, be invalid mempool_destroy() users.
Tweak mempool_destroy() and NULL-check the pointer there.
Proposed by Andrew Morton.
Signed-off-by: Sergey Senozhatsky <[email protected]>
Reported-by: Andrew Morton <[email protected]>
LKML-reference: https://lkml.org/lkml/2015/6/8/583
---
mm/mempool.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/mm/mempool.c b/mm/mempool.c
index 2cc08de..4c533bc 100644
--- a/mm/mempool.c
+++ b/mm/mempool.c
@@ -150,6 +150,9 @@ static void *remove_element(mempool_t *pool)
*/
void mempool_destroy(mempool_t *pool)
{
+ if (unlikely(!pool))
+ return;
+
while (pool->curr_nr) {
void *element = remove_element(pool);
pool->free(element, pool->pool_data);
--
2.4.3.368.g7974889
dma_pool_destroy() does not tolerate a NULL dma_pool pointer
argument and performs a NULL-pointer dereference. This requires
additional attention and effort from developers/reviewers and
forces all dma_pool_destroy() callers to do a NULL check
if (pool)
dma_pool_destroy(pool);
Or, otherwise, be invalid dma_pool_destroy() users.
Tweak dma_pool_destroy() and NULL-check the pointer there.
Proposed by Andrew Morton.
Signed-off-by: Sergey Senozhatsky <[email protected]>
Reported-by: Andrew Morton <[email protected]>
LKML-reference: https://lkml.org/lkml/2015/6/8/583
---
mm/dmapool.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/mm/dmapool.c b/mm/dmapool.c
index fd5fe43..5f2cffc 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -271,6 +271,9 @@ void dma_pool_destroy(struct dma_pool *pool)
{
bool empty = false;
+ if (unlikely(!pool))
+ return;
+
mutex_lock(&pools_reg_lock);
mutex_lock(&pools_lock);
list_del(&pool->pools);
--
2.4.3.368.g7974889
zpool_destroy_pool() does not tolerate a NULL zpool pointer
argument and performs a NULL-pointer dereference. Although
there is only one zpool_destroy_pool() user (as of 4.1),
still update it to be coherent with the corresponding
destroy() functions of the remainig pool-allocators (slab,
mempool, etc.), which now allow NULL pool-pointers.
For consistency, tweak zpool_destroy_pool() and NULL-check the
pointer there.
Proposed by Andrew Morton.
Signed-off-by: Sergey Senozhatsky <[email protected]>
Reported-by: Andrew Morton <[email protected]>
LKML-reference: https://lkml.org/lkml/2015/6/8/583
---
mm/zpool.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/mm/zpool.c b/mm/zpool.c
index bacdab6..2f59b90 100644
--- a/mm/zpool.c
+++ b/mm/zpool.c
@@ -202,6 +202,9 @@ struct zpool *zpool_create_pool(char *type, char *name, gfp_t gfp,
*/
void zpool_destroy_pool(struct zpool *zpool)
{
+ if (unlikely(!zpool))
+ return;
+
pr_info("destroying pool type %s\n", zpool->type);
spin_lock(&pools_lock);
--
2.4.3.368.g7974889
zpool_destroy_pool() does not tolerate a NULL zs_pool pointer
argument and performs a NULL-pointer dereference. Although
there are quite a few zs_destroy_pool() users, still update
it to be coherent with the corresponding destroy() functions
of the remainig pool-allocators (slab, mempool, etc.), which
now allow NULL pool-pointers.
For consistency, tweak zpool_destroy_pool() and NULL-check the
pointer there.
Proposed by Andrew Morton.
Signed-off-by: Sergey Senozhatsky <[email protected]>
Reported-by: Andrew Morton <[email protected]>
LKML-reference: https://lkml.org/lkml/2015/6/8/583
---
mm/zsmalloc.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index c766240..80964d2 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -1868,6 +1868,9 @@ void zs_destroy_pool(struct zs_pool *pool)
{
int i;
+ if (unlikely(!pool))
+ return;
+
zs_pool_stat_destroy(pool);
for (i = 0; i < zs_size_classes; i++) {
--
2.4.3.368.g7974889
On Tue, 9 Jun 2015 21:04:48 +0900 Sergey Senozhatsky <[email protected]> wrote:
> The existing pools' destroy() functions do not allow NULL pool pointers;
> instead, every destructor() caller forced to check if pool is not NULL,
> which:
> a) requires additional attention from developers/reviewers
> b) may lead to a NULL pointer dereferences if (a) didn't work
>
>
> First 3 patches tweak
> - kmem_cache_destroy()
> - mempool_destroy()
> - dma_pool_destroy()
>
> to handle NULL pointers.
Well I like it, even though it's going to cause a zillion little cleanup
patches.
checkpatch already has a "kfree(NULL) is safe and this check is
probably not required" test so I guess Joe will need to get busy ;)
I'll park these patches until after 4.1 is released - it's getting to
that time...
On Tue, 2015-06-09 at 14:25 -0700, Andrew Morton wrote:
> On Tue, 9 Jun 2015 21:04:48 +0900 Sergey Senozhatsky <[email protected]> wrote:
>
> > The existing pools' destroy() functions do not allow NULL pool pointers;
> > instead, every destructor() caller forced to check if pool is not NULL,
> > which:
> > a) requires additional attention from developers/reviewers
> > b) may lead to a NULL pointer dereferences if (a) didn't work
> >
> >
> > First 3 patches tweak
> > - kmem_cache_destroy()
> > - mempool_destroy()
> > - dma_pool_destroy()
> >
> > to handle NULL pointers.
>
> Well I like it, even though it's going to cause a zillion little cleanup
> patches.
>
> checkpatch already has a "kfree(NULL) is safe and this check is
> probably not required" test so I guess Joe will need to get busy ;)
Maybe it'll be Julia's crew.
The checkpatch change is pretty trivial
---
scripts/checkpatch.pl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 69c4716..3d6e34d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4801,7 +4801,7 @@ sub process {
# check for needless "if (<foo>) fn(<foo>)" uses
if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
- if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
+ if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?|(?:kmem_cache|mempool|dma_pool)_destroy)$expr/) {
WARN('NEEDLESS_IF',
"$1(NULL) is safe and this check is probably not required\n" . $hereprev);
}
On Tue, 9 Jun 2015, Andrew Morton wrote:
> Well I like it, even though it's going to cause a zillion little cleanup
> patches.
>
> checkpatch already has a "kfree(NULL) is safe and this check is
> probably not required" test so I guess Joe will need to get busy ;)
>
> I'll park these patches until after 4.1 is released - it's getting to
> that time...
Why do this at all? I understand that kfree/kmem_cache_free can take a
null pointer but this is the destruction of a cache and it usually
requires multiple actions to clean things up and these actions have to be
properly sequenced. All other processors have to stop referencing this
cache before it can be destroyed. I think failing if someone does
something strange like doing cache destruction with a NULL pointer is
valuable.
On Tue, 9 Jun 2015 20:11:25 -0500 (CDT) Christoph Lameter <[email protected]> wrote:
> On Tue, 9 Jun 2015, Andrew Morton wrote:
>
> > Well I like it, even though it's going to cause a zillion little cleanup
> > patches.
> >
> > checkpatch already has a "kfree(NULL) is safe and this check is
> > probably not required" test so I guess Joe will need to get busy ;)
> >
> > I'll park these patches until after 4.1 is released - it's getting to
> > that time...
>
> Why do this at all?
For the third time: because there are approx 200 callsites which are
already doing it.
> I understand that kfree/kmem_cache_free can take a
> null pointer but this is the destruction of a cache and it usually
> requires multiple actions to clean things up and these actions have to be
> properly sequenced. All other processors have to stop referencing this
> cache before it can be destroyed. I think failing if someone does
> something strange like doing cache destruction with a NULL pointer is
> valuable.
More than half of the kmem_cache_destroy() callsites are declining that
value by open-coding the NULL test. That's reality and we should recognize
it.
On Tue, 9 Jun 2015, Andrew Morton wrote:
> > Why do this at all?
>
> For the third time: because there are approx 200 callsites which are
> already doing it.
Did some grepping and I did see some call sites that do this but the
majority has to do other processing as well.
200 call sites? Do we have that many uses of caches? Typical prod system
have ~190 caches active and the merging brings that down to half of that.
> More than half of the kmem_cache_destroy() callsites are declining that
> value by open-coding the NULL test. That's reality and we should recognize
> it.
Well that may just indicate that we need to have a look at those
callsites and the reason there to use a special cache at all. If the cache
is just something that kmalloc can provide then why create a special
cache. On the other hand if something special needs to be accomplished
then it would make sense to have special processing on kmem_cache_destroy.
On (06/09/15 20:11), Christoph Lameter wrote:
> On Tue, 9 Jun 2015, Andrew Morton wrote:
>
> > Well I like it, even though it's going to cause a zillion little cleanup
> > patches.
> >
> > checkpatch already has a "kfree(NULL) is safe and this check is
> > probably not required" test so I guess Joe will need to get busy ;)
> >
> > I'll park these patches until after 4.1 is released - it's getting to
> > that time...
>
> Why do this at all?
this makes things less fragile.
> I understand that kfree/kmem_cache_free can take a
> null pointer but this is the destruction of a cache and it usually
> requires multiple actions to clean things up and these actions have to be
> properly sequenced. All other processors have to stop referencing this
> cache before it can be destroyed.
>I think failing
well, it's not just `failing', it's a NULL pointer deref.
> if someone does something strange like doing cache destruction with a
> NULL pointer is valuable.
>
a missing check is not `something strange'. it's just happening.
(a very quick google search)
http://help.lockergnome.com/linux/PATCH-dlm-NULL-dereference-failure-kmem_cache_create--ftopict555436.html
http://linux-kernel.2935.n7.nabble.com/PATCH-2-6-30-rc6-Remove-kmem-cache-destroy-in-s3c24xx-dma-init-td460417.html
etc.
-ss
On Tue, 9 Jun 2015 21:00:58 -0500 (CDT) Christoph Lameter <[email protected]> wrote:
> On Tue, 9 Jun 2015, Andrew Morton wrote:
>
> > > Why do this at all?
> >
> > For the third time: because there are approx 200 callsites which are
> > already doing it.
>
> Did some grepping and I did see some call sites that do this but the
> majority has to do other processing as well.
>
> 200 call sites? Do we have that many uses of caches? Typical prod system
> have ~190 caches active and the merging brings that down to half of that.
I didn't try terribly hard.
z:/usr/src/linux-4.1-rc7> grep -r -C1 kmem_cache_destroy . | grep "if [(]" | wc -l
158
It's a lot, anyway.
> > More than half of the kmem_cache_destroy() callsites are declining that
> > value by open-coding the NULL test. That's reality and we should recognize
> > it.
>
> Well that may just indicate that we need to have a look at those
> callsites and the reason there to use a special cache at all.
This makes no sense. Go look at the code.
drivers/staging/lustre/lustre/llite/super25.c, for example. It's all
in the basic unwind/recover/exit code.
> If the cache
> is just something that kmalloc can provide then why create a special
> cache. On the other hand if something special needs to be accomplished
> then it would make sense to have special processing on kmem_cache_destroy.
This has nothing to do with anything. We're talking about a basic "if
I created this cache then destroy it" operation.
It's a common pattern. mm/ exists to serve client code and as a lot of
client code is doing this, we should move it into mm/ so as to serve
client code better.
Sergey Senozhatsky has modified several destroy functions that can
now be called with NULL values.
- kmem_cache_destroy()
- mempool_destroy()
- dma_pool_destroy()
Update checkpatch to warn when those functions are preceded by an if.
Update checkpatch to --fix all the calls too only when the code style
form is using leading tabs.
from:
if (foo)
<func>(foo);
to:
<func>(foo);
Signed-off-by: Joe Perches <[email protected]>
---
scripts/checkpatch.pl | 35 +++++++++++++++++++++++++++++++----
1 file changed, 31 insertions(+), 4 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 69c4716..2eff013 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4800,10 +4800,37 @@ sub process {
# check for needless "if (<foo>) fn(<foo>)" uses
if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
- my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
- if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
- WARN('NEEDLESS_IF',
- "$1(NULL) is safe and this check is probably not required\n" . $hereprev);
+ my $tested = quotemeta($1);
+ my $expr = '\s*\(\s*' . quotemeta($tested) . '\s*\)\s*;';
+ if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?|(?:kmem_cache|mempool|dma_pool)_destroy)$expr/) {
+ my $func = $1;
+ if (WARN('NEEDLESS_IF',
+ "$func(NULL) is safe and this check is probably not required\n" . $hereprev) &&
+ $fix) {
+ my $do_fix = 1;
+ my $leading_tabs = "";
+ my $new_leading_tabs = "";
+ if ($lines[$linenr - 2] =~ /^\+(\t*)if\s*\(\s*$tested\s*\)\s*$/) {
+ $leading_tabs = $1;
+ } else {
+ $do_fix = 0;
+ print("here1: <$lines[$linenr - 2]>\n")
+ }
+ if ($lines[$linenr - 1] =~ /^\+(\t+)$func\s*\(\s*$tested\s*\)\s*;\s*$/) {
+ $new_leading_tabs = $1;
+ if (length($leading_tabs) + 1 ne length($new_leading_tabs)) {
+ $do_fix = 0;
+ print("here2: <$lines[$linenr - 1]>\n")
+ }
+ } else {
+ $do_fix = 0;
+ print("here3: <$lines[$linenr - 1]>\n")
+ }
+ if ($do_fix) {
+ fix_delete_line($fixlinenr - 1, $prevrawline);
+ $fixed[$fixlinenr] =~ s/^\+$new_leading_tabs/\+$leading_tabs/;
+ }
+ }
}
}
On Tue, 2015-06-09 at 19:17 -0700, Andrew Morton wrote:
> On Tue, 9 Jun 2015 21:00:58 -0500 (CDT) Christoph Lameter <[email protected]> wrote:
> > On Tue, 9 Jun 2015, Andrew Morton wrote:
> > > > Why do this at all?
> > Did some grepping and I did see some call sites that do this but the
> > majority has to do other processing as well.
> >
> > 200 call sites? Do we have that many uses of caches? Typical prod system
> > have ~190 caches active and the merging brings that down to half of that.
> I didn't try terribly hard.
> z:/usr/src/linux-4.1-rc7> grep -r -C1 kmem_cache_destroy . | grep "if [(]" | wc -l
> 158
>
> It's a lot, anyway.
Yeah.
$ git grep -E -B1 -w "(kmem_cache|mempool|dma_pool)_destroy" *| \
grep -P "\bif\s*\(" | wc -l
268
> > Well I like it, even though it's going to cause a zillion little cleanup
> > patches.
Actually only at most 87. There are some functions that look quite a bit
nicer with the change, like:
void jffs2_destroy_slab_caches(void)
{
- if(full_dnode_slab)
- kmem_cache_destroy(full_dnode_slab);
- if(raw_dirent_slab)
- kmem_cache_destroy(raw_dirent_slab);
- if(raw_inode_slab)
- kmem_cache_destroy(raw_inode_slab);
- if(tmp_dnode_info_slab)
- kmem_cache_destroy(tmp_dnode_info_slab);
- if(raw_node_ref_slab)
- kmem_cache_destroy(raw_node_ref_slab);
- if(node_frag_slab)
- kmem_cache_destroy(node_frag_slab);
- if(inode_cache_slab)
- kmem_cache_destroy(inode_cache_slab);
+ kmem_cache_destroy(full_dnode_slab);
+ kmem_cache_destroy(raw_dirent_slab);
+ kmem_cache_destroy(raw_inode_slab);
+ kmem_cache_destroy(tmp_dnode_info_slab);
+ kmem_cache_destroy(raw_node_ref_slab);
+ kmem_cache_destroy(node_frag_slab);
+ kmem_cache_destroy(inode_cache_slab);
#ifdef CONFIG_JFFS2_FS_XATTR
- if (xattr_datum_cache)
- kmem_cache_destroy(xattr_datum_cache);
- if (xattr_ref_cache)
- kmem_cache_destroy(xattr_ref_cache);
+ kmem_cache_destroy(xattr_datum_cache);
+ kmem_cache_destroy(xattr_ref_cache);
#endif
}
I can try to check and submit the patches.
julia
Sergey Senozhatsky has modified several destroy functions that can
now be called with NULL values.
- kmem_cache_destroy()
- mempool_destroy()
- dma_pool_destroy()
Update checkpatch to warn when those functions are preceded by an if.
Update checkpatch to --fix all the calls too only when the code style
form is using leading tabs.
from:
if (foo)
<func>(foo);
to:
<func>(foo);
Signed-off-by: Joe Perches <[email protected]>
---
V2: Remove useless debugging print messages and multiple quotemetas
scripts/checkpatch.pl | 32 ++++++++++++++++++++++++++++----
1 file changed, 28 insertions(+), 4 deletions(-)
diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 69c4716..87d3bf1aa 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -4800,10 +4800,34 @@ sub process {
# check for needless "if (<foo>) fn(<foo>)" uses
if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
- my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
- if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
- WARN('NEEDLESS_IF',
- "$1(NULL) is safe and this check is probably not required\n" . $hereprev);
+ my $tested = quotemeta($1);
+ my $expr = '\s*\(\s*' . $tested . '\s*\)\s*;';
+ if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?|(?:kmem_cache|mempool|dma_pool)_destroy)$expr/) {
+ my $func = $1;
+ if (WARN('NEEDLESS_IF',
+ "$func(NULL) is safe and this check is probably not required\n" . $hereprev) &&
+ $fix) {
+ my $do_fix = 1;
+ my $leading_tabs = "";
+ my $new_leading_tabs = "";
+ if ($lines[$linenr - 2] =~ /^\+(\t*)if\s*\(\s*$tested\s*\)\s*$/) {
+ $leading_tabs = $1;
+ } else {
+ $do_fix = 0;
+ }
+ if ($lines[$linenr - 1] =~ /^\+(\t+)$func\s*\(\s*$tested\s*\)\s*;\s*$/) {
+ $new_leading_tabs = $1;
+ if (length($leading_tabs) + 1 ne length($new_leading_tabs)) {
+ $do_fix = 0;
+ }
+ } else {
+ $do_fix = 0;
+ }
+ if ($do_fix) {
+ fix_delete_line($fixlinenr - 1, $prevrawline);
+ $fixed[$fixlinenr] =~ s/^\+$new_leading_tabs/\+$leading_tabs/;
+ }
+ }
}
}
On (06/10/15 07:46), Julia Lawall wrote:
> > > Well I like it, even though it's going to cause a zillion little cleanup
> > > patches.
>
> Actually only at most 87. There are some functions that look quite a bit
> nicer with the change, like:
>
> void jffs2_destroy_slab_caches(void)
> {
> - if(full_dnode_slab)
> - kmem_cache_destroy(full_dnode_slab);
> - if(raw_dirent_slab)
> - kmem_cache_destroy(raw_dirent_slab);
> - if(raw_inode_slab)
> - kmem_cache_destroy(raw_inode_slab);
> - if(tmp_dnode_info_slab)
> - kmem_cache_destroy(tmp_dnode_info_slab);
> - if(raw_node_ref_slab)
> - kmem_cache_destroy(raw_node_ref_slab);
> - if(node_frag_slab)
> - kmem_cache_destroy(node_frag_slab);
> - if(inode_cache_slab)
> - kmem_cache_destroy(inode_cache_slab);
> + kmem_cache_destroy(full_dnode_slab);
> + kmem_cache_destroy(raw_dirent_slab);
> + kmem_cache_destroy(raw_inode_slab);
> + kmem_cache_destroy(tmp_dnode_info_slab);
> + kmem_cache_destroy(raw_node_ref_slab);
> + kmem_cache_destroy(node_frag_slab);
> + kmem_cache_destroy(inode_cache_slab);
> #ifdef CONFIG_JFFS2_FS_XATTR
> - if (xattr_datum_cache)
> - kmem_cache_destroy(xattr_datum_cache);
> - if (xattr_ref_cache)
> - kmem_cache_destroy(xattr_ref_cache);
> + kmem_cache_destroy(xattr_datum_cache);
> + kmem_cache_destroy(xattr_ref_cache);
> #endif
> }
>
and some goto labels can go away either. like
[..]
err_percpu_counter_init:
kmem_cache_destroy(sctp_chunk_cachep);
err_chunk_cachep:
kmem_cache_destroy(sctp_bucket_cachep);
[..]
and others.
-ss
On Wed, 10 Jun 2015, Sergey Senozhatsky wrote:
> On (06/10/15 07:46), Julia Lawall wrote:
> > > > Well I like it, even though it's going to cause a zillion little cleanup
> > > > patches.
> >
> > Actually only at most 87. There are some functions that look quite a bit
> > nicer with the change, like:
> >
> > void jffs2_destroy_slab_caches(void)
> > {
> > - if(full_dnode_slab)
> > - kmem_cache_destroy(full_dnode_slab);
> > - if(raw_dirent_slab)
> > - kmem_cache_destroy(raw_dirent_slab);
> > - if(raw_inode_slab)
> > - kmem_cache_destroy(raw_inode_slab);
> > - if(tmp_dnode_info_slab)
> > - kmem_cache_destroy(tmp_dnode_info_slab);
> > - if(raw_node_ref_slab)
> > - kmem_cache_destroy(raw_node_ref_slab);
> > - if(node_frag_slab)
> > - kmem_cache_destroy(node_frag_slab);
> > - if(inode_cache_slab)
> > - kmem_cache_destroy(inode_cache_slab);
> > + kmem_cache_destroy(full_dnode_slab);
> > + kmem_cache_destroy(raw_dirent_slab);
> > + kmem_cache_destroy(raw_inode_slab);
> > + kmem_cache_destroy(tmp_dnode_info_slab);
> > + kmem_cache_destroy(raw_node_ref_slab);
> > + kmem_cache_destroy(node_frag_slab);
> > + kmem_cache_destroy(inode_cache_slab);
> > #ifdef CONFIG_JFFS2_FS_XATTR
> > - if (xattr_datum_cache)
> > - kmem_cache_destroy(xattr_datum_cache);
> > - if (xattr_ref_cache)
> > - kmem_cache_destroy(xattr_ref_cache);
> > + kmem_cache_destroy(xattr_datum_cache);
> > + kmem_cache_destroy(xattr_ref_cache);
> > #endif
> > }
> >
>
> and some goto labels can go away either. like
>
> [..]
>
> err_percpu_counter_init:
> kmem_cache_destroy(sctp_chunk_cachep);
> err_chunk_cachep:
> kmem_cache_destroy(sctp_bucket_cachep);
>
> [..]
>
> and others.
This I find much less appealing. The labels make clear what is needed at
what point. At least from a tool point of view, this is useful
infomation.
julia
On (06/10/15 08:44), Julia Lawall wrote:
> >
> > [..]
> >
> > err_percpu_counter_init:
> > kmem_cache_destroy(sctp_chunk_cachep);
> > err_chunk_cachep:
> > kmem_cache_destroy(sctp_bucket_cachep);
> >
> > [..]
> >
> > and others.
>
> This I find much less appealing. The labels make clear what is needed
>
hm, agree.
-ss
On (06/09/15 22:52), Joe Perches wrote:
> Sergey Senozhatsky has modified several destroy functions that can
> now be called with NULL values.
>
> - kmem_cache_destroy()
> - mempool_destroy()
> - dma_pool_destroy()
>
> Update checkpatch to warn when those functions are preceded by an if.
>
> Update checkpatch to --fix all the calls too only when the code style
> form is using leading tabs.
>
> from:
> if (foo)
> <func>(foo);
> to:
> <func>(foo);
>
> Signed-off-by: Joe Perches <[email protected]>
nice.
works fine to me. you can add
Tested-by: Sergey Senozhatsky <[email protected]>
if needed.
-ss
> ---
> V2: Remove useless debugging print messages and multiple quotemetas
>
> scripts/checkpatch.pl | 32 ++++++++++++++++++++++++++++----
> 1 file changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 69c4716..87d3bf1aa 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -4800,10 +4800,34 @@ sub process {
>
> # check for needless "if (<foo>) fn(<foo>)" uses
> if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
> - my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
> - if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
> - WARN('NEEDLESS_IF',
> - "$1(NULL) is safe and this check is probably not required\n" . $hereprev);
> + my $tested = quotemeta($1);
> + my $expr = '\s*\(\s*' . $tested . '\s*\)\s*;';
> + if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?|(?:kmem_cache|mempool|dma_pool)_destroy)$expr/) {
> + my $func = $1;
> + if (WARN('NEEDLESS_IF',
> + "$func(NULL) is safe and this check is probably not required\n" . $hereprev) &&
> + $fix) {
> + my $do_fix = 1;
> + my $leading_tabs = "";
> + my $new_leading_tabs = "";
> + if ($lines[$linenr - 2] =~ /^\+(\t*)if\s*\(\s*$tested\s*\)\s*$/) {
> + $leading_tabs = $1;
> + } else {
> + $do_fix = 0;
> + }
> + if ($lines[$linenr - 1] =~ /^\+(\t+)$func\s*\(\s*$tested\s*\)\s*;\s*$/) {
> + $new_leading_tabs = $1;
> + if (length($leading_tabs) + 1 ne length($new_leading_tabs)) {
> + $do_fix = 0;
> + }
> + } else {
> + $do_fix = 0;
> + }
> + if ($do_fix) {
> + fix_delete_line($fixlinenr - 1, $prevrawline);
> + $fixed[$fixlinenr] =~ s/^\+$new_leading_tabs/\+$leading_tabs/;
> + }
> + }
> }
> }
>
>
>
On Tue, Jun 9, 2015 at 8:04 AM, Sergey Senozhatsky
<[email protected]> wrote:
> zpool_destroy_pool() does not tolerate a NULL zpool pointer
> argument and performs a NULL-pointer dereference. Although
> there is only one zpool_destroy_pool() user (as of 4.1),
> still update it to be coherent with the corresponding
> destroy() functions of the remainig pool-allocators (slab,
> mempool, etc.), which now allow NULL pool-pointers.
>
> For consistency, tweak zpool_destroy_pool() and NULL-check the
> pointer there.
>
> Proposed by Andrew Morton.
>
> Signed-off-by: Sergey Senozhatsky <[email protected]>
> Reported-by: Andrew Morton <[email protected]>
> LKML-reference: https://lkml.org/lkml/2015/6/8/583
Acked-by: Dan Streetman <[email protected]>
> ---
> mm/zpool.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/mm/zpool.c b/mm/zpool.c
> index bacdab6..2f59b90 100644
> --- a/mm/zpool.c
> +++ b/mm/zpool.c
> @@ -202,6 +202,9 @@ struct zpool *zpool_create_pool(char *type, char *name, gfp_t gfp,
> */
> void zpool_destroy_pool(struct zpool *zpool)
> {
> + if (unlikely(!zpool))
> + return;
> +
> pr_info("destroying pool type %s\n", zpool->type);
>
> spin_lock(&pools_lock);
> --
> 2.4.3.368.g7974889
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
On (06/10/15 16:59), Dan Streetman wrote:
> On Tue, Jun 9, 2015 at 8:04 AM, Sergey Senozhatsky
> <[email protected]> wrote:
> > zpool_destroy_pool() does not tolerate a NULL zpool pointer
> > argument and performs a NULL-pointer dereference. Although
> > there is only one zpool_destroy_pool() user (as of 4.1),
> > still update it to be coherent with the corresponding
> > destroy() functions of the remainig pool-allocators (slab,
> > mempool, etc.), which now allow NULL pool-pointers.
> >
> > For consistency, tweak zpool_destroy_pool() and NULL-check the
> > pointer there.
> >
> > Proposed by Andrew Morton.
> >
> > Signed-off-by: Sergey Senozhatsky <[email protected]>
> > Reported-by: Andrew Morton <[email protected]>
> > LKML-reference: https://lkml.org/lkml/2015/6/8/583
>
> Acked-by: Dan Streetman <[email protected]>
Thanks.
Shall we ask Joe to add zpool_destroy_pool() to the
"$func(NULL) is safe and this check is probably not required" list?
-ss
> > ---
> > mm/zpool.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/mm/zpool.c b/mm/zpool.c
> > index bacdab6..2f59b90 100644
> > --- a/mm/zpool.c
> > +++ b/mm/zpool.c
> > @@ -202,6 +202,9 @@ struct zpool *zpool_create_pool(char *type, char *name, gfp_t gfp,
> > */
> > void zpool_destroy_pool(struct zpool *zpool)
> > {
> > + if (unlikely(!zpool))
> > + return;
> > +
> > pr_info("destroying pool type %s\n", zpool->type);
> >
> > spin_lock(&pools_lock);
> > --
> > 2.4.3.368.g7974889
> >
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > the body to [email protected]. For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
>
On Thu, 2015-06-11 at 08:58 +0900, Sergey Senozhatsky wrote:
> On (06/10/15 16:59), Dan Streetman wrote:
> > On Tue, Jun 9, 2015 at 8:04 AM, Sergey Senozhatsky
> > <[email protected]> wrote:
> > > zpool_destroy_pool() does not tolerate a NULL zpool pointer
> > > argument and performs a NULL-pointer dereference. Although
> > > there is only one zpool_destroy_pool() user (as of 4.1),
> > > still update it to be coherent with the corresponding
> > > destroy() functions of the remainig pool-allocators (slab,
> > > mempool, etc.), which now allow NULL pool-pointers.
> > >
> > > For consistency, tweak zpool_destroy_pool() and NULL-check the
> > > pointer there.
> > >
> > > Proposed by Andrew Morton.
> > >
> > > Signed-off-by: Sergey Senozhatsky <[email protected]>
> > > Reported-by: Andrew Morton <[email protected]>
> > > LKML-reference: https://lkml.org/lkml/2015/6/8/583
> >
> > Acked-by: Dan Streetman <[email protected]>
>
> Thanks.
>
> Shall we ask Joe to add zpool_destroy_pool() to the
> "$func(NULL) is safe and this check is probably not required" list?
[]
Is it really worth it?
There isn't any use of zpool_destroy_pool preceded by an if
There is one and only one use of zpool_destroy_pool.
On (06/10/15 17:48), Joe Perches wrote:
[..]
> > > > For consistency, tweak zpool_destroy_pool() and NULL-check the
> > > > pointer there.
> > > >
> > > > Proposed by Andrew Morton.
> > > >
> > > > Signed-off-by: Sergey Senozhatsky <[email protected]>
> > > > Reported-by: Andrew Morton <[email protected]>
> > > > LKML-reference: https://lkml.org/lkml/2015/6/8/583
> > >
> > > Acked-by: Dan Streetman <[email protected]>
> >
> > Thanks.
> >
> > Shall we ask Joe to add zpool_destroy_pool() to the
> > "$func(NULL) is safe and this check is probably not required" list?
>
> []
>
> Is it really worth it?
>
> There isn't any use of zpool_destroy_pool preceded by an if
> There is one and only one use of zpool_destroy_pool.
>
Yes, that's why I asked. I don't think that zpool_destroy_pool()
will gain any significant amount of users soon (well, who knows),
so I'm fine with keeping it out of checkpatch checks. Just checked
your opinion.
-ss
On Wed, Jun 10, 2015 at 8:59 PM, Sergey Senozhatsky
<[email protected]> wrote:
> On (06/10/15 17:48), Joe Perches wrote:
> [..]
>> > > > For consistency, tweak zpool_destroy_pool() and NULL-check the
>> > > > pointer there.
>> > > >
>> > > > Proposed by Andrew Morton.
>> > > >
>> > > > Signed-off-by: Sergey Senozhatsky <[email protected]>
>> > > > Reported-by: Andrew Morton <[email protected]>
>> > > > LKML-reference: https://lkml.org/lkml/2015/6/8/583
>> > >
>> > > Acked-by: Dan Streetman <[email protected]>
>> >
>> > Thanks.
>> >
>> > Shall we ask Joe to add zpool_destroy_pool() to the
>> > "$func(NULL) is safe and this check is probably not required" list?
>>
>> []
>>
>> Is it really worth it?
>>
>> There isn't any use of zpool_destroy_pool preceded by an if
>> There is one and only one use of zpool_destroy_pool.
>>
>
> Yes, that's why I asked. I don't think that zpool_destroy_pool()
> will gain any significant amount of users soon (well, who knows),
> so I'm fine with keeping it out of checkpatch checks. Just checked
> your opinion.
I really doubt if zpool will be used by anyone other than zswap anytime soon.
>
> -ss
On Tue, 9 Jun 2015, Joe Perches wrote:
> Sergey Senozhatsky has modified several destroy functions that can
> now be called with NULL values.
>
> - kmem_cache_destroy()
> - mempool_destroy()
> - dma_pool_destroy()
I don't actually see any null test in the definition of dma_pool_destroy,
in the linux-next 54896f27dd5 (20150610). So I guess it would be
premature to send patches to remove the null tests.
julia
> Update checkpatch to warn when those functions are preceded by an if.
>
> Update checkpatch to --fix all the calls too only when the code style
> form is using leading tabs.
>
> from:
> if (foo)
> <func>(foo);
> to:
> <func>(foo);
>
> Signed-off-by: Joe Perches <[email protected]>
> ---
> V2: Remove useless debugging print messages and multiple quotemetas
>
> scripts/checkpatch.pl | 32 ++++++++++++++++++++++++++++----
> 1 file changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 69c4716..87d3bf1aa 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -4800,10 +4800,34 @@ sub process {
>
> # check for needless "if (<foo>) fn(<foo>)" uses
> if ($prevline =~ /\bif\s*\(\s*($Lval)\s*\)/) {
> - my $expr = '\s*\(\s*' . quotemeta($1) . '\s*\)\s*;';
> - if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?)$expr/) {
> - WARN('NEEDLESS_IF',
> - "$1(NULL) is safe and this check is probably not required\n" . $hereprev);
> + my $tested = quotemeta($1);
> + my $expr = '\s*\(\s*' . $tested . '\s*\)\s*;';
> + if ($line =~ /\b(kfree|usb_free_urb|debugfs_remove(?:_recursive)?|(?:kmem_cache|mempool|dma_pool)_destroy)$expr/) {
> + my $func = $1;
> + if (WARN('NEEDLESS_IF',
> + "$func(NULL) is safe and this check is probably not required\n" . $hereprev) &&
> + $fix) {
> + my $do_fix = 1;
> + my $leading_tabs = "";
> + my $new_leading_tabs = "";
> + if ($lines[$linenr - 2] =~ /^\+(\t*)if\s*\(\s*$tested\s*\)\s*$/) {
> + $leading_tabs = $1;
> + } else {
> + $do_fix = 0;
> + }
> + if ($lines[$linenr - 1] =~ /^\+(\t+)$func\s*\(\s*$tested\s*\)\s*;\s*$/) {
> + $new_leading_tabs = $1;
> + if (length($leading_tabs) + 1 ne length($new_leading_tabs)) {
> + $do_fix = 0;
> + }
> + } else {
> + $do_fix = 0;
> + }
> + if ($do_fix) {
> + fix_delete_line($fixlinenr - 1, $prevrawline);
> + $fixed[$fixlinenr] =~ s/^\+$new_leading_tabs/\+$leading_tabs/;
> + }
> + }
> }
> }
>
>
>
>
On (06/11/15 11:41), Julia Lawall wrote:
> On Tue, 9 Jun 2015, Joe Perches wrote:
>
> > Sergey Senozhatsky has modified several destroy functions that can
> > now be called with NULL values.
> >
> > - kmem_cache_destroy()
> > - mempool_destroy()
> > - dma_pool_destroy()
>
> I don't actually see any null test in the definition of dma_pool_destroy,
> in the linux-next 54896f27dd5 (20150610). So I guess it would be
> premature to send patches to remove the null tests.
>
yes,
Andrew Morton:
: I'll park these patches until after 4.1 is released - it's getting to
: that time...
-ss
On Thu, 11 Jun 2015, Sergey Senozhatsky wrote:
> On (06/11/15 11:41), Julia Lawall wrote:
> > On Tue, 9 Jun 2015, Joe Perches wrote:
> >
> > > Sergey Senozhatsky has modified several destroy functions that can
> > > now be called with NULL values.
> > >
> > > - kmem_cache_destroy()
> > > - mempool_destroy()
> > > - dma_pool_destroy()
> >
> > I don't actually see any null test in the definition of dma_pool_destroy,
> > in the linux-next 54896f27dd5 (20150610). So I guess it would be
> > premature to send patches to remove the null tests.
> >
>
> yes,
>
> Andrew Morton:
> : I'll park these patches until after 4.1 is released - it's getting to
> : that time...
OK, thanks,
julia
On Tue, 9 Jun 2015, Andrew Morton wrote:
> > > More than half of the kmem_cache_destroy() callsites are declining that
> > > value by open-coding the NULL test. That's reality and we should recognize
> > > it.
> >
> > Well that may just indicate that we need to have a look at those
> > callsites and the reason there to use a special cache at all.
>
> This makes no sense. Go look at the code.
> drivers/staging/lustre/lustre/llite/super25.c, for example. It's all
> in the basic unwind/recover/exit code.
That is screwed up code. I'd do that without the checks simply with a
series of kmem_cache_destroys().
> > If the cache
> > is just something that kmalloc can provide then why create a special
> > cache. On the other hand if something special needs to be accomplished
> > then it would make sense to have special processing on kmem_cache_destroy.
>
> This has nothing to do with anything. We're talking about a basic "if
> I created this cache then destroy it" operation.
As you see in this code snipped you cannot continue if a certain operation
during setup fails. At that point it is known which caches exist and
therefore kmem_cache_destroy() can be called without the checks.
> It's a common pattern. mm/ exists to serve client code and as a lot of
> client code is doing this, we should move it into mm/ so as to serve
> client code better.
Doing this seems to encourage sloppy coding practices.
On Thu, 11 Jun 2015 12:26:11 -0500 (CDT) Christoph Lameter <[email protected]> wrote:
> On Tue, 9 Jun 2015, Andrew Morton wrote:
>
> > > > More than half of the kmem_cache_destroy() callsites are declining that
> > > > value by open-coding the NULL test. That's reality and we should recognize
> > > > it.
> > >
> > > Well that may just indicate that we need to have a look at those
> > > callsites and the reason there to use a special cache at all.
> >
> > This makes no sense. Go look at the code.
> > drivers/staging/lustre/lustre/llite/super25.c, for example. It's all
> > in the basic unwind/recover/exit code.
>
> That is screwed up code. I'd do that without the checks simply with a
> series of kmem_cache_destroys().
So go and review some of the many other callers which do this.
On Tue, 9 Jun 2015, Sergey Senozhatsky wrote:
> kmem_cache_destroy() does not tolerate a NULL kmem_cache pointer
> argument and performs a NULL-pointer dereference. This requires
> additional attention and effort from developers/reviewers and
> forces all kmem_cache_destroy() callers (200+ as of 4.1) to do
> a NULL check
>
> if (cache)
> kmem_cache_destroy(cache);
>
> Or, otherwise, be invalid kmem_cache_destroy() users.
>
> Tweak kmem_cache_destroy() and NULL-check the pointer there.
>
> Proposed by Andrew Morton.
>
> Signed-off-by: Sergey Senozhatsky <[email protected]>
> Reported-by: Andrew Morton <[email protected]>
> LKML-reference: https://lkml.org/lkml/2015/6/8/583
Acked-by: David Rientjes <[email protected]>
kmem_cache_destroy() isn't a fastpath, this is long overdue. Now where's
the patch to remove the NULL checks from the callers? ;)
On Tue, 9 Jun 2015, Sergey Senozhatsky wrote:
> mempool_destroy() does not tolerate a NULL mempool_t pointer
> argument and performs a NULL-pointer dereference. This requires
> additional attention and effort from developers/reviewers and
> forces all mempool_destroy() callers to do a NULL check
>
> if (pool)
> mempool_destroy(pool);
>
> Or, otherwise, be invalid mempool_destroy() users.
>
> Tweak mempool_destroy() and NULL-check the pointer there.
>
> Proposed by Andrew Morton.
>
> Signed-off-by: Sergey Senozhatsky <[email protected]>
> Reported-by: Andrew Morton <[email protected]>
> LKML-reference: https://lkml.org/lkml/2015/6/8/583
Acked-by: David Rientjes <[email protected]>
I like how your patch series is enabling us to remove many lines from the
source code. But doing s/Reported-by/Suggested-by/ can also make your
changelog two lines shorter ;)
On Tue, 9 Jun 2015, Sergey Senozhatsky wrote:
> dma_pool_destroy() does not tolerate a NULL dma_pool pointer
> argument and performs a NULL-pointer dereference. This requires
> additional attention and effort from developers/reviewers and
> forces all dma_pool_destroy() callers to do a NULL check
>
> if (pool)
> dma_pool_destroy(pool);
>
> Or, otherwise, be invalid dma_pool_destroy() users.
>
> Tweak dma_pool_destroy() and NULL-check the pointer there.
>
> Proposed by Andrew Morton.
>
> Signed-off-by: Sergey Senozhatsky <[email protected]>
> Reported-by: Andrew Morton <[email protected]>
> LKML-reference: https://lkml.org/lkml/2015/6/8/583
Acked-by: David Rientjes <[email protected]>
On (06/17/15 16:14), David Rientjes wrote:
[..]
> >
> > Signed-off-by: Sergey Senozhatsky <[email protected]>
> > Reported-by: Andrew Morton <[email protected]>
> > LKML-reference: https://lkml.org/lkml/2015/6/8/583
>
> Acked-by: David Rientjes <[email protected]>
>
> kmem_cache_destroy() isn't a fastpath, this is long overdue. Now where's
> the patch to remove the NULL checks from the callers? ;)
>
Thanks.
Yes, Julia Lawall (Cc'd) already has a patch set ready for submission.
-ss
On (06/17/15 16:21), David Rientjes wrote:
[..]
> > Tweak mempool_destroy() and NULL-check the pointer there.
> >
> > Proposed by Andrew Morton.
> >
> > Signed-off-by: Sergey Senozhatsky <[email protected]>
> > Reported-by: Andrew Morton <[email protected]>
> > LKML-reference: https://lkml.org/lkml/2015/6/8/583
>
> Acked-by: David Rientjes <[email protected]>
>
> I like how your patch series is enabling us to remove many lines from the
> source code. But doing s/Reported-by/Suggested-by/ can also make your
> changelog two lines shorter ;)
>
Thanks.
Oh, s/Reported-by/Suggested-by/ looks better, indeed.
-ss
On Thu, 18 Jun 2015, Sergey Senozhatsky wrote:
> On (06/17/15 16:14), David Rientjes wrote:
> [..]
> > >
> > > Signed-off-by: Sergey Senozhatsky <[email protected]>
> > > Reported-by: Andrew Morton <[email protected]>
> > > LKML-reference: https://lkml.org/lkml/2015/6/8/583
> >
> > Acked-by: David Rientjes <[email protected]>
> >
> > kmem_cache_destroy() isn't a fastpath, this is long overdue. Now where's
> > the patch to remove the NULL checks from the callers? ;)
> >
>
> Thanks.
>
> Yes, Julia Lawall (Cc'd) already has a patch set ready for submission.
I'll refresh it and send it shortly.
julia
On Thu, 18 Jun 2015, Sergey Senozhatsky wrote:
> On (06/17/15 16:14), David Rientjes wrote:
> [..]
> > >
> > > Signed-off-by: Sergey Senozhatsky <[email protected]>
> > > Reported-by: Andrew Morton <[email protected]>
> > > LKML-reference: https://lkml.org/lkml/2015/6/8/583
> >
> > Acked-by: David Rientjes <[email protected]>
> >
> > kmem_cache_destroy() isn't a fastpath, this is long overdue. Now where's
> > the patch to remove the NULL checks from the callers? ;)
> >
>
> Thanks.
>
> Yes, Julia Lawall (Cc'd) already has a patch set ready for submission.
The patch for making these functions able to tolerate NULL doesn't seem to
be in linux-next yet, so I will wait until it appears.
julia
On Tue, 09 Jun 2015 22:52:29 -0700 Joe Perches <[email protected]> wrote:
> Sergey Senozhatsky has modified several destroy functions that can
> now be called with NULL values.
>
> - kmem_cache_destroy()
> - mempool_destroy()
> - dma_pool_destroy()
>
> Update checkpatch to warn when those functions are preceded by an if.
>
> Update checkpatch to --fix all the calls too only when the code style
> form is using leading tabs.
>
> from:
> if (foo)
> <func>(foo);
> to:
> <func>(foo);
There's also zpool_destroy_pool() and zs_destroy_pool(). Did we decide
they're not worth bothering about?
On (07/14/15 16:03), Andrew Morton wrote:
> > Sergey Senozhatsky has modified several destroy functions that can
> > now be called with NULL values.
> >
> > - kmem_cache_destroy()
> > - mempool_destroy()
> > - dma_pool_destroy()
> >
> > Update checkpatch to warn when those functions are preceded by an if.
> >
> > Update checkpatch to --fix all the calls too only when the code style
> > form is using leading tabs.
> >
> > from:
> > if (foo)
> > <func>(foo);
> > to:
> > <func>(foo);
>
> There's also zpool_destroy_pool() and zs_destroy_pool(). Did we decide
> they're not worth bothering about?
Correct. Those two are very unlikely will see any significant number
of users so, I think, we can drop the patches that touch zspool and
zsmalloc destructors.
-ss
On (06/20/15 18:25), Julia Lawall wrote:
> > On (06/17/15 16:14), David Rientjes wrote:
> > [..]
> > > >
> > > > Signed-off-by: Sergey Senozhatsky <[email protected]>
> > > > Reported-by: Andrew Morton <[email protected]>
> > > > LKML-reference: https://lkml.org/lkml/2015/6/8/583
> > >
> > > Acked-by: David Rientjes <[email protected]>
> > >
> > > kmem_cache_destroy() isn't a fastpath, this is long overdue. Now where's
> > > the patch to remove the NULL checks from the callers? ;)
> > >
> >
> > Thanks.
> >
> > Yes, Julia Lawall (Cc'd) already has a patch set ready for submission.
>
> The patch for making these functions able to tolerate NULL doesn't seem to
> be in linux-next yet, so I will wait until it appears.
Hello Julia,
The patches are in -next now.
mm/dmapool: 8bf49946ed8fa01a0b5e7d0de94655c072525344
mm/mempool: eb54bc8469e2977bcef4e284d24cbf3578ce9cd9
mm/slab_common: e88672f95907c14cf8ab2cce592c41bbb9cefc5f
-ss
On (06/19/15 08:50), Julia Lawall wrote:
> On Thu, 18 Jun 2015, Sergey Senozhatsky wrote:
>
> > On (06/17/15 16:14), David Rientjes wrote:
> > [..]
> > > >
> > > > Signed-off-by: Sergey Senozhatsky <[email protected]>
> > > > Reported-by: Andrew Morton <[email protected]>
> > > > LKML-reference: https://lkml.org/lkml/2015/6/8/583
> > >
> > > Acked-by: David Rientjes <[email protected]>
> > >
> > > kmem_cache_destroy() isn't a fastpath, this is long overdue. Now where's
> > > the patch to remove the NULL checks from the callers? ;)
> > >
> >
> > Thanks.
> >
> > Yes, Julia Lawall (Cc'd) already has a patch set ready for submission.
>
> I'll refresh it and send it shortly.
>
I'll re-up this thread.
Julia, do you want to wait until these 3 patches will be merged to
Linus's tree (just to be on a safe side, so someone's tree (out of sync
with linux-next) will not go crazy)?
-ss
On Thu, 6 Aug 2015, Sergey Senozhatsky wrote:
> On (06/19/15 08:50), Julia Lawall wrote:
> > On Thu, 18 Jun 2015, Sergey Senozhatsky wrote:
> >
> > > On (06/17/15 16:14), David Rientjes wrote:
> > > [..]
> > > > >
> > > > > Signed-off-by: Sergey Senozhatsky <[email protected]>
> > > > > Reported-by: Andrew Morton <[email protected]>
> > > > > LKML-reference: https://lkml.org/lkml/2015/6/8/583
> > > >
> > > > Acked-by: David Rientjes <[email protected]>
> > > >
> > > > kmem_cache_destroy() isn't a fastpath, this is long overdue. Now where's
> > > > the patch to remove the NULL checks from the callers? ;)
> > > >
> > >
> > > Thanks.
> > >
> > > Yes, Julia Lawall (Cc'd) already has a patch set ready for submission.
> >
> > I'll refresh it and send it shortly.
> >
>
> I'll re-up this thread.
>
> Julia, do you want to wait until these 3 patches will be merged to
> Linus's tree (just to be on a safe side, so someone's tree (out of sync
> with linux-next) will not go crazy)?
I think it would be safer. Code may crash if the test is removed before
the function can tolerate it.
julia
On (08/06/15 16:27), Julia Lawall wrote:
[..]
> > Julia, do you want to wait until these 3 patches will be merged to
> > Linus's tree (just to be on a safe side, so someone's tree (out of sync
> > with linux-next) will not go crazy)?
>
> I think it would be safer. Code may crash if the test is removed before
> the function can tolerate it.
>
Agree. I'll re-up this thread later (once 4.3 merge window is closed).
Thank you.
-ss