[PATCH] kernel: power: swap: mark a function as __init to save some memory

[PATCH] kernel: power: swap: mark a function as __init to save some memory

[Christophe JAILLET]

'swsusp_header_init()' is only called via 'core_initcall'.
It can be marked as __init to save a few bytes of memory.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 kernel/power/swap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/power/swap.c b/kernel/power/swap.c
index ca0fcb5ced71..01e2858b5fe3 100644
--- a/kernel/power/swap.c
+++ b/kernel/power/swap.c
@@ -1590,7 +1590,7 @@ int swsusp_unmark(void)
 }
 #endif
 
-static int swsusp_header_init(void)
+static int __init swsusp_header_init(void)
 {
 	swsusp_header = (struct swsusp_header*) __get_free_page(GFP_KERNEL);
 	if (!swsusp_header)
-- 
2.25.1

Re: [PATCH] kernel: power: swap: mark a function as __init to save some memory

[Joe Perches]

(adding Dan Carpenter)

On Sun, 2020-05-31 at 23:00 +0200, Christophe JAILLET wrote:
> 'swsusp_header_init()' is only called via 'core_initcall'.
> It can be marked as __init to save a few bytes of memory.

Hey Dan

smatch has a full function calling tree right?

Can smatch find unmarked functions called only by __init
functions so those unmarked functions can be appropriately
marked with __init like the below?

> ---
>  kernel/power/swap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index ca0fcb5ced71..01e2858b5fe3 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -1590,7 +1590,7 @@ int swsusp_unmark(void)
>  }
>  #endif
>  
> -static int swsusp_header_init(void)
> +static int __init swsusp_header_init(void)
>  {
>  	swsusp_header = (struct swsusp_header*) __get_free_page(GFP_KERNEL);
>  	if (!swsusp_header)

Re: [PATCH] kernel: power: swap: mark a function as __init to save some memory

[Christophe JAILLET]

Le 01/06/2020 à 00:11, Joe Perches a écrit :
> (adding Dan Carpenter)
> 
> On Sun, 2020-05-31 at 23:00 +0200, Christophe JAILLET wrote:
>> 'swsusp_header_init()' is only called via 'core_initcall'.
>> It can be marked as __init to save a few bytes of memory.
> 
> Hey Dan
> 
> smatch has a full function calling tree right?
> 
> Can smatch find unmarked functions called only by __init
> functions so those unmarked functions can be appropriately
> marked with __init like the below?
> 

Hi, in case of interest for anyone, I actually find such things as follow:
    - grep to spot xxx_initcall macro (see comments in the perl script 
below)
    - a perl script which tries to spot missing __init

The false positive rate is low.
Feel free to use and propose patches based on it.

CJ

________________________________________________
#!/usr/bin/perl

use warnings;
use strict;

# grep -r --include=*.c -E 
'^[[:space:]]*(early|core|postcore|arch|subsys|fs|device|late)_initcall\(.*\)' 
* > tmp.txt
my $tmp="tmp.txt";

open(my $fh, "<", $tmp);
while (my $line = <$fh>) {
         # Each line looks like:
         #      net/mac80211/main.c:subsys_initcall(ieee80211_init);
         if ($line =~ /^(.*):.*\((.*)\)/) {
             system("grep -E '$2\\(void' $1 | grep -v -E '__.*init'");
         }
}
close($fh)


>> ---
>>   kernel/power/swap.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
>> index ca0fcb5ced71..01e2858b5fe3 100644
>> --- a/kernel/power/swap.c
>> +++ b/kernel/power/swap.c
>> @@ -1590,7 +1590,7 @@ int swsusp_unmark(void)
>>   }
>>   #endif
>>   
>> -static int swsusp_header_init(void)
>> +static int __init swsusp_header_init(void)
>>   {
>>   	swsusp_header = (struct swsusp_header*) __get_free_page(GFP_KERNEL);
>>   	if (!swsusp_header)
> 
> 

Re: [PATCH] kernel: power: swap: mark a function as __init to save some memory

[Rafael J. Wysocki]

On Sun, May 31, 2020 at 11:01 PM Christophe JAILLET
<christophe.jaillet@wanadoo.fr> wrote:
>
> 'swsusp_header_init()' is only called via 'core_initcall'.
> It can be marked as __init to save a few bytes of memory.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  kernel/power/swap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/power/swap.c b/kernel/power/swap.c
> index ca0fcb5ced71..01e2858b5fe3 100644
> --- a/kernel/power/swap.c
> +++ b/kernel/power/swap.c
> @@ -1590,7 +1590,7 @@ int swsusp_unmark(void)
>  }
>  #endif
>
> -static int swsusp_header_init(void)
> +static int __init swsusp_header_init(void)
>  {
>         swsusp_header = (struct swsusp_header*) __get_free_page(GFP_KERNEL);
>         if (!swsusp_header)
> --

Applied as 5.8-rc material under the "PM: hibernate: Add __init
annotation to swsusp_header_init()" subject, thanks!

Re: [PATCH] kernel: power: swap: mark a function as __init to save some memory

[Dan Carpenter]

On Sun, May 31, 2020 at 03:11:27PM -0700, Joe Perches wrote:
> (adding Dan Carpenter)
> 
> On Sun, 2020-05-31 at 23:00 +0200, Christophe JAILLET wrote:
> > 'swsusp_header_init()' is only called via 'core_initcall'.
> > It can be marked as __init to save a few bytes of memory.
> 
> Hey Dan
> 
> smatch has a full function calling tree right?
> 
> Can smatch find unmarked functions called only by __init
> functions so those unmarked functions can be appropriately
> marked with __init like the below?
> 

It turns out it's complicated to do this in Smatch because Sparse
ignores the section attribute.  :/

regards,
dan carpenter

Re: [PATCH] kernel: power: swap: mark a function as __init to save some memory

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1101 bytes --]



On Mon, 8 Jun 2020, Dan Carpenter wrote:

> On Sun, May 31, 2020 at 03:11:27PM -0700, Joe Perches wrote:
> > (adding Dan Carpenter)
> >
> > On Sun, 2020-05-31 at 23:00 +0200, Christophe JAILLET wrote:
> > > 'swsusp_header_init()' is only called via 'core_initcall'.
> > > It can be marked as __init to save a few bytes of memory.
> >
> > Hey Dan
> >
> > smatch has a full function calling tree right?
> >
> > Can smatch find unmarked functions called only by __init
> > functions so those unmarked functions can be appropriately
> > marked with __init like the below?
> >
>
> It turns out it's complicated to do this in Smatch because Sparse
> ignores the section attribute.  :/

I wrote a script at one point for this for Coccinelle, and sent some
patches.  It requires some effort, because you want to run it over and
over - once function Y becomes init, some other functions might become
init as well.  The iteration could be done automatically with Coccinelle,
but I didn't take that option, because it semed safer to check the results
along the way.  A version of the script is attached.

julia

[-- Attachment #2: Type: text/plain, Size: 2715 bytes --]

// No iteration.  Do it by hand.

@initialize:ocaml@
@@

let itbl = Hashtbl.create 101
let ltbl = Hashtbl.create 101
let thefile = ref ""

let hashadd t k =
  let cell =
    try Hashtbl.find t k
    with Not_found ->
      let cell = ref 0 in
      Hashtbl.add t k cell;
      cell in
  cell := !cell + 1

let hashget t k = try !(Hashtbl.find t k) with Not_found -> 0

let seen  = ref []

@script:ocaml@
@@

(let file = List.hd (Coccilib.files()) in
thefile := file;
let file =
    try List.hd(List.tl (Str.split (Str.regexp "/linux-next/") file))
    with _ -> file in
let ofile = "/var/julia/linux-next/" ^
      (Filename.chop_extension file) ^ ".o" in
if not(Sys.file_exists ofile)
then Coccilib.exit());

Hashtbl.clear itbl;
Hashtbl.clear ltbl;
seen := []

@r@
identifier f;
@@

__init f(...) { ... }

@script:ocaml@
f << r.f;
@@

Hashtbl.add itbl f ()

@s disable optional_attributes@
identifier f;
@@

static f(...) { ... }

@script:ocaml@
f << s.f;
@@

Hashtbl.add ltbl f ()

@t exists@
identifier f,g;
position p;
@@

__init f(...) { ... when any
   g@p(...)
   ... when any
 }

@script:ocaml@
g << t.g;
_p << t.p;
@@

if not (Hashtbl.mem ltbl g) || Hashtbl.mem itbl g
then Coccilib.include_match false

@ok1 disable optional_attributes exists@
identifier f,t.g;
@@

f(...) { ... when any
   g
   ... when any
 }

@ok2 disable optional_attributes exists@
identifier i,j,fld,t.g;
@@

struct i j = { .fld = g, };

@ok3 disable optional_attributes exists@
identifier t.g;
declarer d;
@@

d(...,g,...);

@ok4 disable optional_attributes exists@
identifier t.g;
expression e;
@@

(
e(...,g,...)
|
e(...,&g,...)
|
e = &g
|
e = g
)

@script:ocaml depends on !ok1 && !ok2 && !ok3 && !ok4@
g << t.g;
@@

let file = !thefile in
let file =
    try List.hd(List.tl (Str.split (Str.regexp "/linux-next/") file))
    with _ -> file in
if not(List.mem (g,file) !seen)
then
  begin
    seen := (g,file) :: !seen;
    let ofile = "/var/julia/linux-next/" ^
      (Filename.chop_extension file) ^ ".o" in
    if Sys.file_exists ofile
    then
      let l =
	Common.cmd_to_list
	  (Printf.sprintf
	     "objdump -x %s | grep -w %s | grep -w F | grep .text.unlikely"
	     ofile g) in
      match l with
	[] -> Coccilib.include_match false
      | _ ->
	  Printf.printf "Info for %s %s\n" file g;
	  List.iter
	    (function l -> Printf.printf "%s\n" l)
	    l;
	  Printf.printf "\n"; flush stdout
    else Coccilib.include_match false
  end
else Coccilib.include_match false

@depends on !ok1 && !ok2 && !ok3 && !ok4@
identifier t.g;
@@

- g
+__init g
 (...) { ... }