[PATCH] checkpatch: warn that small allocs should be combined

[PATCH] checkpatch: warn that small allocs should be combined

[trix]

From: Tom Rix <trix@redhat.com>

A memory allocation has overhead.  When a
small allocation is made the overhead dominates.
By combining the fixed sized small allocations
with others, the memory usage can be reduced
by eliminating the overhead of the small allocs.

Signed-off-by: Tom Rix <trix@redhat.com>
---
 scripts/checkpatch.pl | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 577e029987011..605d5278677fc 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7076,6 +7076,12 @@ sub process {
 			     "$1 uses number as first arg, sizeof is generally wrong\n" . $herecurr);
 		}
 
+# check for small allocs
+		if ($line =~ /\b(?:kv|k|v)[zm]alloc\s*\(\s*(\d|sizeof\s*\([su](8|16|32)s*\))\s*,/) {
+			WARN("SMALL_ALLOC",
+			     "Small allocs should be combined\n" . $herecurr);
+		}
+
 # check for multiple semicolons
 		if ($line =~ /;\s*;\s*$/) {
 			if (WARN("ONE_SEMICOLON",
-- 
2.26.3

Re: [PATCH] checkpatch: warn that small allocs should be combined

[Joe Perches]

On Sun, 2022-03-13 at 07:08 -0700, trix@redhat.com wrote:
> From: Tom Rix <trix@redhat.com>
> 
> A memory allocation has overhead.  When a
> small allocation is made the overhead dominates.
> By combining the fixed sized small allocations
> with others, the memory usage can be reduced
> by eliminating the overhead of the small allocs.

This will generate false positives as small allocs are
sometimes required for usb dma.

How many of these "small allocs" _could_ be combined and under
what circumstance?

Can you show me a current example in the kernel where this
is useful?

> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -7076,6 +7076,12 @@ sub process {
>  			     "$1 uses number as first arg, sizeof is generally wrong\n" . $herecurr);
>  		}
>  
> +# check for small allocs
> +		if ($line =~ /\b(?:kv|k|v)[zm]alloc\s*\(\s*(\d|sizeof\s*\([su](8|16|32)s*\))\s*,/) {
> +			WARN("SMALL_ALLOC",
> +			     "Small allocs should be combined\n" . $herecurr);
> +		}
> +

Couple more comments:

Anyone using vmalloc variants for a small alloc is confused.
What defines "small"?
Why would a single decimal like 8 be small, but say 16 would not be?

checkpatch has a couple of regexes that could be useful here

Maybe instead of sizeof(your regex) use

	sizeof\s*\(\s*(?:\d|$C90_int_types|$typeTypedefs)\s*,

as that will find more "small" uses of individual types like
"unsigned long", __s32, u_int_16, etc...

Re: [PATCH] checkpatch: warn that small allocs should be combined

[Tom Rix]

On 3/13/22 9:09 AM, Joe Perches wrote:
> On Sun, 2022-03-13 at 07:08 -0700, trix@redhat.com wrote:
>> From: Tom Rix <trix@redhat.com>
>>
>> A memory allocation has overhead.  When a
>> small allocation is made the overhead dominates.
>> By combining the fixed sized small allocations
>> with others, the memory usage can be reduced
>> by eliminating the overhead of the small allocs.
> This will generate false positives as small allocs are
> sometimes required for usb dma.
>
> How many of these "small allocs" _could_ be combined and under
> what circumstance?
>
> Can you show me a current example in the kernel where this
> is useful?

Tracing what the memory is used for is not easy.

And opens a can of worms.

Most/all of the alloc use only GFP_KERNEL.

If this allocs implicitly align / size suites dma which i am

guessing is (void *) aligned/size then then there will be some

cases of overalignment.

The addition of a GFP_DMA could indicate the memory was to be dma-ed, 
but cause other breakage.

So there is not a good way currently for checkpatch to figure this out.

>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
>> @@ -7076,6 +7076,12 @@ sub process {
>>   			     "$1 uses number as first arg, sizeof is generally wrong\n" . $herecurr);
>>   		}
>>   
>> +# check for small allocs
>> +		if ($line =~ /\b(?:kv|k|v)[zm]alloc\s*\(\s*(\d|sizeof\s*\([su](8|16|32)s*\))\s*,/) {
>> +			WARN("SMALL_ALLOC",
>> +			     "Small allocs should be combined\n" . $herecurr);
>> +		}
>> +
> Couple more comments:
>
> Anyone using vmalloc variants for a small alloc is confused.
> What defines "small"?
> Why would a single decimal like 8 be small, but say 16 would not be?
>
> checkpatch has a couple of regexes that could be useful here
>
> Maybe instead of sizeof(your regex) use
>
> 	sizeof\s*\(\s*(?:\d|$C90_int_types|$typeTypedefs)\s*,
>
> as that will find more "small" uses of individual types like
> "unsigned long", __s32, u_int_16, etc...
>
ok

Tom

>