Use !x to check for kzalloc failure
Subsystems (2)
cover Use !x to check for kzalloc failure
Use !x to check for kzalloc failure, as is more normal in the kernel.
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
PATCH 1/12 staging: lustre: fid: Use !x to check for kzalloc failure
!x is more normal for kzalloc failure in the kernel.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x;
statement S1, S2;
@@
x = kzalloc(...);
if (
- x = NULL
+ !x
) S1 else S2
// </smpl>
--- drivers/staging/lustre/lustre/fid/fid_request.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/lustre/lustre/fid/fid_request.c +2 -2
@@ -498,11 +498,11 @@ int client_fid_init(struct obd_device *oint rc; cli->cl_seq =kzalloc (sizeof (*cli->cl_seq),GFP_NOFS ); -if (cli->cl_seq =NULL ) +if (!cli->cl_seq)return -ENOMEM ; prefix =kzalloc (MAX_OBD_NAME +5 ,GFP_NOFS ); -if (prefix =NULL ) { +if (!prefix) { rc = -ENOMEM ; goto out_free_seq; }
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
PATCH 1/12 staging: lustre: fid: Use !x to check for kzalloc failure
Question for lustre-devel... Isn't this the opposite of the style that we're currently trying to use in Gerrit submissions? Is my memory faulty?
-------- Original Message --------
Subject: [HPDD-discuss] [PATCH 01/12] staging: lustre: fid: Use !x to
check for kzalloc failure
Date: Sat, 20 Jun 2015 18:58:59 +0200
From: Julia Lawall <[email protected]>
To: Oleg Drokin <[email protected]>
CC: <[email protected]>, Greg Kroah-Hartman
<[email protected]>, <[email protected]>, <[email protected]>, <[email protected]>
!x is more normal for kzalloc failure in the kernel.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x;
statement S1, S2;
@@
x = kzalloc(...);
if (
- x == NULL
+ !x
) S1 else S2
// </smpl>
--- drivers/staging/lustre/lustre/fid/fid_request.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/lustre/lustre/fid/fid_request.c +2 -2
@@ -498,11 +498,11 @@ int client_fid_init(struct obd_device *oint rc; cli->cl_seq =kzalloc (sizeof (*cli->cl_seq),GFP_NOFS ); -if (cli->cl_seq ==NULL ) +if (!cli->cl_seq)return -ENOMEM ; prefix =kzalloc (MAX_OBD_NAME +5 ,GFP_NOFS ); -if (prefix ==NULL ) { +if (!prefix) { rc = -ENOMEM ; goto out_free_seq; }
_______________________________________________
HPDD-discuss mailing list
HPDD-discuss at lists.01.org
https://lists.01.org/mailman/listinfo/hpdd-discuss
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20150622/efb39765/attachment.htm>
Simmons
PATCH 1/12 staging: lustre: fid: Use !x to check for kzalloc failure
Quoted 1 line
Question for lustre-devel... Isn't this the opposite of the style that we're currently trying to use in Gerrit submissions? Is my memory faulty?
You are correct. Is this change really necessary? I will respone shortly.
-------- Original Message --------
Subject:
[HPDD-discuss] [PATCH 01/12] staging: lustre: fid: Use !x to check for kzalloc failure
Date:
Sat, 20 Jun 2015 18:58:59 +0200
From:
Julia Lawall <[email protected]><mailto:[email protected]>
To:
Oleg Drokin <[email protected]><mailto:[email protected]>
CC:
<[email protected]><mailto:[email protected]>, Greg Kroah-Hartman <[email protected]><mailto:[email protected]>, <[email protected]><mailto:[email protected]>, <[email protected]><mailto:[email protected]>, <[email protected]><mailto:[email protected]>
!x is more normal for kzalloc failure in the kernel.
The semantic patch that makes this change is as follows:
// <smpl>
@@
expression x;
statement S1, S2;
@@
x = kzalloc(...);
if (
- x == NULL
+ !x
) S1 else S2
// </smpl>
---
drivers/staging/lustre/lustre/fid/fid_request.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff -u -p a/drivers/staging/lustre/lustre/fid/fid_request.c b/drivers/staging/lustre/lustre/fid/fid_request.c
--- a/drivers/staging/lustre/lustre/fid/fid_request.c
+++ b/drivers/staging/lustre/lustre/fid/fid_request.c
@@ -498,11 +498,11 @@ int client_fid_init(struct obd_device *o
int rc;
cli->cl_seq = kzalloc(sizeof(*cli->cl_seq), GFP_NOFS);- if (cli->cl_seq == NULL)
+ if (!cli->cl_seq)
return -ENOMEM;
prefix = kzalloc(MAX_OBD_NAME + 5, GFP_NOFS);- if (prefix == NULL) {
+ if (!prefix) {
rc = -ENOMEM;
goto out_free_seq;
}_______________________________________________
HPDD-discuss mailing list
HPDD-discuss at lists.01.org<mailto:[email protected]>
https://lists.01.org/mailman/listinfo/hpdd-discuss
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.lustre.org/pipermail/lustre-devel-lustre.org/attachments/20150622/cc615a1b/attachment.htm>
Dilger
PATCH 1/12 staging: lustre: fid: Use !x to check for kzalloc failure
In reply to Patrick FarrellYes, and IMHO this set of patches is not an improvement.
Cheers, Andreas
On Jun 22, 2015, at 08:46, Patrick Farrell <paf at cray.com<mailto:[email protected]>> wrote:
Question for lustre-devel... Isn't this the opposite of the style that we're currently trying to use in Gerrit submissions? Is my memory faulty?
-------- Original Message --------
Subject: [HPDD-discuss] [PATCH 01/12] staging: lustre: fid: Use !x to check for kzalloc failure Date: Sat, 20 Jun 2015 18:58:59 +0200 From: Julia Lawall <[email protected]><mailto:[email protected]> To: Oleg Drokin <[email protected]><mailto:[email protected]>
!x is more normal for kzalloc failure in the kernel.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x;
statement S1, S2;
@@
x = kzalloc(...);
if (
- x == NULL
+ !x
) S1 else S2
// </smpl>
--- drivers/staging/lustre/lustre/fid/fid_request.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/lustre/lustre/fid/fid_request.c +2 -2
@@ -498,11 +498,11 @@ int client_fid_init(struct obd_device *oint rc; cli->cl_seq =kzalloc (sizeof (*cli->cl_seq),GFP_NOFS ); -if (cli->cl_seq ==NULL ) +if (!cli->cl_seq)return -ENOMEM ; prefix =kzalloc (MAX_OBD_NAME +5 ,GFP_NOFS ); -if (prefix ==NULL ) { +if (!prefix) { rc = -ENOMEM ; goto out_free_seq; }
_______________________________________________
HPDD-discuss mailing list
HPDD-discuss at lists.01.org<mailto:[email protected]>
https://lists.01.org/mailman/listinfo/hpdd-discuss
_______________________________________________
lustre-devel mailing list
lustre-devel at lists.lustre.org<mailto:[email protected]>
http://lists.lustre.org/listinfo.cgi/lustre-devel-lustre.org
Re: [PATCH 01/12] staging: lustre: fid: Use !x to check for kzalloc failure
In reply to Julia LawallQuoted 1 line · Julia Lawall
!x is more normal for kzalloc failure in the kernel.
While "!x" might be more normal for kzalloc(), I don't see that as an improvement over explicitly checking against NULL, which is what kzalloc() and other memory-allocating functions return on error.
I've found in the past that developers can introduce bugs when they treat return values as boolean when they really aren't. I'd prefer that the code is kept with explicit comparisons against NULL, as it is today. Most of the cases that are now using "!x" are from your previous patches.
Cheers, Andreas
Quoted 16 lines
Signed-off-by Julia Lawall <[email protected]>---
drivers/staging/lustre/lustre/fid/fid_request.c| 4 ++--1 file changed, 2 insertions(+), 2 deletions(-)diff -u -p
a/drivers/staging/lustre/lustre/fid/fid_request.cb/drivers/staging/lustre/lustre/fid/fid_request.cdrivers/staging/lustre/lustre/fid/fid_request.c +0
-0@@ -498,11 +498,11 @@ int client_fid_init(struct obd_device *oint rc; cli->cl_seq =kzalloc (sizeof (*cli->cl_seq),GFP_NOFS );
drivers/staging/lustre/lustre/fid/fid_request.c int client_fid_init(struct obd_device *o · line 508
Quoted 10 lines
drivers/staging/lustre/lustre/fid/fid_request.c +2
-2-if (cli->cl_seq =NULL ) +if (!cli->cl_seq)return -ENOMEM ; prefix =kzalloc (MAX_OBD_NAME +5 ,GFP_NOFS ); -if (prefix =NULL ) { +if (!prefix) { rc = -ENOMEM ; goto out_free_seq; }
Cheers, Andreas
Signature · 4 lines
Andreas Dilger Lustre Software Architect Intel High Performance Data Division
Re: [PATCH 01/12] staging: lustre: fid: Use !x to check for kzalloc failure
Quoted 2 lines · Andreas
I've found in the past that developers can introduce bugs when they treat return values as boolean when they really aren't.
I can imagine a bug like that where a function can return 0-2 and people do:
if (ret)
instead of:
if (ret = 1)
but that bug is something else besides pointers so it doesn't apply here.
What someone should do is try to measure it scientifically where we flash some code on the screen and you have to press J for NULL and K for non-NULL and we time it to the hundredth of a second. I have a feeling that (NULL != foo) is the worst way to write it because of the double negative Yoda code.
Yoda code is the most useless thing ever. I have actually measured this and we introduce about 2 = vs = bugs per year. It's probably less now that we have so many static checks against it. But people decided that Yoda code was a good idea based on their gut instead of using statistics and measurements and science.
regards,
dan carpenter
Re: [PATCH 01/12] staging: lustre: fid: Use !x to check for kzalloc failure
Quoted 22 lines · Dan Carpenter
Quoted 2 lines · Andreas
I've found in the past that developers can introduce bugs when they treat return values as boolean when they really aren't.
I can imagine a bug like that where a function can return 0-2 and people do:
if (ret)
instead of:
if (ret = 1)but that bug is something else besides pointers so it doesn't apply here.
What someone should do is try to measure it scientifically where we flash some code on the screen and you have to press J for NULL and K for non-NULL and we time it to the hundredth of a second. I have a feeling that (NULL != foo) is the worst way to write it because of the double negative Yoda code.
Quoted 5 lines
Yoda code is the most useless thing ever. I have actually measured this and we introduce about 2 = vs = bugs per year. It's probably less now that we have so many static checks against it. But people decided that Yoda code was a good idea based on their gut instead of using statistics and measurements and science.
In 2007, Al Viro said (https://lkml.org/lkml/2007/7/27/103):
Idiomatic form for "has allocation succeeded?" is neither "if (p != 0)" nor "if (p != NULL)". It's simply "if (p)".
From the point of view of looking at kernel code, x = NULL for the result of kmalloc etc looks verbose and distracting.
julia
Re: [PATCH 01/12] staging: lustre: fid: Use !x to check for kzalloc failure
Yes. I know Al's thoughts and kernel style.
But Alan Cox and Andreas have both said they think (x = NULL) can help you avoid some kind of boolean vs pointer bugs. I've had co-workers who did massive seds changing !foo to foo = NULL on our code base. But I've never seen a real life example of a bug this fixes.
To be honest, I've never seen a real life proof that (!foo) code is less buggy. I should look through the kbuild mailbox... Hm... But my other idea of setting up code style readability testing website is also a good one.
Linux kernel style is based on Joe Perches finding that 80% of the code prefers one way or the other. That's a valid method for determining code style. I bet it normally picks the more readable style but it would be interesting to measure it more formally.
regards,
dan carpenter
Re: [PATCH 01/12] staging: lustre: fid: Use !x to check for kzalloc failure
Quoted 12 lines · Dan Carpenter
Yes. I know Al's thoughts and kernel style.
But Alan Cox and Andreas have both said they think (x = NULL) can help you avoid some kind of boolean vs pointer bugs. I've had co-workers who did massive seds changing !foo to foo = NULL on our code base. But I've never seen a real life example of a bug this fixes.
To be honest, I've never seen a real life proof that (!foo) code is less buggy. I should look through the kbuild mailbox... Hm... But my other idea of setting up code style readability testing website is also a good one.
Quoted 4 lines
Linux kernel style is based on Joe Perches finding that 80% of the code prefers one way or the other. That's a valid method for determining code style. I bet it normally picks the more readable style but it would be interesting to measure it more formally.
On today's linux-next, I find 3218 tests on the result of kmalloc etc using NULL and 14429 without, making 82% without. The complete semantic patch is shown below.
julia
@initialize:ocaml@
@@
let withnull = ref 0
let withoutnull = ref 0
@r1 disable is_null, isnt_null1 exists@
expression x,e;
position p;
statement S1,S2;
@@
x = \(kmalloc\|kzalloc\|kcalloc\|devm_kmalloc\|devm_kzalloc\)(...) ... when != x = e
when != &x
if@p (<+...\(x = NULL\|x != NULL\|NULL = x\|NULL != x\)...+>) S1 else S2
@r2 disable not_ptr1, not_ptr2 exists@
expression x,e;
position p;
statement S1,S2;
@@
x = \(kmalloc\|kzalloc\|kcalloc\|devm_kmalloc\|devm_kzalloc\)(...) ... when != x = e
when != &x
if@p (<+...\(!x\|x && ...\|x || ...\)...+>) S1 else S2
@script:ocaml@
_p << r1.p;
@@
withnull := !withnull + 1
@script:ocaml@
_p << r2.p;
@@
withoutnull := !withoutnull + 1
@finalize:ocaml@
@@
Printf.printf "withnull %d withoutnull %d\n" !withnull !withoutnull
RE: [lustre-devel] [PATCH 01/12] staging: lustre: fid: Use !x to check for kzalloc failure
Quoted 12 lines
Quoted 12 lines
Yes. I know Al's thoughts and kernel style.
But Alan Cox and Andreas have both said they think (x = NULL) can help you avoid some kind of boolean vs pointer bugs. I've had co-workers who did massive seds changing !foo to foo = NULL on our code base. But I've never seen a real life example of a bug this fixes.
To be honest, I've never seen a real life proof that (!foo) code is less buggy. I should look through the kbuild mailbox... Hm... But my other idea of setting up code style readability testing website is also a good one.
Quoted 8 lines
Quoted 4 lines
Linux kernel style is based on Joe Perches finding that 80% of the code prefers one way or the other. That's a valid method for determining code style. I bet it normally picks the more readable style but it would be interesting to measure it more formally.
On today's linux-next, I find 3218 tests on the result of kmalloc etc using NULL and 14429 without, making 82% without. The complete semantic patch is shown below.
Most people doing something a certain way is not a technical argument. Usually people do what they are taught. From most people's comments their seems to be no technical reason to us one over another. I do have one technical reason not to accept these patches. It is too easy to make a mistake and break things very badly. I don't think it is worth the risk for a non-hard requirement.
Re: [PATCH 01/12] staging: lustre: fid: Use !x to check for kzalloc failure
In reply to Dan CarpenterQuoted 2 lines · Dan Carpenter
I've never seen a real life proof that (!foo) code is less buggy.
Nor have I.
Quoted 8 lines
I should look through the kbuild mailbox... Hm... But my other idea of setting up code style readability testing website is also a good one.
Linux kernel style is based on Joe Perches finding that 80% of the code prefers one way or the other. That's a valid method for determining code style. I bet it normally picks the more readable style but it would be interesting to measure it more formally.
That might be hard to do well.
A code readability testing website is going to be fundamentally biased by the experiences of the coder that is tested.
Flashing code for millisecond type readability tests has more correlation to quantity of white to black than code content does of correctness to memorability.
Re: [PATCH 01/12] staging: lustre: fid: Use !x to check for kzalloc failure
In reply to Dan CarpenterQuoted 3 lines · Dan Carpenter
people decided that
Yoda code was a good idea based on their gut instead of using statistics and measurements and science.
I think that style exists because compilers
disallow CONST = val assignment typos.
LIBCFS_ALLOC
In reply to Dilger, AndreasIt is not clear that all of the uses of LIBCFS_ALLOC really risk needing vmalloc. For example:
lnet/klnds/socklnd/socklnd.c, function ksocknal_accept:
ksock_connreq_t *cr;
...LIBCFS_ALLOC(cr, sizeof(*cr));
The definition of ksock_connreq_t is:
typedef struct ksock_connreq {
struct list_head ksncr_list; /* stash on ksnd_connd_connreqs */
lnet_ni_t *ksncr_ni; /* chosen NI */
struct socket *ksncr_sock; /* accepted socket */
} ksock_connreq_t;
This looks like a very small structure.
LIBCFS_ALLOC relies on a test on the size, which should be able to be compiled away. libcfs_kvzalloc on the other hand relies on the failure of kmalloc and so the test for that won't be compiled away.
Does it matter?
julia
Re: LIBCFS_ALLOC
Yeah. You're right. Doing a vmalloc() when kmalloc() doesn't have even a tiny sliver of RAM isn't going to work. It's easier to use libcfs_kvzalloc() everywhere, but it's probably the wrong thing.
regards,
dan carpenter
RE: LIBCFS_ALLOC
Quoted 3 lines
Yeah. You're right. Doing a
vmalloc()whenkmalloc()doesn't have even a tiny sliver of RAM isn't going to work. It's easier to uselibcfs_kvzalloc()everywhere, but it's probably the wrong thing.
The original reason we have the vmalloc water mark wasn't so much the issue of memory exhaustion but to handle the case of memory fragmentation. Some sites had after a extended period of time started to see failures of allocating even 32K using kmalloc. In our latest development branch we moved away from using a water mark to always try kmalloc first and if it fails then we try vmalloc. At ORNL we ran into severe performance issues when we entered vmalloc territory. It has been discussed before on what might replace vmalloc handling in the case of kmalloc fails but no solution has been worked out.
RE: LIBCFS_ALLOC
Quoted 4 lines · James A.
Quoted 3 lines
Yeah. You're right. Doing a
vmalloc()whenkmalloc()doesn't have even a tiny sliver of RAM isn't going to work. It's easier to uselibcfs_kvzalloc()everywhere, but it's probably the wrong thing.
Quoted 8 lines
The original reason we have the vmalloc water mark wasn't so much the issue of memory exhaustion but to handle the case of memory fragmentation. Some sites had after a extended period of time started to see failures of allocating even 32K using kmalloc. In our latest development branch we moved away from using a water mark to always try kmalloc first and if it fails then we try vmalloc. At ORNL we ran into severe performance issues when we entered vmalloc territory. It has been discussed before on what might replace vmalloc handling in the case of kmalloc fails but no solution has been worked out.
OK, but if a structure contains only 4 words, would it be better to just use kzalloc? Or does it not matter? It would only save trying vmalloc in a case that it is guaranteed to fail, but if a structure with 4 words can't be allocatted, the system has other problems. Another argument is that kzalloc is a well known function that people and bug-finding tools understand, so it is better to use it whenever possible.
Some of the other structures contain a lot more fields, as well as small arrays. They are probably acceptable for kzalloc too, but I wouldn't know the exact dividing line.
julia
RE: [lustre-devel] LIBCFS_ALLOC
Quoted 21 lines
Quoted 5 lines
Quoted 4 lines
Yeah. You're right. Doing a
vmalloc()whenkmalloc()doesn't have even a tiny sliver of RAM isn't going to work. It's easier to uselibcfs_kvzalloc()everywhere, but it's probably the wrong thing.Quoted 8 lines
The original reason we have the vmalloc water mark wasn't so much the issue of memory exhaustion but to handle the case of memory fragmentation. Some sites had after a extended period of time started to see failures of allocating even 32K using kmalloc. In our latest development branch we moved away from using a water mark to always try kmalloc first and if it fails then we try vmalloc. At ORNL we ran into severe performance issues when we entered vmalloc territory. It has been discussed before on what might replace vmalloc handling in the case of kmalloc fails but no solution has been worked out.
OK, but if a structure contains only 4 words, would it be better to just use kzalloc? Or does it not matter? It would only save trying vmalloc in a case that it is guaranteed to fail, but if a structure with 4 words can't be allocated, the system has other problems. Another argument is that kzalloc is a well known function that people and bug-finding tools understand, so it is better to use it whenever possible.
Quoted 3 lines
Some of the other structures contain a lot more fields, as well as small arrays. They are probably acceptable for kzalloc too, but I wouldn't know the exact dividing line.
The reason I bring this up is to discuss sorting this out. Once long ago we had just LIBCFS_ALLOC. For some reason before my time OBD_ALLOC got spawned off of that. Currently LIBCFS_ALLOC is used just by the libcfs/LNet layer. Now OBD_ALLOC in our development branch has moved to a try kmalloc first and if it fails try vmalloc for any size memory allocation. LIBCFS_ALLOC still does the
original approach. So we have two possible solutions depending on if libcfs/LNet
needs to ever do a vmalloc.
One solution if libcfs/LNet never needs a vmalloc is remove LIBCFS_ALLOC and replace it with kzalloc everywhere. We can then move libcfs_kzvalloc to the lustre layer and port the change we did in the development branch to here of the try kmalloc then vmalloc approach.
The other approach is if libcfs/LNet does in some case need to use vmalloc we could then update LIBCFS_ALLOC to first try kmalloc then vmalloc. Once this is implemented we can nuke the OBD_ALLOC system.
Either way I like to see it consolidated down to one system.
Re: [lustre-devel] LIBCFS_ALLOC
Quoted 35 lines · James A.
Quoted 30 lines
Quoted 6 lines
Quoted 2 lines
Yeah. You're right. Doing a
vmalloc()whenkmalloc()doesn't haveeven
Quoted 2 lines
a tiny sliver of RAM isn't going to work. It's easier to use
libcfs_kvzalloc()everywhere, but it's probably the wrong thing.Quoted 15 lines
The original reason we have the vmalloc water mark wasn't so much the issue of memory exhaustion but to handle the case of memory fragmentation.
Some sites had after a extended period of time started to see failures of
allocating even 32K using kmalloc. In our latest development branch we moved
away from using a water mark to always try kmalloc first and if it fails then we
try vmalloc. At ORNL we ran into severe performance issues when we entered
vmalloc territory. It has been discussed before on what might replace vmalloc
handling in the case of kmalloc fails but no solution has been worked out.OK, but if a structure contains only 4 words, would it be better to just use kzalloc? Or does it not matter? It would only save trying vmalloc in
a case that it is guaranteed to fail, but if a structure with 4 words can't be allocated, the system has other problems. Another argument is that kzalloc is a well known function that people and bug-finding tools understand, so it is better to use it whenever possible.Quoted 4 lines
Some of the other structures contain a lot more fields, as well as small arrays. They are probably acceptable for kzalloc too, but I wouldn't know
the exact dividing line.
Quoted 5 lines
The reason I bring this up is to discuss sorting this out. Once long ago we had just
LIBCFS_ALLOC. For some reason before my timeOBD_ALLOCgot spawned off of that. CurrentlyLIBCFS_ALLOCis used just by the libcfs/LNet
layer.
That is because there was (is?) interest from Cray and others to use LNet independently from Lustre (Zest and DVS, for example) so LNet should be self-contained and not depend on anything from Lustre.
Quoted 13 lines
Now
OBD_ALLOCin our development branch has moved to a try kmalloc first and
if it fails try vmalloc for any size memory allocation.LIBCFS_ALLOCstill
does the original approach. So we have two possible solutions
depending on if libcfs/LNet needs to ever do a vmalloc.One solution if libcfs/LNet never needs a vmalloc is remove
LIBCFS_ALLOCand replace it with kzalloc everywhere. We can then movelibcfs_kzvallocto
the lustre layer and port the change we did in the development branch to here of the try kmalloc then vmalloc approach.
Quoted 3 lines
The other approach is if libcfs/LNet does in some case need to use vmalloc we could then update
LIBCFS_ALLOCto first try kmalloc then vmalloc. Once this is implemented we can nuke theOBD_ALLOCsystem.
I don't agree. I think there are a few places where vmalloc() makes sense to try (if the allocation may be large), but in most places LIBCFS_ALLOC() should only use kmalloc().
Unfortunately, there wasn't a separate LIBCFS_ALLOC_LARGE() like there was for OBD_ALLOC_LARGE() that made it clear which callsites are (potentially) large and which are small. The macro approach allowed the compile-time optimization of the small callsites, but that needs to be done by hand now.
Quoted 1 line
Either way I like to see it consolidated down to one system.
Given the proliferation of foo_kvmalloc() and foo_kvzalloc() helpers (ext4_, kvm_, dm_, apparmor, ceph_, __aa_), maybe it it is time to move these to common kernel code instead of introducing yet another new one?
Cheers, Andreas
Signature · 4 lines
Andreas Dilger Lustre Software Architect Intel High Performance Data Division
Re: LIBCFS_ALLOC
In reply to Simmons, James A.All that you are saying is true and stuff that Julia and I have discussed before. For this call site though we are not allocating 32k, we're allocating 4 pointers so libcfs_kvzalloc() doesn't make sense.
regards,
dan carpenter
Re: [lustre-devel] LIBCFS_ALLOC
In reply to Julia LawallQuoted 17 lines · Julia Lawall
It is not clear that all of the uses of
LIBCFS_ALLOCreally risk needing vmalloc. For example:
lnet/klnds/socklnd/socklnd.c, functionksocknal_accept:
ksock_connreq_t*cr;
...LIBCFS_ALLOC(cr, sizeof(*cr));The definition of
ksock_connreq_tis:typedef struct
ksock_connreq{
structlist_headksncr_list; /* stash onksnd_connd_connreqs*/
lnet_ni_t*ksncr_ni; /* chosen NI */
struct socket*ksncr_sock; /* accepted socket */
}ksock_connreq_t;
Quoted 6 lines
This looks like a very small structure.
LIBCFS_ALLOCrelies on a test on the size, which should be able to be compiled away.libcfs_kvzallocon the other hand relies on the failure of
kmalloc and so the test for that won't be compiled away.
There are probably only a handful of places where trying vmalloc() even makes sense. In most cases, LIBCFS_ALLOC() can be replaced by a straight call to kmalloc() because the allocation size is small enough, and only a few need to use libcfs_kvzalloc(). Anything at PAGE_SIZE or less will either work with kmalloc() or it won't work at all.
I do agree with James' comments that vmalloc() was needed for both very large allocations that can't be satisfied by kmalloc() at all, as well as smaller allocations (anything over a couple of pages) due to fragmentation of free pages after running for a long time. I think libcfs_kvzalloc() is at least as good as the size-based threshold used previously, since using kmalloc() is going to be faster than vmalloc() and would work better on 32-bit platforms with limited vmalloc() size.
Cheers, Andreas
Signature · 4 lines
Andreas Dilger Lustre Software Architect Intel High Performance Data Division
PATCH 2/12 staging: lustre: fld: Use !x to check for kzalloc failure
In reply to Julia Lawall!x is more normal for kzalloc failure in the kernel.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x;
statement S1, S2;
@@
x = kzalloc(...);
if (
- x = NULL
+ !x
) S1 else S2
// </smpl>
--- drivers/staging/lustre/lustre/fld/fld_cache.c | 2 +- drivers/staging/lustre/lustre/fld/fld_request.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-)
drivers/staging/lustre/lustre/fld/fld_request.c +1 -1
@@ -222,7 +222,7 @@ int fld_client_add_target(struct lu_clie fld->lcf_name, name, tar->ft_idx); target =kzalloc (sizeof (*target),GFP_NOFS ); -if (target =NULL ) +if (!target)return -ENOMEM ;spin_lock (&fld->lcf_lock);
drivers/staging/lustre/lustre/fld/fld_cache.c +1 -1
@@ -70,7 +70,7 @@ struct fld_cache *fld_cache_init(const cLASSERT (cache_threshold < cache_size); cache =kzalloc (sizeof (*cache),GFP_NOFS ); -if (cache =NULL ) +if (!cache)return ERR_PTR (-ENOMEM );INIT_LIST_HEAD (&cache->fci_entries_head);
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
PATCH 3/12 staging: lustre: lclient: Use !x to check for kzalloc failure
In reply to Julia Lawall!x is more normal for kzalloc failure in the kernel.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x;
statement S1, S2;
@@
x = kzalloc(...);
if (
- x = NULL
+ !x
) S1 else S2
// </smpl>
--- drivers/staging/lustre/lustre/lclient/lcommon_cl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/staging/lustre/lustre/lclient/lcommon_cl.c +1 -1
@@ -203,7 +203,7 @@ struct lu_device *ccc_device_alloc(constint rc; vdv =kzalloc (sizeof (*vdv),GFP_NOFS ); -if (vdv =NULL ) +if (!vdv)return ERR_PTR (-ENOMEM ); lud = &vdv->cdv_cl.cd_lu_dev;
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
PATCH 4/12 staging: lustre: ldlm: Use !x to check for kzalloc failure
In reply to Julia Lawall!x is more normal for kzalloc failure in the kernel.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x;
statement S1, S2;
@@
x = kzalloc(...);
if (
- x = NULL
+ !x
) S1 else S2
// </smpl>
--- drivers/staging/lustre/lustre/ldlm/ldlm_lock.c | 4 ++-- drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c | 8 ++++---- drivers/staging/lustre/lustre/ldlm/ldlm_pool.c | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-)
drivers/staging/lustre/lustre/ldlm/ldlm_pool.c +1 -1
@@ -1422,7 +1422,7 @@ static int ldlm_pools_thread_start(void)return -EALREADY ; ldlm_pools_thread =kzalloc (sizeof (*ldlm_pools_thread),GFP_NOFS ); -if (ldlm_pools_thread =NULL ) +if (!ldlm_pools_thread)return -ENOMEM ;init_completion (&ldlm_pools_comp);
drivers/staging/lustre/lustre/ldlm/ldlm_lockd.c +4 -4
@@ -225,7 +225,7 @@ static void ldlm_handle_cp_callback(struvoid *lvb_data; lvb_data =kzalloc (lvb_len,GFP_NOFS ); -if (lvb_data =NULL ) { +if (!lvb_data) {LDLM_ERROR (lock,"No memory: %d.\n" , lvb_len); rc = -ENOMEM ; goto out;
@@ -453,7 +453,7 @@ static int ldlm_bl_to_thread(struct ldlmstruct ldlm_bl_work_item *blwi; blwi =kzalloc (sizeof (*blwi),GFP_NOFS ); -if (blwi =NULL ) +if (!blwi)return -ENOMEM ;init_blwi (blwi, ns, ld, cancels, count, lock, cancel_flags);
@@ -1053,7 +1053,7 @@ static int ldlm_setup(void)return -EALREADY ; ldlm_state =kzalloc (sizeof (*ldlm_state),GFP_NOFS ); -if (ldlm_state =NULL ) +if (!ldlm_state)return -ENOMEM ; ldlm_kobj =kobject_create_and_add ("ldlm" , lustre_kobj);
@@ -1123,7 +1123,7 @@ static int ldlm_setup(void) blp =kzalloc (sizeof (*blp),GFP_NOFS ); -if (blp =NULL ) { +if (!blp) { rc = -ENOMEM ; goto out; }
drivers/staging/lustre/lustre/ldlm/ldlm_lock.c +2 -2
@@ -1528,7 +1528,7 @@ struct ldlm_lock *ldlm_lock_create(strucif (lvb_len) { lock->l_lvb_len = lvb_len; lock->l_lvb_data =kzalloc (lvb_len,GFP_NOFS ); -if (lock->l_lvb_data =NULL ) +if (!lock->l_lvb_data) goto out; }
@@ -1813,7 +1813,7 @@ int ldlm_run_ast_work(struct ldlm_namespreturn 0 ; arg =kzalloc (sizeof (*arg),GFP_NOFS ); -if (arg =NULL ) +if (!arg)return -ENOMEM ;atomic_set (&arg->restart,0 );
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
PATCH 5/12 staging: lustre: lmv: Use !x to check for kzalloc failure
In reply to Julia Lawall!x is more normal for kzalloc failure in the kernel.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x;
statement S1, S2;
@@
x = kzalloc(...);
if (
- x = NULL
+ !x
) S1 else S2
// </smpl>
--- drivers/staging/lustre/lustre/lmv/lmv_intent.c | 2 +- drivers/staging/lustre/lustre/lmv/lmv_obd.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-)
drivers/staging/lustre/lustre/lmv/lmv_obd.c +3 -3
@@ -716,7 +716,7 @@ repeat_fid2path:if (remote_gf =NULL ) { remote_gf_size =sizeof (*remote_gf) +PATH_MAX ; remote_gf =kzalloc (remote_gf_size,GFP_NOFS ); -if (remote_gf =NULL ) { +if (!remote_gf) { rc = -ENOMEM ; goto out_fid2path; }
@@ -1398,7 +1398,7 @@ static int lmv_statfs(const struct lu_enreturn rc; temp =kzalloc (sizeof (*temp),GFP_NOFS ); -if (temp =NULL ) +if (!temp)return -ENOMEM ;for (i =0 ; i < lmv->desc.ld_tgt_count; i++) {
@@ -1730,7 +1730,7 @@ lmv_enqueue_remote(struct obd_export *ex } rdata =kzalloc (sizeof (*rdata),GFP_NOFS ); -if (rdata =NULL ) { +if (!rdata) { rc = -ENOMEM ; goto out; }
drivers/staging/lustre/lustre/lmv/lmv_intent.c +1 -1
@@ -100,7 +100,7 @@ static int lmv_intent_remote(struct obd_ } op_data =kzalloc (sizeof (*op_data),GFP_NOFS ); -if (op_data =NULL ) { +if (!op_data) { rc = -ENOMEM ; goto out; }
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
PATCH 6/12 staging: lustre: lov: Use !x to check for kzalloc failure
In reply to Julia Lawall!x is more normal for kzalloc failure in the kernel.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x;
statement S1, S2;
@@
x = kzalloc(...);
if (
- x = NULL
+ !x
) S1 else S2
// </smpl>
--- drivers/staging/lustre/lustre/lov/lov_dev.c | 2 +- drivers/staging/lustre/lustre/lov/lov_io.c | 2 +- drivers/staging/lustre/lustre/lov/lov_obd.c | 2 +- drivers/staging/lustre/lustre/lov/lov_pool.c | 2 +- drivers/staging/lustre/lustre/lov/lov_request.c | 18 +++++++++--------- 5 files changed, 13 insertions(+), 13 deletions(-)
drivers/staging/lustre/lustre/lov/lov_request.c +9 -9
@@ -275,7 +275,7 @@ int lov_prep_getattr_set(struct obd_expoint rc =0 , i; set =kzalloc (sizeof (*set),GFP_NOFS ); -if (set =NULL ) +if (!set)return -ENOMEM ;lov_init_set (set);
@@ -301,7 +301,7 @@ int lov_prep_getattr_set(struct obd_expo } req =kzalloc (sizeof (*req),GFP_NOFS ); -if (req =NULL ) { +if (!req) { rc = -ENOMEM ; goto out_set; }
@@ -358,7 +358,7 @@ int lov_prep_destroy_set(struct obd_expoint rc =0 , i; set =kzalloc (sizeof (*set),GFP_NOFS ); -if (set =NULL ) +if (!set)return -ENOMEM ;lov_init_set (set);
@@ -384,7 +384,7 @@ int lov_prep_destroy_set(struct obd_expo } req =kzalloc (sizeof (*req),GFP_NOFS ); -if (req =NULL ) { +if (!req) { rc = -ENOMEM ; goto out_set; }
@@ -477,7 +477,7 @@ int lov_prep_setattr_set(struct obd_expoint rc =0 , i; set =kzalloc (sizeof (*set),GFP_NOFS ); -if (set =NULL ) +if (!set)return -ENOMEM ;lov_init_set (set);
@@ -500,7 +500,7 @@ int lov_prep_setattr_set(struct obd_expo } req =kzalloc (sizeof (*req),GFP_NOFS ); -if (req =NULL ) { +if (!req) { rc = -ENOMEM ; goto out_set; }
@@ -704,7 +704,7 @@ int lov_prep_statfs_set(struct obd_devicint rc =0 , i; set =kzalloc (sizeof (*set),GFP_NOFS ); -if (set =NULL ) +if (!set)return -ENOMEM ;lov_init_set (set);
@@ -730,14 +730,14 @@ int lov_prep_statfs_set(struct obd_devic } req =kzalloc (sizeof (*req),GFP_NOFS ); -if (req =NULL ) { +if (!req) { rc = -ENOMEM ; goto out_set; } req->rq_oi.oi_osfs =kzalloc (sizeof (*req->rq_oi.oi_osfs),GFP_NOFS ); -if (req->rq_oi.oi_osfs =NULL ) { +if (!req->rq_oi.oi_osfs) {kfree (req); rc = -ENOMEM ; goto out_set;
drivers/staging/lustre/lustre/lov/lov_pool.c +1 -1
@@ -431,7 +431,7 @@ int lov_pool_new(struct obd_device *obd,return -ENAMETOOLONG ; new_pool =kzalloc (sizeof (*new_pool),GFP_NOFS ); -if (new_pool =NULL ) +if (!new_pool)return -ENOMEM ;strncpy (new_pool->pool_name, poolname,LOV_MAXPOOLNAME );
drivers/staging/lustre/lustre/lov/lov_obd.c +1 -1
@@ -976,7 +976,7 @@ static int lov_recreate(struct obd_expor src_oa->o_flags & OBD_FL_RECREATE_OBJS); obj_mdp =kzalloc (sizeof (*obj_mdp),GFP_NOFS ); -if (obj_mdp =NULL ) +if (!obj_mdp)return -ENOMEM ; ost_idx = src_oa->o_nlink;
drivers/staging/lustre/lustre/lov/lov_io.c +1 -1
@@ -181,7 +181,7 @@ static int lov_io_sub_init(const struct }else { sub->sub_io =kzalloc (sizeof (*sub->sub_io),GFP_NOFS ); -if (sub->sub_io =NULL ) +if (!sub->sub_io) result = -ENOMEM ; } }
drivers/staging/lustre/lustre/lov/lov_dev.c +1 -1
@@ -478,7 +478,7 @@ static struct lu_device *lov_device_alloint rc; ld =kzalloc (sizeof (*ld),GFP_NOFS ); -if (ld =NULL ) +if (!ld)return ERR_PTR (-ENOMEM );cl_device_init (&ld->ld_cl, t);
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
PATCH 7/12 staging: lustre: mdc: Use !x to check for kzalloc failure
In reply to Julia Lawall!x is more normal for kzalloc failure in the kernel.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x;
statement S1, S2;
@@
x = kzalloc(...);
if (
- x = NULL
+ !x
) S1 else S2
// </smpl>
--- drivers/staging/lustre/lustre/mdc/mdc_request.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/staging/lustre/lustre/mdc/mdc_request.c +3 -3
@@ -1202,7 +1202,7 @@ static int mdc_ioc_fid2path(struct obd_e/* Key is KEY_FID2PATH + getinfo_fid2path description */ keylen =cfs_size_round (sizeof (KEY_FID2PATH )) +sizeof (*gf); key =kzalloc (keylen,GFP_NOFS ); -if (key =NULL ) +if (!key)return -ENOMEM ;memcpy (key,KEY_FID2PATH ,sizeof (KEY_FID2PATH ));memcpy (key +cfs_size_round (sizeof (KEY_FID2PATH )), gf,sizeof (*gf));
@@ -1605,7 +1605,7 @@ static int mdc_changelog_send_thread(voi cs->cs_fp, cs->cs_startrec); cs->cs_buf =kzalloc (KUC_CHANGELOG_MSG_MAXSIZE ,GFP_NOFS ); -if (cs->cs_buf =NULL ) { +if (!cs->cs_buf) { rc = -ENOMEM ; goto out; }
@@ -1934,7 +1934,7 @@ static int mdc_iocontrol(unsigned int cmstruct obd_quotactl *oqctl; oqctl =kzalloc (sizeof (*oqctl),GFP_NOFS ); -if (oqctl =NULL ) { +if (!oqctl) { rc = -ENOMEM ; goto out; }
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
PATCH 8/12 staging: lustre: mgc: Use !x to check for kzalloc failure
In reply to Julia Lawall!x is more normal for kzalloc failure in the kernel.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x;
statement S1, S2;
@@
x = kzalloc(...);
if (
- x = NULL
+ !x
) S1 else S2
// </smpl>
--- drivers/staging/lustre/lustre/mgc/mgc_request.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/staging/lustre/lustre/mgc/mgc_request.c +2 -2
@@ -1128,7 +1128,7 @@ static int mgc_apply_recover_logs(structLASSERT (cfg->cfg_sb = cfg->cfg_instance); inst =kzalloc (PAGE_CACHE_SIZE ,GFP_NOFS ); -if (inst =NULL ) +if (!inst)return -ENOMEM ;if (!IS_SERVER (lsi)) {
@@ -1493,7 +1493,7 @@ static int mgc_process_cfg_log(struct ob lsi =s2lsi (cld->cld_cfg.cfg_sb); env =kzalloc (sizeof (*env),GFP_NOFS ); -if (env =NULL ) +if (!env)return -ENOMEM ; rc =lu_env_init (env,LCT_MG_THREAD );
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
PATCH 9/12 staging: lustre: obdclass: Use !x to check for kzalloc failure
In reply to Julia Lawall!x is more normal for kzalloc failure in the kernel.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x;
statement S1, S2;
@@
x = kzalloc(...);
if (
- x = NULL
+ !x
) S1 else S2
// </smpl>
--- drivers/staging/lustre/lustre/obdclass/class_obd.c | 2 +- drivers/staging/lustre/lustre/obdclass/genops.c | 6 +++--- drivers/staging/lustre/lustre/obdclass/llog.c | 6 +++--- drivers/staging/lustre/lustre/obdclass/lprocfs_status.c | 2 +- drivers/staging/lustre/lustre/obdclass/lustre_peer.c | 2 +- drivers/staging/lustre/lustre/obdclass/obd_config.c | 10 +++++----- drivers/staging/lustre/lustre/obdclass/obd_mount.c | 12 ++++++------ 7 files changed, 20 insertions(+), 20 deletions(-)
drivers/staging/lustre/lustre/obdclass/obd_mount.c +6 -6
@@ -85,7 +85,7 @@ int lustre_process_log(struct super_blocLASSERT (cfg); bufs =kzalloc (sizeof (*bufs),GFP_NOFS ); -if (bufs =NULL ) +if (!bufs)return -ENOMEM ;/* mgc_process_config */
@@ -258,7 +258,7 @@ int lustre_start_mgc(struct super_block mgssec = lsi->lsi_lmd->lmd_mgssec ? lsi->lsi_lmd->lmd_mgssec :"" ; data =kzalloc (sizeof (*data),GFP_NOFS ); -if (data =NULL ) { +if (!data) { rc = -ENOMEM ; goto out_free; }
@@ -885,7 +885,7 @@ static int lmd_parse_mgssec(struct lustr length = tail - ptr; lmd->lmd_mgssec =kzalloc (length +1 ,GFP_NOFS ); -if (lmd->lmd_mgssec =NULL ) +if (!lmd->lmd_mgssec)return -ENOMEM ;memcpy (lmd->lmd_mgssec, ptr, length);
@@ -911,7 +911,7 @@ static int lmd_parse_string(char **handl length = tail - ptr; *handle =kzalloc (length +1 ,GFP_NOFS ); -if (*handle =NULL ) +if (!*handle)return -ENOMEM ;memcpy (*handle, ptr, length);
@@ -941,7 +941,7 @@ static int lmd_parse_mgs(struct lustre_m oldlen =strlen (lmd->lmd_mgs) +1 ; mgsnid =kzalloc (oldlen + length +1 ,GFP_NOFS ); -if (mgsnid =NULL ) +if (!mgsnid)return -ENOMEM ;if (lmd->lmd_mgs !=NULL ) {
@@ -983,7 +983,7 @@ static int lmd_parse(char *options, stru lmd->lmd_magic =LMD_MAGIC ; lmd->lmd_params =kzalloc (4096 ,GFP_NOFS ); -if (lmd->lmd_params =NULL ) +if (!lmd->lmd_params)return -ENOMEM ; lmd->lmd_params[0 ] ='\0' ;
drivers/staging/lustre/lustre/obdclass/obd_config.c +5 -5
@@ -835,7 +835,7 @@ int class_add_profile(int proflen, charCDEBUG (D_CONFIG ,"Add profile %s\n" , prof); lprof =kzalloc (sizeof (*lprof),GFP_NOFS ); -if (lprof =NULL ) +if (!lprof)return -ENOMEM ;INIT_LIST_HEAD (&lprof->lp_list);
@@ -979,7 +979,7 @@ struct lustre_cfg *lustre_cfg_rename(str new_len =LUSTRE_CFG_BUFLEN (cfg,1 ) +strlen (new_name) - name_len; new_param =kzalloc (new_len,GFP_NOFS ); -if (new_param =NULL ) +if (!new_param)return ERR_PTR (-ENOMEM );strcpy (new_param, new_name);
@@ -987,7 +987,7 @@ struct lustre_cfg *lustre_cfg_rename(strstrcat (new_param, value); bufs =kzalloc (sizeof (*bufs),GFP_NOFS ); -if (bufs =NULL ) { +if (!bufs) {kfree (new_param);return ERR_PTR (-ENOMEM ); }
@@ -1461,7 +1461,7 @@ int class_config_llog_handler(const stru inst_len =LUSTRE_CFG_BUFLEN (lcfg,0 ) +sizeof (clli->cfg_instance) *2 +4 ; inst_name =kzalloc (inst_len,GFP_NOFS ); -if (inst_name =NULL ) { +if (!inst_name) { rc = -ENOMEM ; goto out; }
@@ -1639,7 +1639,7 @@ int class_config_dump_handler(const struint rc =0 ; outstr =kzalloc (256 ,GFP_NOFS ); -if (outstr =NULL ) +if (!outstr)return -ENOMEM ;if (rec->lrh_type =OBD_CFG_REC ) {
drivers/staging/lustre/lustre/obdclass/lustre_peer.c +1 -1
@@ -105,7 +105,7 @@ int class_add_uuid(const char *uuid, __ureturn -EOVERFLOW ; data =kzalloc (sizeof (*data),GFP_NOFS ); -if (data =NULL ) +if (!data)return -ENOMEM ;obd_str2uuid (&data->un_uuid, uuid);
drivers/staging/lustre/lustre/obdclass/lprocfs_status.c +1 -1
@@ -275,7 +275,7 @@ struct dentry *ldebugfs_add_symlink(consreturn NULL ; dest =kzalloc (MAX_STRING_SIZE +1 ,GFP_KERNEL ); -if (dest =NULL ) +if (!dest)return NULL ;va_start (ap, format);
drivers/staging/lustre/lustre/obdclass/llog.c +3 -3
@@ -61,7 +61,7 @@ static struct llog_handle *llog_alloc_hastruct llog_handle *loghandle; loghandle =kzalloc (sizeof (*loghandle),GFP_NOFS ); -if (loghandle =NULL ) +if (!loghandle)return NULL ;init_rwsem (&loghandle->lgh_lock);
@@ -208,7 +208,7 @@ int llog_init_handle(const struct lu_envLASSERT (handle->lgh_hdr =NULL ); llh =kzalloc (sizeof (*llh),GFP_NOFS ); -if (llh =NULL ) +if (!llh)return -ENOMEM ; handle->lgh_hdr = llh;/* first assign flags to use llog_client_ops */
@@ -435,7 +435,7 @@ int llog_process_or_fork(const struct luint rc; lpi =kzalloc (sizeof (*lpi),GFP_NOFS ); -if (lpi =NULL ) { +if (!lpi) {CERROR ("cannot alloc pointer\n" );return -ENOMEM ; }
drivers/staging/lustre/lustre/obdclass/genops.c +3 -3
@@ -172,7 +172,7 @@ int class_register_type(struct obd_ops * rc = -ENOMEM ; type =kzalloc (sizeof (*type),GFP_NOFS ); -if (type =NULL ) +if (!type)return rc; type->typ_dt_ops =kzalloc (sizeof (*type->typ_dt_ops),GFP_NOFS );
@@ -1016,7 +1016,7 @@ struct obd_import *class_new_import(strustruct obd_import *imp; imp =kzalloc (sizeof (*imp),GFP_NOFS ); -if (imp =NULL ) +if (!imp)return NULL ;INIT_LIST_HEAD (&imp->imp_pinger_chain);
@@ -1819,7 +1819,7 @@ void *kuc_alloc(int payload_len, int traint len =kuc_len (payload_len); lh =kzalloc (len,GFP_NOFS ); -if (lh =NULL ) +if (!lh)return ERR_PTR (-ENOMEM ); lh->kuc_magic =KUC_MAGIC ;
drivers/staging/lustre/lustre/obdclass/class_obd.c +1 -1
@@ -232,7 +232,7 @@ int class_handle_ioctl(unsigned int cmd, goto out; } lcfg =kzalloc (data->ioc_plen1,GFP_NOFS ); -if (lcfg =NULL ) { +if (!lcfg) { err = -ENOMEM ; goto out; }
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
Re: [PATCH 09/12] staging: lustre: obdclass: Use !x to check for kzalloc failure
Am 20.06.2015 18:59, schrieb Julia Lawall:
Quoted 52 lines
!x is more normal for kzalloc failure in the kernel.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)// <smpl>
@@
expression x;
statement S1, S2;
@@x = kzalloc(...);
if (
- x = NULL
+ !x
) S1 else S2
// </smpl>Signed-off-by Julia Lawall <[email protected]>--- drivers/staging/lustre/lustre/obdclass/class_obd.c | 2 +- drivers/staging/lustre/lustre/obdclass/genops.c | 6 +++--- drivers/staging/lustre/lustre/obdclass/llog.c | 6 +++--- drivers/staging/lustre/lustre/obdclass/lprocfs_status.c | 2 +- drivers/staging/lustre/lustre/obdclass/lustre_peer.c | 2 +- drivers/staging/lustre/lustre/obdclass/obd_config.c | 10 +++++----- drivers/staging/lustre/lustre/obdclass/obd_mount.c | 12 ++++++------ 7 files changed, 20 insertions(+), 20 deletions(-)diff -u -p
a/drivers/staging/lustre/lustre/obdclass/obd_mount.cb/drivers/staging/lustre/lustre/obdclass/obd_mount.cdrivers/staging/lustre/lustre/obdclass/obd_mount.c +2
-2@@ -85,7 +85,7 @@ int lustre_process_log(struct super_blocLASSERT (cfg); bufs =kzalloc (sizeof (*bufs),GFP_NOFS ); -if (bufs =NULL ) +if (!bufs)return -ENOMEM ;/* mgc_process_config */ @@ -258,7 +258,7 @@ int lustre_start_mgc(struct super_block mgssec = lsi->lsi_lmd->lmd_mgssec ? lsi->lsi_lmd->lmd_mgssec :"" ; data =kzalloc (sizeof (*data),GFP_NOFS ); -if (data =NULL ) { +if (!data) { rc = -ENOMEM ; goto out_free; }
drivers/staging/lustre/lustre/obdclass/obd_mount.c static int lmd_parse_mgssec(struct lustr · line 891
Quoted 8 lines
drivers/staging/lustre/lustre/obdclass/obd_mount.c +1
-1@@ -885,7 +885,7 @@ static int lmd_parse_mgssec(struct lustr length = tail - ptr; lmd->lmd_mgssec =kzalloc (length +1 ,GFP_NOFS ); -if (lmd->lmd_mgssec =NULL ) +if (!lmd->lmd_mgssec)return -ENOMEM ;memcpy (lmd->lmd_mgssec, ptr, length);
looks like memdup()
drivers/staging/lustre/lustre/obdclass/obd_mount.c static int lmd_parse_string(char **handl · line 917
Quoted 9 lines
drivers/staging/lustre/lustre/obdclass/obd_mount.c +1
-1@@ -911,7 +911,7 @@ static int lmd_parse_string(char **handl length = tail - ptr; *handle =kzalloc (length +1 ,GFP_NOFS ); -if (*handle =NULL ) +if (!*handle)return -ENOMEM ;memcpy (*handle, ptr, length);
looks like memdup()
Quoted 164 lines
drivers/staging/lustre/lustre/obdclass/obd_mount.c +2
-2@@ -941,7 +941,7 @@ static int lmd_parse_mgs(struct lustre_m oldlen =strlen (lmd->lmd_mgs) +1 ; mgsnid =kzalloc (oldlen + length +1 ,GFP_NOFS ); -if (mgsnid =NULL ) +if (!mgsnid)return -ENOMEM ;if (lmd->lmd_mgs !=NULL ) {@@ -983,7 +983,7 @@ static int lmd_parse(char *options, stru lmd->lmd_magic =LMD_MAGIC ; lmd->lmd_params =kzalloc (4096 ,GFP_NOFS ); -if (lmd->lmd_params =NULL ) +if (!lmd->lmd_params)return -ENOMEM ; lmd->lmd_params[0 ] ='\0' ;diff -u -p
a/drivers/staging/lustre/lustre/obdclass/obd_config.cb/drivers/staging/lustre/lustre/obdclass/obd_config.cdrivers/staging/lustre/lustre/obdclass/obd_config.c +5
-5@@ -835,7 +835,7 @@ int class_add_profile(int proflen, charCDEBUG (D_CONFIG ,"Add profile %s\n" , prof); lprof =kzalloc (sizeof (*lprof),GFP_NOFS ); -if (lprof =NULL ) +if (!lprof)return -ENOMEM ;INIT_LIST_HEAD (&lprof->lp_list);@@ -979,7 +979,7 @@ struct lustre_cfg *lustre_cfg_rename(str new_len =LUSTRE_CFG_BUFLEN (cfg,1 ) +strlen (new_name) - name_len; new_param =kzalloc (new_len,GFP_NOFS ); -if (new_param =NULL ) +if (!new_param)return ERR_PTR (-ENOMEM );strcpy (new_param, new_name);@@ -987,7 +987,7 @@ struct lustre_cfg *lustre_cfg_rename(strstrcat (new_param, value); bufs =kzalloc (sizeof (*bufs),GFP_NOFS ); -if (bufs =NULL ) { +if (!bufs) {kfree (new_param);return ERR_PTR (-ENOMEM ); }@@ -1461,7 +1461,7 @@ int class_config_llog_handler(const stru inst_len =LUSTRE_CFG_BUFLEN (lcfg,0 ) +sizeof (clli->cfg_instance) *2 +4 ; inst_name =kzalloc (inst_len,GFP_NOFS ); -if (inst_name =NULL ) { +if (!inst_name) { rc = -ENOMEM ; goto out; }@@ -1639,7 +1639,7 @@ int class_config_dump_handler(const struint rc =0 ; outstr =kzalloc (256 ,GFP_NOFS ); -if (outstr =NULL ) +if (!outstr)return -ENOMEM ;if (rec->lrh_type =OBD_CFG_REC ) {diff -u -p
a/drivers/staging/lustre/lustre/obdclass/lustre_peer.cb/drivers/staging/lustre/lustre/obdclass/lustre_peer.cdrivers/staging/lustre/lustre/obdclass/lustre_peer.c +1
-1@@ -105,7 +105,7 @@ int class_add_uuid(const char *uuid, __ureturn -EOVERFLOW ; data =kzalloc (sizeof (*data),GFP_NOFS ); -if (data =NULL ) +if (!data)return -ENOMEM ;obd_str2uuid (&data->un_uuid, uuid);diff -u -p
a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.cb/drivers/staging/lustre/lustre/obdclass/lprocfs_status.cdrivers/staging/lustre/lustre/obdclass/lprocfs_status.c +1
-1@@ -275,7 +275,7 @@ struct dentry *ldebugfs_add_symlink(consreturn NULL ; dest =kzalloc (MAX_STRING_SIZE +1 ,GFP_KERNEL ); -if (dest =NULL ) +if (!dest)return NULL ;va_start (ap, format);diff -u -p
a/drivers/staging/lustre/lustre/obdclass/llog.cb/drivers/staging/lustre/lustre/obdclass/llog.cdrivers/staging/lustre/lustre/obdclass/llog.c +3
-3@@ -61,7 +61,7 @@ static struct llog_handle *llog_alloc_hastruct llog_handle *loghandle; loghandle =kzalloc (sizeof (*loghandle),GFP_NOFS ); -if (loghandle =NULL ) +if (!loghandle)return NULL ;init_rwsem (&loghandle->lgh_lock);@@ -208,7 +208,7 @@ int llog_init_handle(const struct lu_envLASSERT (handle->lgh_hdr =NULL ); llh =kzalloc (sizeof (*llh),GFP_NOFS ); -if (llh =NULL ) +if (!llh)return -ENOMEM ; handle->lgh_hdr = llh;/* first assign flags to use llog_client_ops */ @@ -435,7 +435,7 @@ int llog_process_or_fork(const struct luint rc; lpi =kzalloc (sizeof (*lpi),GFP_NOFS ); -if (lpi =NULL ) { +if (!lpi) {CERROR ("cannot alloc pointer\n" );return -ENOMEM ; }diff -u -p
a/drivers/staging/lustre/lustre/obdclass/genops.cb/drivers/staging/lustre/lustre/obdclass/genops.cdrivers/staging/lustre/lustre/obdclass/genops.c +3
-3@@ -172,7 +172,7 @@ int class_register_type(struct obd_ops * rc = -ENOMEM ; type =kzalloc (sizeof (*type),GFP_NOFS ); -if (type =NULL ) +if (!type)return rc; type->typ_dt_ops =kzalloc (sizeof (*type->typ_dt_ops),GFP_NOFS );@@ -1016,7 +1016,7 @@ struct obd_import *class_new_import(strustruct obd_import *imp; imp =kzalloc (sizeof (*imp),GFP_NOFS ); -if (imp =NULL ) +if (!imp)return NULL ;INIT_LIST_HEAD (&imp->imp_pinger_chain);@@ -1819,7 +1819,7 @@ void *kuc_alloc(int payload_len, int traint len =kuc_len (payload_len); lh =kzalloc (len,GFP_NOFS ); -if (lh =NULL ) +if (!lh)return ERR_PTR (-ENOMEM ); lh->kuc_magic =KUC_MAGIC ;diff -u -p
a/drivers/staging/lustre/lustre/obdclass/class_obd.cb/drivers/staging/lustre/lustre/obdclass/class_obd.cdrivers/staging/lustre/lustre/obdclass/class_obd.c +1
-2@@ -232,7 +232,7 @@ int class_handle_ioctl(unsigned int cmd, goto out; } lcfg =kzalloc (data->ioc_plen1,GFP_NOFS ); -if (lcfg =NULL ) { +if (!lcfg) { err = -ENOMEM ; goto out; } --
Quoted 2 lines
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
Re: [PATCH 09/12] staging: lustre: obdclass: Use !x to check for kzalloc failure
Quoted 10 lines
drivers/staging/lustre/lustre/obdclass/obd_mount.c static int lmd_parse_mgssec(struct lustr · line 891
Quoted 9 lines
drivers/staging/lustre/lustre/obdclass/obd_mount.c +1
-1@@ -885,7 +885,7 @@ static int lmd_parse_mgssec(struct lustr length = tail - ptr; lmd->lmd_mgssec =kzalloc (length +1 ,GFP_NOFS ); -if (lmd->lmd_mgssec =NULL ) +if (!lmd->lmd_mgssec)return -ENOMEM ;memcpy (lmd->lmd_mgssec, ptr, length);looks like
memdup()
kmemdup has the same length for both calls. There is kstrndup, but it recalculates the length, which looks unnecessary here.
julia
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
Re: [PATCH 09/12] staging: lustre: obdclass: Use !x to check for kzalloc failure
In reply to Julia LawallAm 21.06.2015 12:29, schrieb Julia Lawall:
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/staging/lustre/lustre/obdclass/obd_mount.c +1
-1@@ -885,7 +885,7 @@ static int lmd_parse_mgssec(struct lustr length = tail - ptr; lmd->lmd_mgssec =kzalloc (length +1 ,GFP_NOFS ); -if (lmd->lmd_mgssec =NULL ) +if (!lmd->lmd_mgssec)return -ENOMEM ;
Quoted 8 lines
Quoted 2 lines
drivers/staging/lustre/lustre/obdclass/obd_mount.c static int lmd_parse_mgssec(struct lustr · line 891
Quoted 1 line
drivers/staging/lustre/lustre/obdclass/obd_mount.c +0
-0memcpy (lmd->lmd_mgssec, ptr, length);looks like
memdup()kmemdup has the same length for both calls. There is kstrndup, but it recalculates the length, which looks unnecessary here.
julia
mmh, it seem to be use only while mount,
i am wonderingparsing a comma sperareted list must be commen problem for all FS (and others) maybe i am looking in thew wrong place, there should be some helpers available ? are they ?
re,
wh
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
PATCH 10/12 staging: lustre: obdecho: Use !x to check for kzalloc failure
In reply to Julia Lawall!x is more normal for kzalloc failure in the kernel.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x;
statement S1, S2;
@@
x = kzalloc(...);
if (
- x = NULL
+ !x
) S1 else S2
// </smpl>
--- drivers/staging/lustre/lustre/obdecho/echo_client.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/staging/lustre/lustre/obdecho/echo_client.c +5 -5
@@ -480,11 +480,11 @@ static int echo_alloc_memmd(struct echo_LASSERT (*lsmp =NULL ); *lsmp =kzalloc (lsm_size,GFP_NOFS ); -if (*lsmp =NULL ) +if (!*lsmp)return -ENOMEM ; (*lsmp)->lsm_oinfo[0 ] =kzalloc (sizeof (struct lov_oinfo ),GFP_NOFS ); -if ((*lsmp)->lsm_oinfo[0 ] =NULL ) { +if (!(*lsmp)->lsm_oinfo[0 ]) {kfree (*lsmp);return -ENOMEM ; }
@@ -701,7 +701,7 @@ static struct lu_device *echo_device_allint cleanup =0 ; ed =kzalloc (sizeof (*ed),GFP_NOFS ); -if (ed =NULL ) { +if (!ed) { rc = -ENOMEM ; goto out; }
@@ -1878,7 +1878,7 @@ echo_client_iocontrol(unsigned int cmd,return rc; env =kzalloc (sizeof (*env),GFP_NOFS ); -if (env =NULL ) +if (!env)return -ENOMEM ; rc =lu_env_init (env,LCT_DT_THREAD );
@@ -2049,7 +2049,7 @@ static int echo_client_setup(const struc ec->ec_nstripes =0 ; ocd =kzalloc (sizeof (*ocd),GFP_NOFS ); -if (ocd =NULL ) { +if (!ocd) {CERROR ("Can't alloc ocd connecting to %s\n" ,lustre_cfg_string (lcfg,1 ));return -ENOMEM ;
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
PATCH 11/12 staging: lustre: osc: Use !x to check for kzalloc failure
In reply to Julia Lawall!x is more normal for kzalloc failure in the kernel.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x;
statement S1, S2;
@@
x = kzalloc(...);
if (
- x = NULL
+ !x
) S1 else S2
// </smpl>
--- drivers/staging/lustre/lustre/osc/osc_dev.c | 2 +- drivers/staging/lustre/lustre/osc/osc_request.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
drivers/staging/lustre/lustre/osc/osc_request.c +2 -2
@@ -119,7 +119,7 @@ static int osc_packmd(struct obd_exportif (*lmmp =NULL ) { *lmmp =kzalloc (lmm_size,GFP_NOFS ); -if (*lmmp =NULL ) +if (!*lmmp)return -ENOMEM ; }
@@ -1909,7 +1909,7 @@ int osc_build_rpc(const struct lu_env *e mpflag =cfs_memory_pressure_get_and_set (); crattr =kzalloc (sizeof (*crattr),GFP_NOFS ); -if (crattr =NULL ) { +if (!crattr) { rc = -ENOMEM ; goto out; }
drivers/staging/lustre/lustre/osc/osc_dev.c +1 -1
@@ -218,7 +218,7 @@ static struct lu_device *osc_device_alloint rc; od =kzalloc (sizeof (*od),GFP_NOFS ); -if (od =NULL ) +if (!od)return ERR_PTR (-ENOMEM );cl_device_init (&od->od_cl, t);
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
PATCH 12/12 staging: lustre: ptlrpc: Use !x to check for kzalloc failure
In reply to Julia Lawall!x is more normal for kzalloc failure in the kernel.
The semantic patch that makes this change is as follows:
(http://coccinelle.lip6.fr/)
// <smpl>
@@
expression x;
statement S1, S2;
@@
x = kzalloc(...);
if (
- x = NULL
+ !x
) S1 else S2
// </smpl>
--- drivers/staging/lustre/lustre/ptlrpc/client.c | 2 +- drivers/staging/lustre/lustre/ptlrpc/lproc_ptlrpc.c | 8 ++++---- drivers/staging/lustre/lustre/ptlrpc/nrs.c | 2 +- drivers/staging/lustre/lustre/ptlrpc/ptlrpcd.c | 2 +- drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c | 2 +- drivers/staging/lustre/lustre/ptlrpc/sec_config.c | 2 +- drivers/staging/lustre/lustre/ptlrpc/sec_plain.c | 2 +- drivers/staging/lustre/lustre/ptlrpc/service.c | 4 ++-- 8 files changed, 12 insertions(+), 12 deletions(-)
drivers/staging/lustre/lustre/ptlrpc/service.c +2 -2
@@ -732,7 +732,7 @@ ptlrpc_register_service(struct ptlrpc_se service = kzalloc(offsetof(struct ptlrpc_service, srv_parts[ncpts]), GFP_NOFS); -if (service =NULL ) { +if (!service) {kfree (cpts);return ERR_PTR (-ENOMEM ); }
@@ -2298,7 +2298,7 @@ static int ptlrpc_main(void *arg) } env =kzalloc (sizeof (*env),GFP_NOFS ); -if (env =NULL ) { +if (!env) { rc = -ENOMEM ; goto out_srv_fini; }
drivers/staging/lustre/lustre/ptlrpc/sec_plain.c +1 -1
@@ -444,7 +444,7 @@ struct ptlrpc_sec *plain_create_sec(struLASSERT (SPTLRPC_FLVR_POLICY (sf->sf_rpc) =SPTLRPC_POLICY_PLAIN ); plsec =kzalloc (sizeof (*plsec),GFP_NOFS ); -if (plsec =NULL ) +if (!plsec)return NULL ;/*
drivers/staging/lustre/lustre/ptlrpc/sec_config.c +1 -1
@@ -564,7 +564,7 @@ struct sptlrpc_conf *sptlrpc_conf_get(coreturn NULL ; conf =kzalloc (sizeof (*conf),GFP_NOFS ); -if (conf =NULL ) +if (!conf)return NULL ;strcpy (conf->sc_fsname, fsname);
drivers/staging/lustre/lustre/ptlrpc/sec_bulk.c +1 -1
@@ -415,7 +415,7 @@ static int enc_pools_add_pages(int npagefor (i =0 ; i < npools; i++) { pools[i] =kzalloc (PAGE_CACHE_SIZE ,GFP_NOFS ); -if (pools[i] =NULL ) +if (!pools[i]) goto out_pools;for (j =0 ; j <PAGES_PER_POOL && alloced < npages; j++) {
drivers/staging/lustre/lustre/ptlrpc/ptlrpcd.c +1 -1
@@ -739,7 +739,7 @@ static int ptlrpcd_init(void) size = offsetof(struct ptlrpcd, pd_threads[nthreads]); ptlrpcds =kzalloc (size,GFP_NOFS ); -if (ptlrpcds =NULL ) { +if (!ptlrpcds) { rc = -ENOMEM ; goto out; }
drivers/staging/lustre/lustre/ptlrpc/nrs.c +1 -1
@@ -1156,7 +1156,7 @@ int ptlrpc_nrs_policy_register(struct pt } desc =kzalloc (sizeof (*desc),GFP_NOFS ); -if (desc =NULL ) { +if (!desc) { rc = -ENOMEM ; goto fail; }
drivers/staging/lustre/lustre/ptlrpc/lproc_ptlrpc.c +4 -4
@@ -652,7 +652,7 @@ static ssize_t ptlrpc_lprocfs_nrs_seq_wrreturn -EINVAL ; cmd =kzalloc (LPROCFS_NRS_WR_MAX_CMD ,GFP_NOFS ); -if (cmd =NULL ) +if (!cmd)return -ENOMEM ;/** * strsep() modifies its argument, so keep a copy
@@ -819,7 +819,7 @@ ptlrpc_lprocfs_svc_req_history_start(str } srhi =kzalloc (sizeof (*srhi),GFP_NOFS ); -if (srhi =NULL ) +if (!srhi)return NULL ; srhi->srhi_seq =0 ;
@@ -1219,7 +1219,7 @@ int lprocfs_wr_evict_client(struct filechar *tmpbuf; kbuf =kzalloc (BUFLEN ,GFP_NOFS ); -if (kbuf =NULL ) +if (!kbuf)return -ENOMEM ;/*
@@ -1303,7 +1303,7 @@ int lprocfs_wr_import(struct file *file,return -EINVAL ; kbuf =kzalloc (count +1 ,GFP_NOFS ); -if (kbuf =NULL ) +if (!kbuf)return -ENOMEM ;if (copy_from_user (kbuf, buffer, count)) {
drivers/staging/lustre/lustre/ptlrpc/client.c +1 -1
@@ -971,7 +971,7 @@ int ptlrpc_set_add_cb(struct ptlrpc_requstruct ptlrpc_set_cbdata *cbdata; cbdata =kzalloc (sizeof (*cbdata),GFP_NOFS ); -if (cbdata =NULL ) +if (!cbdata)return -ENOMEM ; cbdata->psc_interpret = fn;
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in