Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test
On Tue, 17 Jun 2014, Joe Perches wrote:
> On Wed, 2014-06-18 at 07:25 +0200, Julia Lawall wrote:
> >
> > On Tue, 17 Jun 2014, Joe Perches wrote:
> >
> > > (adding Jesper Juhl)
> > >
> > > On Tue, 2014-06-17 at 23:33 +0200, Julia Lawall wrote:
> > > > On Tue, 17 Jun 2014, Joe Perches wrote:
> > > > > On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote:
> > > > > > This patch adds a trivial script warning on
> > > > > >
> > > > > > if(foo)
> > > > > > kfree(foo)
> > > > > >
> > > > > > (based on checkpatch)
> > > > > []
> > > > > > diff --git a/scripts/coccinelle/free/cond_kfree.cocci
> > > > > > b/scripts/coccinelle/free/cond_kfree.cocci
> > > > > []
> > > > > > +* if (E)
> > > > > > +* kfree@p(E);
> > > > >
> > > > > You should probably add all of the unnecessary
> > > > > conditional tests that checkpatch uses:
> > > > >
> > > > > kfree
> > > > > usb_free_urb
> > > > > debugfs_remove
> > > > > debugfs_remove_recursive
> > > >
> > > > Personally, I would prefer that the message encourage the user to
> > > > consider
> > > > whether it is necessary to call these functions with NULL as an argument
> > > > in any case.
> > >
> > > Jesper quite awhile ago wrote:
> > >
> > > https://lkml.org/lkml/2005/10/13/81
> > >
> > > - Since kfree always checks for a NULL argument there's no reason to have
> > > an
> > > additional check prior to calling kfree. It's redundant.
> > > - In many cases gcc produce significantly smaller code without the
> > > redundant
> > > check before the call.
> > > - It's been shown in the past (in discussions on LKML) that it's
> > > generally a
> > > win performance wise to avoid the extra NULL check even though it might
> > > save
> > > a function call. Only when the NULL check avoids the function call in the
> > > vast
> > > majority of cases and the code is in a hot path does it make sense to
> > > have it.
> > > - This patch removes quite a few more source lines than it adds, cutting
> > > down
> > > on the overall number of source lines is generally a good thing.
> > > - This patch reduces the indentation level, which is nice when the kfree
> > > call
> > > is inside some deeply nested construct.
> >
> > What I don't like is:
> >
> > a = kmalloc(...);
> > if (!a) goto out;
> > b = kmalloc(...);
> > if (!b) goto out;
> > c = kmalloc(...);
> > if (!c) goto out;
> > ...
> > out:
> > kfree(a);
> > kfree(b);
> > kfree(c);
> >
> > With a little thought, one could reorganize the code to not call kfree on
> > a null value.
>
> And I think most kernel malloc failures are written like:
>
> a = kmalloc(...);
> if (!a) goto out1;
> b = kmalloc(...);
> if (!b) goto out2;
> c = kmalloc(...)
> if (!c) goto out3;
> ...
> out3: kfree(b);
> out2: kfree(a);
> out1: ...
This is the case for the good code. But not necessarily in staging.
> > Someone who is not familiar with Linux programming style
> > could interpret the feedback as that the above code is perfectly fine.
> > (And perhaps some people do consider that it is perfectly fine).
>
> maybe so.
>
> > On the other hand, in the case:
> >
> > x = NULL;
> > if (complicated_condition)
> > x = kmalloc(...);
> > if (!x) return;
> > y = something(...);
> > if (!y)
> > goto out1;
> > ...
> > out1: kfree(x);
> >
> > I guess it's OK. Mildly unpleasant, but probably the best option given
> > the various tradeoff.
> >
> > In looking at Jesper's patch, I see that another case is:
> >
> > a = kmalloc(...);
> > b = kmalloc(...);
> > if (!a || !b) {
> > kfree(a);
> > kfree(b);
> > }
> >
> > Personally, I would rather see each call have its own error handling code.
> > There is no point to make the second call if the first one fails.
> >
> > When one tries to understand code, the main questions are why is this done
> > here, and why is this not done here. Doing things that are unnecessary
> > introduces confusion in this regard. Perhaps it doesn't matter for
> > kmalloc and kfree because everyone is familiar with them and they are
> > pretty innocuous. But for the more obscure functions, like in my
> > recollection of Markus's patch, I'm not convinced that simply blindly
> > removing all unneeded tests without thinking whether the code could be
> > written in a better way is a good idea.
>
> Blindly applying patches, even those produced by coccinelle,
> let alone mine, is rarely good practice.
Sure. I would just argue for changing the text associated with the
semantic patch a little bit, to suggest considering whether the code can
be reorganized to eliminate the possibility of passing NULL to these
functions in the first place.
julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test
On Tue, 17 Jun 2014, Joe Perches wrote:
On Wed, 2014-06-18 at 07:25 +0200, Julia Lawall wrote:
On Tue, 17 Jun 2014, Joe Perches wrote:
(adding Jesper Juhl)
On Tue, 2014-06-17 at 23:33 +0200, Julia Lawall wrote:
On Tue, 17 Jun 2014, Joe Perches wrote:
On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote:
This patch adds a trivial script warning on
if(foo)
kfree(foo)
(based on checkpatch)
[]
diff --git a/scripts/coccinelle/free/cond_kfree.cocci
b/scripts/coccinelle/free/cond_kfree.cocci
[]
+* if (E)
+* kfree@p(E);
You should probably add all of the unnecessary
conditional tests that checkpatch uses:
kfree
usb_free_urb
debugfs_remove
debugfs_remove_recursive
Personally, I would prefer that the message encourage the user to
consider
whether it is necessary to call these functions with NULL as an argument
in any case.
Jesper quite awhile ago wrote:
https://lkml.org/lkml/2005/10/13/81
- Since kfree always checks for a NULL argument there's no reason to have
an
additional check prior to calling kfree. It's redundant.
- In many cases gcc produce significantly smaller code without the
redundant
check before the call.
- It's been shown in the past (in discussions on LKML) that it's
generally a
win performance wise to avoid the extra NULL check even though it might
save
a function call. Only when the NULL check avoids the function call in the
vast
majority of cases and the code is in a hot path does it make sense to
have it.
- This patch removes quite a few more source lines than it adds, cutting
down
on the overall number of source lines is generally a good thing.
- This patch reduces the indentation level, which is nice when the kfree
call
is inside some deeply nested construct.
What I don't like is:
a = kmalloc(...);
if (!a) goto out;
b = kmalloc(...);
if (!b) goto out;
c = kmalloc(...);
if (!c) goto out;
...
out:
kfree(a);
kfree(b);
kfree(c);
With a little thought, one could reorganize the code to not call kfree on
a null value.
And I think most kernel malloc failures are written like:
a = kmalloc(...);
if (!a) goto out1;
b = kmalloc(...);
if (!b) goto out2;
c = kmalloc(...)
if (!c) goto out3;
...
out3: kfree(b);
out2: kfree(a);
out1: ...
This is the case for the good code. But not necessarily in staging.
Someone who is not familiar with Linux programming style
could interpret the feedback as that the above code is perfectly fine.
(And perhaps some people do consider that it is perfectly fine).
maybe so.
On the other hand, in the case:
x = NULL;
if (complicated_condition)
x = kmalloc(...);
if (!x) return;
y = something(...);
if (!y)
goto out1;
...
out1: kfree(x);
I guess it's OK. Mildly unpleasant, but probably the best option given
the various tradeoff.
In looking at Jesper's patch, I see that another case is:
a = kmalloc(...);
b = kmalloc(...);
if (!a || !b) {
kfree(a);
kfree(b);
}
Personally, I would rather see each call have its own error handling code.
There is no point to make the second call if the first one fails.
When one tries to understand code, the main questions are why is this done
here, and why is this not done here. Doing things that are unnecessary
introduces confusion in this regard. Perhaps it doesn't matter for
kmalloc and kfree because everyone is familiar with them and they are
pretty innocuous. But for the more obscure functions, like in my
recollection of Markus's patch, I'm not convinced that simply blindly
removing all unneeded tests without thinking whether the code could be
written in a better way is a good idea.
Blindly applying patches, even those produced by coccinelle,
let alone mine, is rarely good practice.
Sure. I would just argue for changing the text associated with the
semantic patch a little bit, to suggest considering whether the code can
be reorganized to eliminate the possibility of passing NULL to these
functions in the first place.
julia
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test
On Wed, 2014-06-18 at 07:25 +0200, Julia Lawall wrote:
>
> On Tue, 17 Jun 2014, Joe Perches wrote:
>
> > (adding Jesper Juhl)
> >
> > On Tue, 2014-06-17 at 23:33 +0200, Julia Lawall wrote:
> > > On Tue, 17 Jun 2014, Joe Perches wrote:
> > > > On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote:
> > > > > This patch adds a trivial script warning on
> > > > >
> > > > > if(foo)
> > > > > kfree(foo)
> > > > >
> > > > > (based on checkpatch)
> > > > []
> > > > > diff --git a/scripts/coccinelle/free/cond_kfree.cocci
> > > > > b/scripts/coccinelle/free/cond_kfree.cocci
> > > > []
> > > > > +* if (E)
> > > > > +*kfree@p(E);
> > > >
> > > > You should probably add all of the unnecessary
> > > > conditional tests that checkpatch uses:
> > > >
> > > > kfree
> > > > usb_free_urb
> > > > debugfs_remove
> > > > debugfs_remove_recursive
> > >
> > > Personally, I would prefer that the message encourage the user to
> > > consider
> > > whether it is necessary to call these functions with NULL as an argument
> > > in any case.
> >
> > Jesper quite awhile ago wrote:
> >
> > https://lkml.org/lkml/2005/10/13/81
> >
> > - Since kfree always checks for a NULL argument there's no reason to have an
> > additional check prior to calling kfree. It's redundant.
> > - In many cases gcc produce significantly smaller code without the redundant
> > check before the call.
> > - It's been shown in the past (in discussions on LKML) that it's generally a
> > win performance wise to avoid the extra NULL check even though it might save
> > a function call. Only when the NULL check avoids the function call in the
> > vast
> > majority of cases and the code is in a hot path does it make sense to have
> > it.
> > - This patch removes quite a few more source lines than it adds, cutting
> > down
> > on the overall number of source lines is generally a good thing.
> > - This patch reduces the indentation level, which is nice when the kfree
> > call
> > is inside some deeply nested construct.
>
> What I don't like is:
>
> a = kmalloc(...);
> if (!a) goto out;
> b = kmalloc(...);
> if (!b) goto out;
> c = kmalloc(...);
> if (!c) goto out;
> ...
> out:
> kfree(a);
> kfree(b);
> kfree(c);
>
> With a little thought, one could reorganize the code to not call kfree on
> a null value.
And I think most kernel malloc failures are written like:
a = kmalloc(...);
if (!a) goto out1;
b = kmalloc(...);
if (!b) goto out2;
c = kmalloc(...)
if (!c) goto out3;
...
out3: kfree(b);
out2: kfree(a);
out1: ...
> Someone who is not familiar with Linux programming style
> could interpret the feedback as that the above code is perfectly fine.
> (And perhaps some people do consider that it is perfectly fine).
maybe so.
> On the other hand, in the case:
>
> x = NULL;
> if (complicated_condition)
> x = kmalloc(...);
> if (!x) return;
> y = something(...);
> if (!y)
> goto out1;
> ...
> out1: kfree(x);
>
> I guess it's OK. Mildly unpleasant, but probably the best option given
> the various tradeoff.
>
> In looking at Jesper's patch, I see that another case is:
>
> a = kmalloc(...);
> b = kmalloc(...);
> if (!a || !b) {
> kfree(a);
> kfree(b);
> }
>
> Personally, I would rather see each call have its own error handling code.
> There is no point to make the second call if the first one fails.
>
> When one tries to understand code, the main questions are why is this done
> here, and why is this not done here. Doing things that are unnecessary
> introduces confusion in this regard. Perhaps it doesn't matter for
> kmalloc and kfree because everyone is familiar with them and they are
> pretty innocuous. But for the more obscure functions, like in my
> recollection of Markus's patch, I'm not convinced that simply blindly
> removing all unneeded tests without thinking whether the code could be
> written in a better way is a good idea.
Blindly applying patches, even those produced by coccinelle,
let alone mine, is rarely good practice.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test
On Tue, 17 Jun 2014, Joe Perches wrote:
> (adding Jesper Juhl)
>
> On Tue, 2014-06-17 at 23:33 +0200, Julia Lawall wrote:
> > On Tue, 17 Jun 2014, Joe Perches wrote:
> > > On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote:
> > > > This patch adds a trivial script warning on
> > > >
> > > > if(foo)
> > > > kfree(foo)
> > > >
> > > > (based on checkpatch)
> > > []
> > > > diff --git a/scripts/coccinelle/free/cond_kfree.cocci
> > > > b/scripts/coccinelle/free/cond_kfree.cocci
> > > []
> > > > +* if (E)
> > > > +* kfree@p(E);
> > >
> > > You should probably add all of the unnecessary
> > > conditional tests that checkpatch uses:
> > >
> > > kfree
> > > usb_free_urb
> > > debugfs_remove
> > > debugfs_remove_recursive
> >
> > Personally, I would prefer that the message encourage the user to consider
> > whether it is necessary to call these functions with NULL as an argument
> > in any case.
>
> Jesper quite awhile ago wrote:
>
> https://lkml.org/lkml/2005/10/13/81
>
> - Since kfree always checks for a NULL argument there's no reason to have an
> additional check prior to calling kfree. It's redundant.
> - In many cases gcc produce significantly smaller code without the redundant
> check before the call.
> - It's been shown in the past (in discussions on LKML) that it's generally a
> win performance wise to avoid the extra NULL check even though it might save
> a function call. Only when the NULL check avoids the function call in the vast
> majority of cases and the code is in a hot path does it make sense to have it.
> - This patch removes quite a few more source lines than it adds, cutting down
> on the overall number of source lines is generally a good thing.
> - This patch reduces the indentation level, which is nice when the kfree call
> is inside some deeply nested construct.
What I don't like is:
a = kmalloc(...);
if (!a) goto out;
b = kmalloc(...);
if (!b) goto out;
c = kmalloc(...);
if (!c) goto out;
...
out:
kfree(a);
kfree(b);
kfree(c);
With a little thought, one could reorganize the code to not call kfree on
a null value. Someone who is not familiar with Linux programming style
could interpret the feedback as that the above code is perfectly fine.
(And perhaps some people do consider that it is perfectly fine).
On the other hand, in the case:
x = NULL;
if (complicated_condition)
x = kmalloc(...);
if (!x) return;
y = something(...);
if (!y)
goto out1;
...
out1: kfree(x);
I guess it's OK. Mildly unpleasant, but probably the best option given
the various tradeoff.
In looking at Jesper's patch, I see that another case is:
a = kmalloc(...);
b = kmalloc(...);
if (!a || !b) {
kfree(a);
kfree(b);
}
Personally, I would rather see each call have its own error handling code.
There is no point to make the second call if the first one fails.
When one tries to understand code, the main questions are why is this done
here, and why is this not done here. Doing things that are unnecessary
introduces confusion in this regard. Perhaps it doesn't matter for
kmalloc and kfree because everyone is familiar with them and they are
pretty innocuous. But for the more obscure functions, like in my
recollection of Markus's patch, I'm not convinced that simply blindly
removing all unneeded tests without thinking whether the code could be
written in a better way is a good idea.
julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test
(adding Jesper Juhl) On Tue, 2014-06-17 at 23:33 +0200, Julia Lawall wrote: > On Tue, 17 Jun 2014, Joe Perches wrote: > > On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote: > > > This patch adds a trivial script warning on > > > > > > if(foo) > > > kfree(foo) > > > > > > (based on checkpatch) > > [] > > > diff --git a/scripts/coccinelle/free/cond_kfree.cocci > > > b/scripts/coccinelle/free/cond_kfree.cocci > > [] > > > +* if (E) > > > +*kfree@p(E); > > > > You should probably add all of the unnecessary > > conditional tests that checkpatch uses: > > > > kfree > > usb_free_urb > > debugfs_remove > > debugfs_remove_recursive > > Personally, I would prefer that the message encourage the user to consider > whether it is necessary to call these functions with NULL as an argument > in any case. Jesper quite awhile ago wrote: https://lkml.org/lkml/2005/10/13/81 - Since kfree always checks for a NULL argument there's no reason to have an additional check prior to calling kfree. It's redundant. - In many cases gcc produce significantly smaller code without the redundant check before the call. - It's been shown in the past (in discussions on LKML) that it's generally a win performance wise to avoid the extra NULL check even though it might save a function call. Only when the NULL check avoids the function call in the vast majority of cases and the code is in a hot path does it make sense to have it. - This patch removes quite a few more source lines than it adds, cutting down on the overall number of source lines is generally a good thing. - This patch reduces the indentation level, which is nice when the kfree call is inside some deeply nested construct. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test
On Tue, 17 Jun 2014, Joe Perches wrote: > On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote: > > This patch adds a trivial script warning on > > > > if(foo) > > kfree(foo) > > > > (based on checkpatch) > [] > > diff --git a/scripts/coccinelle/free/cond_kfree.cocci > > b/scripts/coccinelle/free/cond_kfree.cocci > [] > > +* if (E) > > +* kfree@p(E); > > You should probably add all of the unnecessary > conditional tests that checkpatch uses: > > kfree > usb_free_urb > debugfs_remove > debugfs_remove_recursive Personally, I would prefer that the message encourage the user to consider whether it is necessary to call these functions with NULL as an argument in any case. julia -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test
>> This patch adds a trivial script warning on >> >> if(foo) >> kfree(foo) [...] > You should probably add all of the unnecessary > conditional tests that checkpatch uses: [...] Would you like to look at my previous update suggestion "Deletion of unnecessary checks before specific function calls" again? https://systeme.lip6.fr/pipermail/cocci/2014-March/000675.html https://lkml.org/lkml/2014/3/5/344 Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test
On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote: > This patch adds a trivial script warning on > > if(foo) > kfree(foo) > > (based on checkpatch) [] > diff --git a/scripts/coccinelle/free/cond_kfree.cocci > b/scripts/coccinelle/free/cond_kfree.cocci [] > +* if (E) > +*kfree@p(E); You should probably add all of the unnecessary conditional tests that checkpatch uses: kfree usb_free_urb debugfs_remove debugfs_remove_recursive -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] scripts/coccinelle/free: add conditional kfree test
This patch adds a trivial script warning on
if(foo)
kfree(foo)
(based on checkpatch)
Cc: Julia Lawall
Cc: Gilles Muller
Cc: Joe Perches
Signed-off-by: Fabian Frederick
---
scripts/coccinelle/free/cond_kfree.cocci | 34
1 file changed, 34 insertions(+)
create mode 100644 scripts/coccinelle/free/cond_kfree.cocci
diff --git a/scripts/coccinelle/free/cond_kfree.cocci
b/scripts/coccinelle/free/cond_kfree.cocci
new file mode 100644
index 000..0b8093e
--- /dev/null
+++ b/scripts/coccinelle/free/cond_kfree.cocci
@@ -0,0 +1,34 @@
+/// conditional kfree - NULL check before kfree is not needed.
+///
+/// Based on checkpatch warning
+/// "kfree(NULL) is safe this check is probably not required"
+/// and kfreeaddr.cocci by Julia Lawall.
+///
+/// Comments: -
+/// Options: --no-includes --include-headers
+
+virtual org
+virtual report
+virtual context
+
+@r depends on context || report || org @
+expression E;
+position p;
+@@
+
+* if (E)
+* kfree@p(E);
+
+@script:python depends on org@
+p << r.p;
+@@
+
+cocci.print_main("kfree", p)
+
+@script:python depends on report@
+p << r.p;
+@@
+
+msg = "WARNING: checking value to avoid kfree(NULL) is unnecessary."
+coccilib.report.print_report(p[0], msg)
+
--
1.8.4.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 1/1] scripts/coccinelle/free: add conditional kfree test
This patch adds a trivial script warning on if(foo) kfree(foo) (based on checkpatch) Cc: Julia Lawall julia.law...@lip6.fr Cc: Gilles Muller gilles.mul...@lip6.fr Cc: Joe Perches j...@perches.com Signed-off-by: Fabian Frederick f...@skynet.be --- scripts/coccinelle/free/cond_kfree.cocci | 34 1 file changed, 34 insertions(+) create mode 100644 scripts/coccinelle/free/cond_kfree.cocci diff --git a/scripts/coccinelle/free/cond_kfree.cocci b/scripts/coccinelle/free/cond_kfree.cocci new file mode 100644 index 000..0b8093e --- /dev/null +++ b/scripts/coccinelle/free/cond_kfree.cocci @@ -0,0 +1,34 @@ +/// conditional kfree - NULL check before kfree is not needed. +/// +/// Based on checkpatch warning +/// kfree(NULL) is safe this check is probably not required +/// and kfreeaddr.cocci by Julia Lawall. +/// +/// Comments: - +/// Options: --no-includes --include-headers + +virtual org +virtual report +virtual context + +@r depends on context || report || org @ +expression E; +position p; +@@ + +* if (E) +* kfree@p(E); + +@script:python depends on org@ +p r.p; +@@ + +cocci.print_main(kfree, p) + +@script:python depends on report@ +p r.p; +@@ + +msg = WARNING: checking value to avoid kfree(NULL) is unnecessary. +coccilib.report.print_report(p[0], msg) + -- 1.8.4.5 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test
On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote: This patch adds a trivial script warning on if(foo) kfree(foo) (based on checkpatch) [] diff --git a/scripts/coccinelle/free/cond_kfree.cocci b/scripts/coccinelle/free/cond_kfree.cocci [] +* if (E) +*kfree@p(E); You should probably add all of the unnecessary conditional tests that checkpatch uses: kfree usb_free_urb debugfs_remove debugfs_remove_recursive -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test
This patch adds a trivial script warning on if(foo) kfree(foo) [...] You should probably add all of the unnecessary conditional tests that checkpatch uses: [...] Would you like to look at my previous update suggestion Deletion of unnecessary checks before specific function calls again? https://systeme.lip6.fr/pipermail/cocci/2014-March/000675.html https://lkml.org/lkml/2014/3/5/344 Regards, Markus -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test
On Tue, 17 Jun 2014, Joe Perches wrote: On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote: This patch adds a trivial script warning on if(foo) kfree(foo) (based on checkpatch) [] diff --git a/scripts/coccinelle/free/cond_kfree.cocci b/scripts/coccinelle/free/cond_kfree.cocci [] +* if (E) +* kfree@p(E); You should probably add all of the unnecessary conditional tests that checkpatch uses: kfree usb_free_urb debugfs_remove debugfs_remove_recursive Personally, I would prefer that the message encourage the user to consider whether it is necessary to call these functions with NULL as an argument in any case. julia -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test
(adding Jesper Juhl) On Tue, 2014-06-17 at 23:33 +0200, Julia Lawall wrote: On Tue, 17 Jun 2014, Joe Perches wrote: On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote: This patch adds a trivial script warning on if(foo) kfree(foo) (based on checkpatch) [] diff --git a/scripts/coccinelle/free/cond_kfree.cocci b/scripts/coccinelle/free/cond_kfree.cocci [] +* if (E) +*kfree@p(E); You should probably add all of the unnecessary conditional tests that checkpatch uses: kfree usb_free_urb debugfs_remove debugfs_remove_recursive Personally, I would prefer that the message encourage the user to consider whether it is necessary to call these functions with NULL as an argument in any case. Jesper quite awhile ago wrote: https://lkml.org/lkml/2005/10/13/81 - Since kfree always checks for a NULL argument there's no reason to have an additional check prior to calling kfree. It's redundant. - In many cases gcc produce significantly smaller code without the redundant check before the call. - It's been shown in the past (in discussions on LKML) that it's generally a win performance wise to avoid the extra NULL check even though it might save a function call. Only when the NULL check avoids the function call in the vast majority of cases and the code is in a hot path does it make sense to have it. - This patch removes quite a few more source lines than it adds, cutting down on the overall number of source lines is generally a good thing. - This patch reduces the indentation level, which is nice when the kfree call is inside some deeply nested construct. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test
On Tue, 17 Jun 2014, Joe Perches wrote:
(adding Jesper Juhl)
On Tue, 2014-06-17 at 23:33 +0200, Julia Lawall wrote:
On Tue, 17 Jun 2014, Joe Perches wrote:
On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote:
This patch adds a trivial script warning on
if(foo)
kfree(foo)
(based on checkpatch)
[]
diff --git a/scripts/coccinelle/free/cond_kfree.cocci
b/scripts/coccinelle/free/cond_kfree.cocci
[]
+* if (E)
+* kfree@p(E);
You should probably add all of the unnecessary
conditional tests that checkpatch uses:
kfree
usb_free_urb
debugfs_remove
debugfs_remove_recursive
Personally, I would prefer that the message encourage the user to consider
whether it is necessary to call these functions with NULL as an argument
in any case.
Jesper quite awhile ago wrote:
https://lkml.org/lkml/2005/10/13/81
- Since kfree always checks for a NULL argument there's no reason to have an
additional check prior to calling kfree. It's redundant.
- In many cases gcc produce significantly smaller code without the redundant
check before the call.
- It's been shown in the past (in discussions on LKML) that it's generally a
win performance wise to avoid the extra NULL check even though it might save
a function call. Only when the NULL check avoids the function call in the vast
majority of cases and the code is in a hot path does it make sense to have it.
- This patch removes quite a few more source lines than it adds, cutting down
on the overall number of source lines is generally a good thing.
- This patch reduces the indentation level, which is nice when the kfree call
is inside some deeply nested construct.
What I don't like is:
a = kmalloc(...);
if (!a) goto out;
b = kmalloc(...);
if (!b) goto out;
c = kmalloc(...);
if (!c) goto out;
...
out:
kfree(a);
kfree(b);
kfree(c);
With a little thought, one could reorganize the code to not call kfree on
a null value. Someone who is not familiar with Linux programming style
could interpret the feedback as that the above code is perfectly fine.
(And perhaps some people do consider that it is perfectly fine).
On the other hand, in the case:
x = NULL;
if (complicated_condition)
x = kmalloc(...);
if (!x) return;
y = something(...);
if (!y)
goto out1;
...
out1: kfree(x);
I guess it's OK. Mildly unpleasant, but probably the best option given
the various tradeoff.
In looking at Jesper's patch, I see that another case is:
a = kmalloc(...);
b = kmalloc(...);
if (!a || !b) {
kfree(a);
kfree(b);
}
Personally, I would rather see each call have its own error handling code.
There is no point to make the second call if the first one fails.
When one tries to understand code, the main questions are why is this done
here, and why is this not done here. Doing things that are unnecessary
introduces confusion in this regard. Perhaps it doesn't matter for
kmalloc and kfree because everyone is familiar with them and they are
pretty innocuous. But for the more obscure functions, like in my
recollection of Markus's patch, I'm not convinced that simply blindly
removing all unneeded tests without thinking whether the code could be
written in a better way is a good idea.
julia
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 1/1] scripts/coccinelle/free: add conditional kfree test
On Wed, 2014-06-18 at 07:25 +0200, Julia Lawall wrote:
On Tue, 17 Jun 2014, Joe Perches wrote:
(adding Jesper Juhl)
On Tue, 2014-06-17 at 23:33 +0200, Julia Lawall wrote:
On Tue, 17 Jun 2014, Joe Perches wrote:
On Tue, 2014-06-17 at 21:43 +0200, Fabian Frederick wrote:
This patch adds a trivial script warning on
if(foo)
kfree(foo)
(based on checkpatch)
[]
diff --git a/scripts/coccinelle/free/cond_kfree.cocci
b/scripts/coccinelle/free/cond_kfree.cocci
[]
+* if (E)
+*kfree@p(E);
You should probably add all of the unnecessary
conditional tests that checkpatch uses:
kfree
usb_free_urb
debugfs_remove
debugfs_remove_recursive
Personally, I would prefer that the message encourage the user to
consider
whether it is necessary to call these functions with NULL as an argument
in any case.
Jesper quite awhile ago wrote:
https://lkml.org/lkml/2005/10/13/81
- Since kfree always checks for a NULL argument there's no reason to have an
additional check prior to calling kfree. It's redundant.
- In many cases gcc produce significantly smaller code without the redundant
check before the call.
- It's been shown in the past (in discussions on LKML) that it's generally a
win performance wise to avoid the extra NULL check even though it might save
a function call. Only when the NULL check avoids the function call in the
vast
majority of cases and the code is in a hot path does it make sense to have
it.
- This patch removes quite a few more source lines than it adds, cutting
down
on the overall number of source lines is generally a good thing.
- This patch reduces the indentation level, which is nice when the kfree
call
is inside some deeply nested construct.
What I don't like is:
a = kmalloc(...);
if (!a) goto out;
b = kmalloc(...);
if (!b) goto out;
c = kmalloc(...);
if (!c) goto out;
...
out:
kfree(a);
kfree(b);
kfree(c);
With a little thought, one could reorganize the code to not call kfree on
a null value.
And I think most kernel malloc failures are written like:
a = kmalloc(...);
if (!a) goto out1;
b = kmalloc(...);
if (!b) goto out2;
c = kmalloc(...)
if (!c) goto out3;
...
out3: kfree(b);
out2: kfree(a);
out1: ...
Someone who is not familiar with Linux programming style
could interpret the feedback as that the above code is perfectly fine.
(And perhaps some people do consider that it is perfectly fine).
maybe so.
On the other hand, in the case:
x = NULL;
if (complicated_condition)
x = kmalloc(...);
if (!x) return;
y = something(...);
if (!y)
goto out1;
...
out1: kfree(x);
I guess it's OK. Mildly unpleasant, but probably the best option given
the various tradeoff.
In looking at Jesper's patch, I see that another case is:
a = kmalloc(...);
b = kmalloc(...);
if (!a || !b) {
kfree(a);
kfree(b);
}
Personally, I would rather see each call have its own error handling code.
There is no point to make the second call if the first one fails.
When one tries to understand code, the main questions are why is this done
here, and why is this not done here. Doing things that are unnecessary
introduces confusion in this regard. Perhaps it doesn't matter for
kmalloc and kfree because everyone is familiar with them and they are
pretty innocuous. But for the more obscure functions, like in my
recollection of Markus's patch, I'm not convinced that simply blindly
removing all unneeded tests without thinking whether the code could be
written in a better way is a good idea.
Blindly applying patches, even those produced by coccinelle,
let alone mine, is rarely good practice.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
