[01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

Commit Message

SF Markus Elfring Sept. 29, 2016, 9:07 a.m. UTC

From: Markus Elfring <[e-mail]>
Date: Wed, 28 Sep 2016 22:20:08 +0200

* Multiplications for the size determination of memory allocations
  indicated that array data structures should be processed.
  Thus use the corresponding function "kmalloc_array".

  This issue was detected by using the Coccinelle software.

* Replace the specification of data structures by pointer dereferences
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <[e-mail]>
---
 drivers/md/dm-snap.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Paul Bolle Sept. 29, 2016, 11:12 a.m. UTC | #1

On Thu, 2016-09-29 at 03:02 -0700, Joe Perches wrote:
> What's the false positive?
> 
> I get:
> 
> $ ./scripts/checkpatch.pl -f drivers/md/dm-snap.c --show-types --types=alloc_with_multiply
> WARNING:ALLOC_WITH_MULTIPLY: Prefer kmalloc_array over kmalloc with multiply
> #329: FILE: drivers/md/dm-snap.c:329:
> +	_origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
> 
> WARNING:ALLOC_WITH_MULTIPLY: Prefer kmalloc_array over kmalloc with multiply
> #338: FILE: drivers/md/dm-snap.c:338:
> +	_dm_origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
> 
> total: 0 errors, 2 warnings, 2490 lines checked
> 
> NOTE: For some of the reported defects, checkpatch may be able to
>       mechanically convert to the typical style using --fix or --fix-inplace.
> 
> drivers/md/dm-snap.c has style problems, please review.
> 
> NOTE: Used message types: ALLOC_WITH_MULTIPLY
> 
> NOTE: If any of the errors are false positives, please report
>       them to the maintainer, see CHECKPATCH in MAINTAINERS.

It seems it was intended to be silent about multiplying with constants,
where things that look like preprocessor defines are also considered
constants. Or did I misread that test?


Paul Bolle

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Paul Bolle Sept. 29, 2016, 11:45 a.m. UTC | #2

On Thu, 2016-09-29 at 13:12 +0200, Paul Bolle wrote:
> Or did I misread that test?

I finally did some digging: commit e367455a9f25 ("checkpatch: emit
fewer kmalloc_array/kcalloc conversion warnings") shows I didn't.


Paul Bolle

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

SF Markus Elfring Sept. 29, 2016, 11:45 a.m. UTC | #3

> We have no hope of fixing Markus' homegrown coccinelle script.

I have got an other impression. I see further possibilities
to clarify involved communication and software development challenges
for a few source code search patterns.

How do you think about to discuss the corresponding collateral evolution
a bit more?

Are there any more constraints to consider?

Regards,
Markus

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Paul Bolle Sept. 29, 2016, 7:43 p.m. UTC | #4

On Thu, 2016-09-29 at 08:01 -0700, Joe Perches wrote:
> $Constant there is any number and the match regex is
> any upper case variable.

Why doesn't that regex match on "ORIGIN_HASH_SIZE"?


Paul Bolle

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Paul Bolle Sept. 29, 2016, 8:39 p.m. UTC | #5

On Thu, 2016-09-29 at 13:24 -0700, Joe Perches wrote:
> On Thu, 2016-09-29 at 21:43 +0200, Paul Bolle wrote:
> > Why doesn't that regex match on "ORIGIN_HASH_SIZE"?
> 
> It does match.

If that regex does match, it being part of a negative test, the
specific checkpatch rule should be silent, shouldn't it?


Paul Bolle

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Paul Bolle Sept. 29, 2016, 9:14 p.m. UTC | #6

On Thu, 2016-09-29 at 13:56 -0700, Joe Perches wrote:
> It doesn't matter match either way to me.
> 
> The case for the unnecessary multiply with <= gcc 4.8 was
> removed with:
> 
> commit 91c6a05f72a996bee5133e76374ab3ad7d3b9b72
> Author: Alexey Dobriyan <[e-mail]>
> Date:   Tue Jul 26 15:22:08 2016 -0700
> 
>     mm: faster kmalloc_array(), kcalloc()
>     
>     When both arguments to kmalloc_array() or kcalloc() are known at compile
>     time then their product is known at compile time but search for kmalloc
>     cache happens at runtime not at compile time.
>     
>     Link: http://lkml.kernel.org/r/[e-mail]
>     Signed-off-by: Alexey Dobriyan <[e-mail]>
>     Cc: Christoph Lameter <[e-mail]>
>     Cc: Pekka Enberg <[e-mail]>
>     Cc: David Rientjes <[e-mail]>
>     Cc: Joonsoo Kim <[e-mail]>
>     Signed-off-by: Andrew Morton <[e-mail]>
>     Signed-off-by: Linus Torvalds <[e-mail]>

You've lost me.

Why does this stop you fixing an apparently wrong checkpatch rule,
crude as parts of it are (ie, uppercase identifier must be a constant)?


Paul Bolle

--
dm-devel mailing list
[e-mail]
https://www.redhat.com/mailman/listinfo/dm-devel

Patch

diff --git a/drivers/md/dm-snap.c b/drivers/md/dm-snap.c
index c65feea..f262f7e 100644
--- a/drivers/md/dm-snap.c
+++ b/drivers/md/dm-snap.c
@@ -326,8 +326,9 @@  static int init_origin_hash(void)
 {
 	int i;
 
-	_origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
-			   GFP_KERNEL);
+	_origins = kmalloc_array(ORIGIN_HASH_SIZE,
+				 sizeof(*_origins),
+				 GFP_KERNEL);
 	if (!_origins) {
 		DMERR("unable to allocate memory for _origins");
 		return -ENOMEM;
@@ -335,8 +336,9 @@  static int init_origin_hash(void)
 	for (i = 0; i < ORIGIN_HASH_SIZE; i++)
 		INIT_LIST_HEAD(_origins + i);
 
-	_dm_origins = kmalloc(ORIGIN_HASH_SIZE * sizeof(struct list_head),
-			      GFP_KERNEL);
+	_dm_origins = kmalloc_array(ORIGIN_HASH_SIZE,
+				    sizeof(*_dm_origins),
+				    GFP_KERNEL);
 	if (!_dm_origins) {
 		DMERR("unable to allocate memory for _dm_origins");
 		kfree(_origins);