Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1493223 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-29 11:10 +0200
    Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in  init_origin_hash() Paul Bolle <pebolle@tiscali.nl> - 2016-09-29 12:00 +0200
      Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in  init_origin_hash() Joe Perches <joe@perches.com> - 2016-09-29 12:10 +0200
        Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in  init_origin_hash() Paul Bolle <pebolle@tiscali.nl> - 2016-09-29 13:20 +0200
          Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in  init_origin_hash() Paul Bolle <pebolle@tiscali.nl> - 2016-09-29 13:50 +0200
            Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in  init_origin_hash() Joe Perches <joe@perches.com> - 2016-09-29 17:10 +0200
              Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in  init_origin_hash() Paul Bolle <pebolle@tiscali.nl> - 2016-09-29 21:50 +0200
                Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in  init_origin_hash() Joe Perches <joe@perches.com> - 2016-09-29 22:30 +0200
                  Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in  init_origin_hash() Paul Bolle <pebolle@tiscali.nl> - 2016-09-29 22:50 +0200
                    Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in  init_origin_hash() Joe Perches <joe@perches.com> - 2016-09-29 23:00 +0200
                      Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in  init_origin_hash() Paul Bolle <pebolle@tiscali.nl> - 2016-09-29 23:20 +0200
                        Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in  init_origin_hash() Joe Perches <joe@perches.com> - 2016-09-29 23:30 +0200
                          Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in  init_origin_hash() Paul Bolle <pebolle@tiscali.nl> - 2016-09-29 23:50 +0200
                            Re: dm snapshot: Use kmalloc_array() in init_origin_hash() ? SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-30 09:20 +0200
      Re: dm snapshot: Use kmalloc_array() in init_origin_hash() SF Markus Elfring <elfring@users.sourceforge.net> - 2016-09-29 13:50 +0200
        Re: dm snapshot: Use kmalloc_array() in init_origin_hash() Theodore Ts'o <tytso@mit.edu> - 2016-09-29 15:10 +0200

#1493223 — [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

From: Markus Elfring <elfring@users.sourceforge.net>
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 <elfring@users.sourceforge.net>
---
 drivers/md/dm-snap.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

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);
-- 
2.10.0

[toc] | [next] | [standalone]

#1493246 — Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

Andy, Joe,

On Thu, 2016-09-29 at 11:07 +0200, SF Markus Elfring wrote:
> * 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.

We have no hope of fixing Markus' homegrown coccinelle script. But we
could try to fix the checkpatch false positive here. Something like:

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 206a6b3..b47201d 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5693,7 +5693,7 @@ sub process {
 				$r2 = $a1;
 			}
 			if ($r1 !~ /^sizeof\b/ && $r2 =~ /^sizeof\s*\S/ &&
-			    !($r1 =~ /^$Constant$/ || $r1 =~ /^[A-Z_][A-Z0-9_]*$/)) {
+			    !($r1 =~ /^$Constant$/ || $r1 =~ /^[A-Z_][A-Z0-9_]*\b/)) {
 				if (WARN("ALLOC_WITH_MULTIPLY",
 					 "Prefer $newfunc over $oldfunc with multiply\n" . $herecurr) &&
 				    $fix) {

Does that work for you too?

> --- 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);

Thanks,


Paul Bolle

[toc] | [prev] | [next] | [standalone]

#1493253 — Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

On Thu, 2016-09-29 at 11:54 +0200, Paul Bolle wrote:
> Andy, Joe,
> 
> On Thu, 2016-09-29 at 11:07 +0200, SF Markus Elfring wrote:
> > * 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.
> 
> 
> We have no hope of fixing Markus' homegrown coccinelle script. But we
> could try to fix the checkpatch false positive here.

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.

[toc] | [prev] | [next] | [standalone]

#1493402 — Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

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

[toc] | [prev] | [next] | [standalone]

#1493417 — Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

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

[toc] | [prev] | [next] | [standalone]

#1493531 — Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

On Thu, 2016-09-29 at 13:45 +0200, Paul Bolle wrote:
> 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.

You still misread it a little.
I think it's fine as-is.

$Constant there is any number and the match regex is
any upper case variable.

[toc] | [prev] | [next] | [standalone]

#1493698 — Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

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

[toc] | [prev] | [next] | [standalone]

#1493705 — Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

On Thu, 2016-09-29 at 21:43 +0200, Paul Bolle wrote:
> 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"?

It does match.

Did you see my earlier email?

$ ./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

[toc] | [prev] | [next] | [standalone]

#1493713 — Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

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

[toc] | [prev] | [next] | [standalone]

#1493718 — Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

On Thu, 2016-09-29 at 22:39 +0200, Paul Bolle wrote:
> 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?

'cause I forgot to trim() the original $4 and $10 matches.

Oh well.

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 <adobriyan@gmail.com>
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/20160627213454.GA2440@p183.telecom.by
    Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
    Cc: Christoph Lameter <cl@linux.com>
    Cc: Pekka Enberg <penberg@kernel.org>
    Cc: David Rientjes <rientjes@google.com>
    Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

[toc] | [prev] | [next] | [standalone]

#1493727 — Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

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 <adobriyan@gmail.com>
> 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/20160627213454.GA2440@p183.telecom.by
>     Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>     Cc: Christoph Lameter <cl@linux.com>
>     Cc: Pekka Enberg <penberg@kernel.org>
>     Cc: David Rientjes <rientjes@google.com>
>     Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
>     Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

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

[toc] | [prev] | [next] | [standalone]

#1493729 — Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

On Thu, 2016-09-29 at 23:14 +0200, Paul Bolle wrote:
> On Thu, 2016-09-29 at 13:56 -0700, Joe Perches wrote:
> > It doesn't matter match either way to 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)?

It doesn't.  It just doesn't matter much (match) to me.
Here:
---
 scripts/checkpatch.pl | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 3373c65fef1c..fc931d89152e 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -5835,8 +5835,8 @@ sub process {
 		if ($^V && $^V ge 5.10.0 &&
 		    $line =~ /\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/) {
 			my $oldfunc = $3;
-			my $a1 = $4;
-			my $a2 = $10;
+			my $a1 = trim($4);
+			my $a2 = trim($10);
 			my $newfunc = "kmalloc_array";
 			$newfunc = "kcalloc" if ($oldfunc eq "kzalloc");
 			my $r1 = $a1;
@@ -5850,7 +5850,7 @@ sub process {
 				if (WARN("ALLOC_WITH_MULTIPLY",
 					 "Prefer $newfunc over $oldfunc with multiply\n" . $herecurr) &&
 				    $fix) {
-					$fixed[$fixlinenr] =~ s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1 . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
+					$fixed[$fixlinenr] =~ s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(k[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1 . ' = ' . "$newfunc(" . $r1 . ', ' . $r2/e;
 
 				}
 			}

[toc] | [prev] | [next] | [standalone]

#1493738 — Re: [PATCH 01/10] dm snapshot: Use kmalloc_array() in init_origin_hash()

On Thu, 2016-09-29 at 14:21 -0700, Joe Perches wrote:
> > > It doesn't matter match either way to 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)?
> 
> It doesn't.  It just doesn't matter much (match) to me.

Joe, please.

I've recently ping-ponged with the kernel's "resident wrong bot of the
day" over this very rule (kmalloc_array() is safer than kmalloc(), so
change your driver now!). Could we just give wrong bots a bit less
ammunition whenever that's feasible?

Even if you don't care about my ping-pong experiences: this checkpatch
test is broken, please just fix it!


Paul Bolle

[toc] | [prev] | [next] | [standalone]

#1493923 — Re: dm snapshot: Use kmalloc_array() in init_origin_hash() ?

> I've recently ping-ponged with the kernel's "resident wrong bot of the
> day" over this very rule (kmalloc_array() is safer than kmalloc(), so
> change your driver now!).

Your bot of the day is going to point more update candidates out
in various source files that can "accidentally" belong also to Linux. ;-)


> Could we just give wrong bots a bit less ammunition whenever that's feasible?

How do you think about to clarify constraints any further so that
the probability for false positives can be reduced as desired for
the involved source code analysis tools?


> Even if you don't care about my ping-pong experiences: this checkpatch
> test is broken, please just fix it!

I am curious how collateral software evolution will be continued.

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1493416 — Re: dm snapshot: Use kmalloc_array() in init_origin_hash()

> 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

[toc] | [prev] | [next] | [standalone]

#1493451 — Re: dm snapshot: Use kmalloc_array() in init_origin_hash()

On Thu, Sep 29, 2016 at 01:45:41PM +0200, SF Markus Elfring wrote:
> > 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?

Here's my suggestion:

	https://www.youtube.com/watch?v=xAnVNXaa5oA

Regards,

					- Ted

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web