Coccinelle Archive on lore.kernel.org
 help / color / Atom feed
* [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
@ 2021-03-02 17:42 Joe Perches
  2021-03-02 21:41 ` Julia Lawall
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Joe Perches @ 2021-03-02 17:42 UTC (permalink / raw)
  To: kernelnewbies, kernel-janitors, cocci; +Cc: LKML

Here is a possible opportunity to reduce data usage in the kernel.

$ git grep -P -n '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]' drivers/ | \
  grep -v __initdata | \
  wc -l
3250

Meaning there are ~3000 declarations of arrays with what appears to be
file static const content that are not marked const.

So there are many static arrays that could be marked const to move the
compiled object code from data to text minimizing the total amount of
exposed r/w data.

However, I do not know of a mechanism using coccinelle to determine
whether or not any of these static declarations are ever modified.

So it appears that each instance of these declarations might need
manual inspection.

But for arrays declared inside functions, it's much more likely that
the static declaration without const is done with the intent to modify
the array:

(note the difference in the git grep with a leading '^\s+')

$ git grep -Pn '^\s+static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]' drivers/ | \
  grep -v __initdata | \
  wc -l
323

------------- For instance: (head -10 of the git grep for file statics)

drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = { 32, 16, 8, 4, 2, 1 };
drivers/accessibility/speakup/keyhelp.c:26:static u_char funcvals[] = {
drivers/accessibility/speakup/main.c:2059:static spkup_hand spkup_handler[] = {
drivers/accessibility/speakup/speakup_acntpc.c:35:static unsigned int synth_portlist[] = { 0x2a8, 0 };
drivers/accessibility/speakup/speakup_decpc.c:133:static int synth_portlist[] = { 0x340, 0x350, 0x240, 0x250, 0 };
drivers/accessibility/speakup/speakup_dectlk.c:110:static int ap_defaults[] = {122, 89, 155, 110, 208, 240, 200, 106, 306};
drivers/accessibility/speakup/speakup_dectlk.c:111:static int g5_defaults[] = {86, 81, 86, 84, 81, 80, 83, 83, 73};
drivers/accessibility/speakup/speakup_dtlk.c:34:static unsigned int synth_portlist[] = {
drivers/accessibility/speakup/speakup_keypc.c:34:static unsigned int synth_portlist[] = { 0x2a8, 0 };
drivers/acpi/ac.c:137:static enum power_supply_property ac_props[] = {

For drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = { 32, 16, 8, 4, 2, 1 };

masks is only used in static function say_key and should be const and
perhaps the declaration might be better moved into that function.

For drivers/accessibility/speakup/keyhelp.c:26:static u_char funcvals[] = {

funcvals is only used in static function spk_handle_help and should be const
and perhaps the declaration might be better moved into that function.

For drivers/accessibility/speakup/main.c:2059:static spkup_hand spkup_handler[] = {

spkup_handler is only used in static function do_spkup and should be const
and perhaps the declaration might be better moved into that function.

etc... for speakup

For drivers/acpi/ac.c:137:static enum power_supply_property ac_props[] = {

array ac_props is assigned as a reference in acpi_ac_add as a 
"const enum power_supply_property *" member of a struct power_supply_desc.

------------- For instance: (head -10 of the git grep for function statics)

drivers/acpi/apei/apei-base.c:781:	static u8 whea_uuid_str[] = "ed855e0c-6c90-47bf-a62a-26de0fc5ad5c";
drivers/block/amiflop.c:1051:	static unsigned char CRCTable1[] = {
drivers/block/amiflop.c:1070:	static unsigned char CRCTable2[] = {
drivers/block/drbd/drbd_nl.c:872:	static char units[] = { 'K', 'M', 'G', 'T', 'P', 'E' };
drivers/block/drbd/drbd_proc.c:224:	static char write_ordering_chars[] = {
drivers/block/drbd/drbd_receiver.c:4363:	static enum drbd_conns c_tab[] = {
drivers/char/pcmcia/synclink_cs.c:3717:	static unsigned char patterns[] =
drivers/cpufreq/intel_pstate.c:1515:	static int silvermont_freq_table[] = {
drivers/cpufreq/intel_pstate.c:1530:	static int airmont_freq_table[] = {
drivers/dma/xgene-dma.c:360:	static u8 flyby_type[] = {

Some of these could be const, but some could not.  For instance:

For drivers/acpi/apei/apei-base.c:781:	static u8 whea_uuid_str[] = "ed855e0c-6c90-47bf-a62a-26de0fc5ad5c";

whea_uuid_str is assigned as a reference in "int apei_osc_setup(void)"
a struct acpi_osc_context where .uuid_str is not declared as const char *.




_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
  2021-03-02 17:42 [Cocci] linux-kernel janitorial RFP: Mark static arrays as const Joe Perches
@ 2021-03-02 21:41 ` Julia Lawall
  2021-03-03  2:47   ` Joe Perches
  2021-03-02 22:18 ` Bernd Petrovitsch
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 13+ messages in thread
From: Julia Lawall @ 2021-03-02 21:41 UTC (permalink / raw)
  To: Joe Perches; +Cc: LKML, kernel-janitors, cocci, kernelnewbies



On Tue, 2 Mar 2021, Joe Perches wrote:

> Here is a possible opportunity to reduce data usage in the kernel.

Does it actually reduce data usage?

julia

>
> $ git grep -P -n '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]' drivers/ | \
>   grep -v __initdata | \
>   wc -l
> 3250
>
> Meaning there are ~3000 declarations of arrays with what appears to be
> file static const content that are not marked const.
>
> So there are many static arrays that could be marked const to move the
> compiled object code from data to text minimizing the total amount of
> exposed r/w data.
>
> However, I do not know of a mechanism using coccinelle to determine
> whether or not any of these static declarations are ever modified.
>
> So it appears that each instance of these declarations might need
> manual inspection.
>
> But for arrays declared inside functions, it's much more likely that
> the static declaration without const is done with the intent to modify
> the array:
>
> (note the difference in the git grep with a leading '^\s+')
>
> $ git grep -Pn '^\s+static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]' drivers/ | \
>   grep -v __initdata | \
>   wc -l
> 323
>
> ------------- For instance: (head -10 of the git grep for file statics)
>
> drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = { 32, 16, 8, 4, 2, 1 };
> drivers/accessibility/speakup/keyhelp.c:26:static u_char funcvals[] = {
> drivers/accessibility/speakup/main.c:2059:static spkup_hand spkup_handler[] = {
> drivers/accessibility/speakup/speakup_acntpc.c:35:static unsigned int synth_portlist[] = { 0x2a8, 0 };
> drivers/accessibility/speakup/speakup_decpc.c:133:static int synth_portlist[] = { 0x340, 0x350, 0x240, 0x250, 0 };
> drivers/accessibility/speakup/speakup_dectlk.c:110:static int ap_defaults[] = {122, 89, 155, 110, 208, 240, 200, 106, 306};
> drivers/accessibility/speakup/speakup_dectlk.c:111:static int g5_defaults[] = {86, 81, 86, 84, 81, 80, 83, 83, 73};
> drivers/accessibility/speakup/speakup_dtlk.c:34:static unsigned int synth_portlist[] = {
> drivers/accessibility/speakup/speakup_keypc.c:34:static unsigned int synth_portlist[] = { 0x2a8, 0 };
> drivers/acpi/ac.c:137:static enum power_supply_property ac_props[] = {
>
> For drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = { 32, 16, 8, 4, 2, 1 };
>
> masks is only used in static function say_key and should be const and
> perhaps the declaration might be better moved into that function.
>
> For drivers/accessibility/speakup/keyhelp.c:26:static u_char funcvals[] = {
>
> funcvals is only used in static function spk_handle_help and should be const
> and perhaps the declaration might be better moved into that function.
>
> For drivers/accessibility/speakup/main.c:2059:static spkup_hand spkup_handler[] = {
>
> spkup_handler is only used in static function do_spkup and should be const
> and perhaps the declaration might be better moved into that function.
>
> etc... for speakup
>
> For drivers/acpi/ac.c:137:static enum power_supply_property ac_props[] = {
>
> array ac_props is assigned as a reference in acpi_ac_add as a
> "const enum power_supply_property *" member of a struct power_supply_desc.
>
> ------------- For instance: (head -10 of the git grep for function statics)
>
> drivers/acpi/apei/apei-base.c:781:	static u8 whea_uuid_str[] = "ed855e0c-6c90-47bf-a62a-26de0fc5ad5c";
> drivers/block/amiflop.c:1051:	static unsigned char CRCTable1[] = {
> drivers/block/amiflop.c:1070:	static unsigned char CRCTable2[] = {
> drivers/block/drbd/drbd_nl.c:872:	static char units[] = { 'K', 'M', 'G', 'T', 'P', 'E' };
> drivers/block/drbd/drbd_proc.c:224:	static char write_ordering_chars[] = {
> drivers/block/drbd/drbd_receiver.c:4363:	static enum drbd_conns c_tab[] = {
> drivers/char/pcmcia/synclink_cs.c:3717:	static unsigned char patterns[] =
> drivers/cpufreq/intel_pstate.c:1515:	static int silvermont_freq_table[] = {
> drivers/cpufreq/intel_pstate.c:1530:	static int airmont_freq_table[] = {
> drivers/dma/xgene-dma.c:360:	static u8 flyby_type[] = {
>
> Some of these could be const, but some could not.  For instance:
>
> For drivers/acpi/apei/apei-base.c:781:	static u8 whea_uuid_str[] = "ed855e0c-6c90-47bf-a62a-26de0fc5ad5c";
>
> whea_uuid_str is assigned as a reference in "int apei_osc_setup(void)"
> a struct acpi_osc_context where .uuid_str is not declared as const char *.
>
>
>
>
> _______________________________________________
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
  2021-03-02 17:42 [Cocci] linux-kernel janitorial RFP: Mark static arrays as const Joe Perches
  2021-03-02 21:41 ` Julia Lawall
@ 2021-03-02 22:18 ` Bernd Petrovitsch
  2021-03-03  8:36   ` Julia Lawall
  2021-03-04  8:34   ` Andy Shevchenko
  2021-03-03  9:41 ` Rasmus Villemoes
  2021-03-03 17:06 ` Mansour Moufid
  3 siblings, 2 replies; 13+ messages in thread
From: Bernd Petrovitsch @ 2021-03-02 22:18 UTC (permalink / raw)
  To: Joe Perches; +Cc: cocci, kernel-janitors, LKML, kernelnewbies

Hi all!

On 02/03/2021 18:42, Joe Perches wrote:
[...]
> ------------- For instance: (head -10 of the git grep for file statics)
> 
> drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = { 32, 16, 8, 4, 2, 1 };
> drivers/accessibility/speakup/keyhelp.c:26:static u_char funcvals[] = {
> drivers/accessibility/speakup/main.c:2059:static spkup_hand spkup_handler[] = {
> drivers/accessibility/speakup/speakup_acntpc.c:35:static unsigned int synth_portlist[] = { 0x2a8, 0 };
> drivers/accessibility/speakup/speakup_decpc.c:133:static int synth_portlist[] = { 0x340, 0x350, 0x240, 0x250, 0 };
> drivers/accessibility/speakup/speakup_dectlk.c:110:static int ap_defaults[] = {122, 89, 155, 110, 208, 240, 200, 106, 306};
> drivers/accessibility/speakup/speakup_dectlk.c:111:static int g5_defaults[] = {86, 81, 86, 84, 81, 80, 83, 83, 73};
> drivers/accessibility/speakup/speakup_dtlk.c:34:static unsigned int synth_portlist[] = {
> drivers/accessibility/speakup/speakup_keypc.c:34:static unsigned int synth_portlist[] = { 0x2a8, 0 };
> drivers/acpi/ac.c:137:static enum power_supply_property ac_props[] = {
> 
> For drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = { 32, 16, 8, 4, 2, 1 };

Looking at the examples: Just s/^static /static const / in the lines
reported by the grep's above and see if it compiles (and save space)?

MfG,
	Bernd
-- 
Bernd Petrovitsch                  Email : bernd@petrovitsch.priv.at
     There is NO CLOUD, just other people's computers. - FSFE
                     LUGA : http://www.luga.at
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
  2021-03-02 21:41 ` Julia Lawall
@ 2021-03-03  2:47   ` Joe Perches
  0 siblings, 0 replies; 13+ messages in thread
From: Joe Perches @ 2021-03-03  2:47 UTC (permalink / raw)
  To: Julia Lawall; +Cc: LKML, kernel-janitors, cocci, kernelnewbies

On Tue, 2021-03-02 at 22:41 +0100, Julia Lawall wrote:
> 
> On Tue, 2 Mar 2021, Joe Perches wrote:
> 
> > Here is a possible opportunity to reduce data usage in the kernel.
> 
> Does it actually reduce data usage?

Yes, at least for gcc.  For instance:

$ gcc --version
gcc (Ubuntu 10.2.0-13ubuntu1) 10.2.0

And with the diff below (x86-64 defconfig with hwmon/pmbus and max20730)

$ diff --git a/drivers/hwmon/pmbus/max20730.c b/drivers/hwmon/pmbus/max20730.c
index 9dd3dd79bc18..b824eab95456 100644
--- a/drivers/hwmon/pmbus/max20730.c
+++ b/drivers/hwmon/pmbus/max20730.c
@@ -434,7 +434,7 @@ static long direct_to_val(u16 w, enum pmbus_sensor_classes >
        return d;
 }
 
-static u32 max_current[][5] = {
+static const u32 max_current[][5] = {
        [max20710] = { 6200, 8000, 9700, 11600 },
        [max20730] = { 13000, 16600, 20100, 23600 },
        [max20734] = { 21000, 27000, 32000, 38000 },

$ size drivers/hwmon/pmbus/max20730.o*
   text	   data	    bss	    dec	    hex	filename
   9245	    256	      0	   9501	   251d	drivers/hwmon/pmbus/max20730.o.gcc.new
   9149	    352	      0	   9501	   251d	drivers/hwmon/pmbus/max20730.o.gcc.old

with clang-11 it doesn't seem to make a difference:

$ /usr/bin/clang --version
Ubuntu clang version 11.0.0-2

$ size drivers/hwmon/pmbus/max20730.o*
   text	   data	    bss	    dec	    hex	filename
   9166	    256	      1	   9423	   24cf	drivers/hwmon/pmbus/max20730.o.clang-11.new
   9166	    256	      1	   9423	   24cf	drivers/hwmon/pmbus/max20730.o.clang-11.old


_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
  2021-03-02 22:18 ` Bernd Petrovitsch
@ 2021-03-03  8:36   ` Julia Lawall
  2021-03-04  8:34   ` Andy Shevchenko
  1 sibling, 0 replies; 13+ messages in thread
From: Julia Lawall @ 2021-03-03  8:36 UTC (permalink / raw)
  To: Bernd Petrovitsch
  Cc: Joe Perches, kernelnewbies, kernel-janitors, cocci, LKML



On Tue, 2 Mar 2021, Bernd Petrovitsch wrote:

> Hi all!
>
> On 02/03/2021 18:42, Joe Perches wrote:
> [...]
> > ------------- For instance: (head -10 of the git grep for file statics)
> >
> > drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = { 32, 16, 8, 4, 2, 1 };
> > drivers/accessibility/speakup/keyhelp.c:26:static u_char funcvals[] = {
> > drivers/accessibility/speakup/main.c:2059:static spkup_hand spkup_handler[] = {
> > drivers/accessibility/speakup/speakup_acntpc.c:35:static unsigned int synth_portlist[] = { 0x2a8, 0 };
> > drivers/accessibility/speakup/speakup_decpc.c:133:static int synth_portlist[] = { 0x340, 0x350, 0x240, 0x250, 0 };
> > drivers/accessibility/speakup/speakup_dectlk.c:110:static int ap_defaults[] = {122, 89, 155, 110, 208, 240, 200, 106, 306};
> > drivers/accessibility/speakup/speakup_dectlk.c:111:static int g5_defaults[] = {86, 81, 86, 84, 81, 80, 83, 83, 73};
> > drivers/accessibility/speakup/speakup_dtlk.c:34:static unsigned int synth_portlist[] = {
> > drivers/accessibility/speakup/speakup_keypc.c:34:static unsigned int synth_portlist[] = { 0x2a8, 0 };
> > drivers/acpi/ac.c:137:static enum power_supply_property ac_props[] = {
> >
> > For drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = { 32, 16, 8, 4, 2, 1 };
>
> Looking at the examples: Just s/^static /static const / in the lines
> reported by the grep's above and see if it compiles (and save space)?

There is a small risk with compiling that there may be a problem for a
configuration you haven't considered.

julia
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
  2021-03-02 17:42 [Cocci] linux-kernel janitorial RFP: Mark static arrays as const Joe Perches
  2021-03-02 21:41 ` Julia Lawall
  2021-03-02 22:18 ` Bernd Petrovitsch
@ 2021-03-03  9:41 ` Rasmus Villemoes
  2021-03-03 14:51   ` Joe Perches
  2021-03-03 17:06 ` Mansour Moufid
  3 siblings, 1 reply; 13+ messages in thread
From: Rasmus Villemoes @ 2021-03-03  9:41 UTC (permalink / raw)
  To: Joe Perches, kernelnewbies, kernel-janitors, cocci; +Cc: LKML

On 02/03/2021 18.42, Joe Perches wrote:
> Here is a possible opportunity to reduce data usage in the kernel.
> 
> $ git grep -P -n '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]' drivers/ | \
>   grep -v __initdata | \
>   wc -l
> 3250
> 
> Meaning there are ~3000 declarations of arrays with what appears to be
> file static const content that are not marked const.
> 
> So there are many static arrays that could be marked const to move the
> compiled object code from data to text minimizing the total amount of
> exposed r/w data.

You can add const if you like, but it will rarely change the generated
code. gcc is already smart enough to take a static array whose contents
are provably never modified within the TU and put it in .rodata:

static int x = 7;
static int y[2] = {13, 19};

static int p(int a, const int *foo)
{
	return a + *foo;
}
int q(int a)
{
	int b = p(a, &x);
	return p(b, &y[b & 1]);
}
$ nm c.o
0000000000000000 T q
0000000000000000 r y
$ size c.o
   text    data     bss     dec     hex filename
    111       0       0     111      6f c.o

So x gets optimized away completely, but y isn't so easy to get rid of -
nevertheless, it's never modified and the address doesn't escape the TU,
so gcc treats is as if it was declared const.

I think you'll see the same if you try adding the const on a few of your
real-life examples. (Of course, the control flow may be so convoluted
that gcc isn't able to infer the constness, so I'm not saying it will
never make a difference - only that you shouldn't expect too much.)

Rasmus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
  2021-03-03  9:41 ` Rasmus Villemoes
@ 2021-03-03 14:51   ` Joe Perches
  2021-03-07 19:14     ` Julia Lawall
  0 siblings, 1 reply; 13+ messages in thread
From: Joe Perches @ 2021-03-03 14:51 UTC (permalink / raw)
  To: Rasmus Villemoes, kernelnewbies, kernel-janitors, cocci; +Cc: LKML

On Wed, 2021-03-03 at 10:41 +0100, Rasmus Villemoes wrote:
> On 02/03/2021 18.42, Joe Perches wrote:
> > Here is a possible opportunity to reduce data usage in the kernel.
> > 
> > $ git grep -P -n '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]' drivers/ | \
> >   grep -v __initdata | \
> >   wc -l
> > 3250
> > 
> > Meaning there are ~3000 declarations of arrays with what appears to be
> > file static const content that are not marked const.
> > 
> > So there are many static arrays that could be marked const to move the
> > compiled object code from data to text minimizing the total amount of
> > exposed r/w data.
> 
> You can add const if you like, but it will rarely change the generated
> code. gcc is already smart enough to take a static array whose contents
> are provably never modified within the TU and put it in .rodata:

At least some or perhaps even most of the time, true, but the gcc compiler
from v5 through at least v10 seems inconsistent about when it does the
appropriate conversion.

See the example I posted:
https://lore.kernel.org/lkml/6b8b250a06a98ce42120a14824531a8641f5e8aa.camel@perches.com/

It was a randomly chosen source file conversion btw, I had no prior
knowledge of whether the text/data use would change.

I'm unsure about clang consistently moving static but provably const arrays
from data to text.  I rarely use clang.  At least for v11 it seems to be
better though.  I didn't try 10.1.


_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
  2021-03-02 17:42 [Cocci] linux-kernel janitorial RFP: Mark static arrays as const Joe Perches
                   ` (2 preceding siblings ...)
  2021-03-03  9:41 ` Rasmus Villemoes
@ 2021-03-03 17:06 ` Mansour Moufid
  2021-03-03 17:21   ` Julia Lawall
  3 siblings, 1 reply; 13+ messages in thread
From: Mansour Moufid @ 2021-03-03 17:06 UTC (permalink / raw)
  To: Joe Perches; +Cc: cocci

On Tue, Mar 2, 2021 at 4:22 PM Joe Perches <joe@perches.com> wrote:
>
> Here is a possible opportunity to reduce data usage in the kernel.
>
> $ git grep -P -n '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]' drivers/ | \
>   grep -v __initdata | \
>   wc -l
> 3250
>
> Meaning there are ~3000 declarations of arrays with what appears to be
> file static const content that are not marked const.
>
> So there are many static arrays that could be marked const to move the
> compiled object code from data to text minimizing the total amount of
> exposed r/w data.
>
> However, I do not know of a mechanism using coccinelle to determine
> whether or not any of these static declarations are ever modified.

I thought it would be a fun exercise but it got tedious quick.

I don't know how to ignore an attribute like __initdata.

Feel free to refine it:


@@
type t;
identifier x;
@@
(
    static const struct { ... } x[];
|
    static
+   const
    struct { ... } x[];
|
    static const struct s *x[];
|
    static
+   const
    struct s *x[];
|
    static const struct s x[];
|
    static
+   const
    struct s x[];
|
    static const t *x[];
|
    static
+   const
    t *x[];
|
    static const t x[];
|
    static
+   const
    t x[];
)

@@
type t;
identifier s, x, y, z;
assignment operator xx;
@@
(
    static const struct { ... } x[] = { ... };
|
    static
+   const
    struct { ... } x[] = { ... };
|
    static const struct s *x[] = { ... };
|
    static
+   const
    struct s *x[] = { ... };
|
    static const struct s x[] = { ... };
|
    static
+   const
    struct s x[] = { ... };
|
    static const t *x[] = { ... };
|
    static
+   const
    t *x[] = { ... };
|
    static const t x[] = { ... };
|
    static
+   const
    t x[] = { ... };
)
    ... when != x.y xx ...
        when != x[...] xx ...
        when != z = x
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
  2021-03-03 17:06 ` Mansour Moufid
@ 2021-03-03 17:21   ` Julia Lawall
  0 siblings, 0 replies; 13+ messages in thread
From: Julia Lawall @ 2021-03-03 17:21 UTC (permalink / raw)
  To: Mansour Moufid; +Cc: Joe Perches, cocci



On Wed, 3 Mar 2021, Mansour Moufid wrote:

> On Tue, Mar 2, 2021 at 4:22 PM Joe Perches <joe@perches.com> wrote:
> >
> > Here is a possible opportunity to reduce data usage in the kernel.
> >
> > $ git grep -P -n '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]' drivers/ | \
> >   grep -v __initdata | \
> >   wc -l
> > 3250
> >
> > Meaning there are ~3000 declarations of arrays with what appears to be
> > file static const content that are not marked const.
> >
> > So there are many static arrays that could be marked const to move the
> > compiled object code from data to text minimizing the total amount of
> > exposed r/w data.
> >
> > However, I do not know of a mechanism using coccinelle to determine
> > whether or not any of these static declarations are ever modified.
>
> I thought it would be a fun exercise but it got tedious quick.
>
> I don't know how to ignore an attribute like __initdata.
>
> Feel free to refine it:

This adds the consts, but it doesn't check that the array is never
updated, or never stored in a field from which it could get updated.

In my attempt, I tried something like this for the first part:

@r disable optional_qualifier@
type T;
identifier x;
position p != ok.p;
@@

static T x@p[] = { ... };

In principle, this should match cases where there is no const.  But it
dones't work.  It matches some cases with const too.

Then I tried:

@ok@
type T;
identifier x;
position p;
@@

static const T x@p[] = { ... };

@r@
type T;
identifier x;
position p != ok.p;
@@

static T x@p[] = { ... };

The first rule matches the cases with const, and then the second rule
matches all of the cases with the declared variable at a position
different than that of the first rule.  It works if the type T is a simple
type like int, but it doesn't work if it is something more complex, like
struct foo *.

Afterwards, I have the following:

@bad@
position p;
identifier r.x;
expression e,y;
@@

(
 x@p[y] = e
|
 &x@p[y]
)

@good@
identifier r.x;
expression y;
position p != bad.p;
@@

x@p[y]

@notgood@
identifier r.x;
position p != good.p;
@@

x@p

@depends on good && !notgood@
identifier r.x;
type r.T;
@@

static
+const
 T x[] = { ... };

That is,

* rule bad: Give up if we assign an element or take the address of an
element.

* rule good: Things are going well if we access an element of the array.
If there is no access at all, there is something suspicious, perhaps uses
in macros.

* rule notgood: A montion of the array that is not an element access is
not good either

In the end, if we match good and we don't match notgood, then we can make
the change.

I got stuck on the const problem, and didn't try the __initdata issue.
But one could address with a rule like the rule ok above.

The const problem is at least something worth looking into.

julia

>
>
> @@
> type t;
> identifier x;
> @@
> (
>     static const struct { ... } x[];
> |
>     static
> +   const
>     struct { ... } x[];
> |
>     static const struct s *x[];
> |
>     static
> +   const
>     struct s *x[];
> |
>     static const struct s x[];
> |
>     static
> +   const
>     struct s x[];
> |
>     static const t *x[];
> |
>     static
> +   const
>     t *x[];
> |
>     static const t x[];
> |
>     static
> +   const
>     t x[];
> )
>
> @@
> type t;
> identifier s, x, y, z;
> assignment operator xx;
> @@
> (
>     static const struct { ... } x[] = { ... };
> |
>     static
> +   const
>     struct { ... } x[] = { ... };
> |
>     static const struct s *x[] = { ... };
> |
>     static
> +   const
>     struct s *x[] = { ... };
> |
>     static const struct s x[] = { ... };
> |
>     static
> +   const
>     struct s x[] = { ... };
> |
>     static const t *x[] = { ... };
> |
>     static
> +   const
>     t *x[] = { ... };
> |
>     static const t x[] = { ... };
> |
>     static
> +   const
>     t x[] = { ... };
> )
>     ... when != x.y xx ...
>         when != x[...] xx ...
>         when != z = x
> _______________________________________________
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
  2021-03-02 22:18 ` Bernd Petrovitsch
  2021-03-03  8:36   ` Julia Lawall
@ 2021-03-04  8:34   ` Andy Shevchenko
  1 sibling, 0 replies; 13+ messages in thread
From: Andy Shevchenko @ 2021-03-04  8:34 UTC (permalink / raw)
  To: Bernd Petrovitsch
  Cc: Joe Perches, cocci, kernel-janitors, LKML, kernelnewbies

[-- Attachment #1.1: Type: text/plain, Size: 2179 bytes --]

On Wednesday, March 3, 2021, Bernd Petrovitsch <bernd@petrovitsch.priv.at>
wrote:

> Hi all!
>
> On 02/03/2021 18:42, Joe Perches wrote:
> [...]
> > ------------- For instance: (head -10 of the git grep for file statics)
> >
> > drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] = {
> 32, 16, 8, 4, 2, 1 };
> > drivers/accessibility/speakup/keyhelp.c:26:static u_char funcvals[] = {
> > drivers/accessibility/speakup/main.c:2059:static spkup_hand
> spkup_handler[] = {
> > drivers/accessibility/speakup/speakup_acntpc.c:35:static unsigned int
> synth_portlist[] = { 0x2a8, 0 };
> > drivers/accessibility/speakup/speakup_decpc.c:133:static int
> synth_portlist[] = { 0x340, 0x350, 0x240, 0x250, 0 };
> > drivers/accessibility/speakup/speakup_dectlk.c:110:static int
> ap_defaults[] = {122, 89, 155, 110, 208, 240, 200, 106, 306};
> > drivers/accessibility/speakup/speakup_dectlk.c:111:static int
> g5_defaults[] = {86, 81, 86, 84, 81, 80, 83, 83, 73};
> > drivers/accessibility/speakup/speakup_dtlk.c:34:static unsigned int
> synth_portlist[] = {
> > drivers/accessibility/speakup/speakup_keypc.c:34:static unsigned int
> synth_portlist[] = { 0x2a8, 0 };
> > drivers/acpi/ac.c:137:static enum power_supply_property ac_props[] = {
> >
> > For drivers/accessibility/speakup/keyhelp.c:18:static u_short masks[] =
> { 32, 16, 8, 4, 2, 1 };
>
> Looking at the examples: Just s/^static /static const / in the lines
> reported by the grep's above and see if it compiles (and save space)?



I did two reverts and reported at least one issue with blind
constification. Besides that we have a lot of data structures that require
to drop const spécifier and the consumer won’t actually know if it’s
possible to write there or not. I’m talking about driver data fields where
they are defined as type of kernel_ulong_t. So, first you need to fix that,



>
> MfG,
>         Bernd
> --
> Bernd Petrovitsch                  Email : bernd@petrovitsch.priv.at
>      There is NO CLOUD, just other people's computers. - FSFE
>                      LUGA : http://www.luga.at
>


-- 
With Best Regards,
Andy Shevchenko

[-- Attachment #1.2: Type: text/html, Size: 2791 bytes --]

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

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
  2021-03-03 14:51   ` Joe Perches
@ 2021-03-07 19:14     ` Julia Lawall
  2021-03-08  5:38       ` Joe Perches
  0 siblings, 1 reply; 13+ messages in thread
From: Julia Lawall @ 2021-03-07 19:14 UTC (permalink / raw)
  To: Joe Perches; +Cc: LKML, kernelnewbies, kernel-janitors, cocci, Rasmus Villemoes


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



On Wed, 3 Mar 2021, Joe Perches wrote:

> On Wed, 2021-03-03 at 10:41 +0100, Rasmus Villemoes wrote:
> > On 02/03/2021 18.42, Joe Perches wrote:
> > > Here is a possible opportunity to reduce data usage in the kernel.
> > >
> > > $ git grep -P -n '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]' drivers/ | \
> > >   grep -v __initdata | \
> > >   wc -l
> > > 3250
> > >
> > > Meaning there are ~3000 declarations of arrays with what appears to be
> > > file static const content that are not marked const.
> > >
> > > So there are many static arrays that could be marked const to move the
> > > compiled object code from data to text minimizing the total amount of
> > > exposed r/w data.
> >
> > You can add const if you like, but it will rarely change the generated
> > code. gcc is already smart enough to take a static array whose contents
> > are provably never modified within the TU and put it in .rodata:
>
> At least some or perhaps even most of the time, true, but the gcc compiler
> from v5 through at least v10 seems inconsistent about when it does the
> appropriate conversion.
>
> See the example I posted:
> https://lore.kernel.org/lkml/6b8b250a06a98ce42120a14824531a8641f5e8aa.camel@perches.com/
>
> It was a randomly chosen source file conversion btw, I had no prior
> knowledge of whether the text/data use would change.
>
> I'm unsure about clang consistently moving static but provably const arrays
> from data to text.  I rarely use clang.  At least for v11 it seems to be
> better though.  I didn't try 10.1.

I tried the relevnt drivers in drivers/input/joystick.  I got only one
driver that changed with gcc 9.3, which was
drivers/input/joystick/analog.c.  It actually got larger:

original:

   text    data     bss     dec     hex filename
  22607   10560     320   33487    82cf drivers/input/joystick/analog.o

after adding const:

   text    data     bss     dec     hex filename
  22728   10816     320   33864    8448 drivers/input/joystick/analog.o

This was the only case where bss was not 0, but I don't know if there is a
connection.

julia

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

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
  2021-03-07 19:14     ` Julia Lawall
@ 2021-03-08  5:38       ` Joe Perches
  2021-03-08  6:54         ` Julia Lawall
  0 siblings, 1 reply; 13+ messages in thread
From: Joe Perches @ 2021-03-08  5:38 UTC (permalink / raw)
  To: Julia Lawall
  Cc: LKML, kernelnewbies, kernel-janitors, cocci, Rasmus Villemoes

On Sun, 2021-03-07 at 20:14 +0100, Julia Lawall wrote:
> 
> On Wed, 3 Mar 2021, Joe Perches wrote:
> 
> > On Wed, 2021-03-03 at 10:41 +0100, Rasmus Villemoes wrote:
> > > On 02/03/2021 18.42, Joe Perches wrote:
> > > > Here is a possible opportunity to reduce data usage in the kernel.
> > > > 
> > > > $ git grep -P -n '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]' drivers/ | \
> > > >   grep -v __initdata | \
> > > >   wc -l
> > > > 3250
> > > > 
> > > > Meaning there are ~3000 declarations of arrays with what appears to be
> > > > file static const content that are not marked const.
> > > > 
> > > > So there are many static arrays that could be marked const to move the
> > > > compiled object code from data to text minimizing the total amount of
> > > > exposed r/w data.
> > > 
> > > You can add const if you like, but it will rarely change the generated
> > > code. gcc is already smart enough to take a static array whose contents
> > > are provably never modified within the TU and put it in .rodata:
> > 
> > At least some or perhaps even most of the time, true, but the gcc compiler
> > from v5 through at least v10 seems inconsistent about when it does the
> > appropriate conversion.
> > 
> > See the example I posted:
> > https://lore.kernel.org/lkml/6b8b250a06a98ce42120a14824531a8641f5e8aa.camel@perches.com/
> > 
> > It was a randomly chosen source file conversion btw, I had no prior
> > knowledge of whether the text/data use would change.
> > 
> > I'm unsure about clang consistently moving static but provably const arrays
> > from data to text.  I rarely use clang.  At least for v11 it seems to be
> > better though.  I didn't try 10.1.
> 
> I tried the relevnt drivers in drivers/input/joystick.  I got only one
> driver that changed with gcc 9.3, which was
> drivers/input/joystick/analog.c.  It actually got larger:
> 
> original:
> 
>    text    data     bss     dec     hex filename
>   22607   10560     320   33487    82cf drivers/input/joystick/analog.o
> 
> after adding const:
> 
>    text    data     bss     dec     hex filename
>   22728   10816     320   33864    8448 drivers/input/joystick/analog.o
> 
> This was the only case where bss was not 0, but I don't know if there is a
> connection.

You really need consider using defconfig so whatever object code
does not have tracing/debugging support.

For instance, this code with defconfig and analog joystick:

Original:

$ size drivers/input/joystick/analog.o
   text	   data	    bss	    dec	    hex	filename
   8115	    261	    224	   8600	   2198	drivers/input/joystick/analog.o

with const:

$ size drivers/input/joystick/analog.o
   text	   data	    bss	    dec	    hex	filename
   8179	    201	    224	   8604	   219c	drivers/input/joystick/analog.o


_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [Cocci] linux-kernel janitorial RFP: Mark static arrays as const
  2021-03-08  5:38       ` Joe Perches
@ 2021-03-08  6:54         ` Julia Lawall
  0 siblings, 0 replies; 13+ messages in thread
From: Julia Lawall @ 2021-03-08  6:54 UTC (permalink / raw)
  To: Joe Perches; +Cc: LKML, kernelnewbies, kernel-janitors, cocci, Rasmus Villemoes


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



On Sun, 7 Mar 2021, Joe Perches wrote:

> On Sun, 2021-03-07 at 20:14 +0100, Julia Lawall wrote:
> >
> > On Wed, 3 Mar 2021, Joe Perches wrote:
> >
> > > On Wed, 2021-03-03 at 10:41 +0100, Rasmus Villemoes wrote:
> > > > On 02/03/2021 18.42, Joe Perches wrote:
> > > > > Here is a possible opportunity to reduce data usage in the kernel.
> > > > >
> > > > > $ git grep -P -n '^static\s+(?!const|struct)(?:\w+\s+){1,3}\w+\s*\[\s*\]' drivers/ | \
> > > > >   grep -v __initdata | \
> > > > >   wc -l
> > > > > 3250
> > > > >
> > > > > Meaning there are ~3000 declarations of arrays with what appears to be
> > > > > file static const content that are not marked const.
> > > > >
> > > > > So there are many static arrays that could be marked const to move the
> > > > > compiled object code from data to text minimizing the total amount of
> > > > > exposed r/w data.
> > > >
> > > > You can add const if you like, but it will rarely change the generated
> > > > code. gcc is already smart enough to take a static array whose contents
> > > > are provably never modified within the TU and put it in .rodata:
> > >
> > > At least some or perhaps even most of the time, true, but the gcc compiler
> > > from v5 through at least v10 seems inconsistent about when it does the
> > > appropriate conversion.
> > >
> > > See the example I posted:
> > > https://lore.kernel.org/lkml/6b8b250a06a98ce42120a14824531a8641f5e8aa.camel@perches.com/
> > >
> > > It was a randomly chosen source file conversion btw, I had no prior
> > > knowledge of whether the text/data use would change.
> > >
> > > I'm unsure about clang consistently moving static but provably const arrays
> > > from data to text.  I rarely use clang.  At least for v11 it seems to be
> > > better though.  I didn't try 10.1.
> >
> > I tried the relevnt drivers in drivers/input/joystick.  I got only one
> > driver that changed with gcc 9.3, which was
> > drivers/input/joystick/analog.c.  It actually got larger:
> >
> > original:
> >
> >    text    data     bss     dec     hex filename
> >   22607   10560     320   33487    82cf drivers/input/joystick/analog.o
> >
> > after adding const:
> >
> >    text    data     bss     dec     hex filename
> >   22728   10816     320   33864    8448 drivers/input/joystick/analog.o
> >
> > This was the only case where bss was not 0, but I don't know if there is a
> > connection.
>
> You really need consider using defconfig so whatever object code
> does not have tracing/debugging support.
>
> For instance, this code with defconfig and analog joystick:
>
> Original:
>
> $ size drivers/input/joystick/analog.o
>    text	   data	    bss	    dec	    hex	filename
>    8115	    261	    224	   8600	   2198	drivers/input/joystick/analog.o
>
> with const:
>
> $ size drivers/input/joystick/analog.o
>    text	   data	    bss	    dec	    hex	filename
>    8179	    201	    224	   8604	   219c	drivers/input/joystick/analog.o

Thanks for the suggestion.  It occurred to me that in one case my
transformation was wrong, because the array was multi-level, and a sub
array was being stored in a structure field that was not declared as
const.  So the argument that the compiler would figure out to put the
array in .rodata didn't make sense.  But I still go the same sizes for
that file.  So I'll try the whole thing again.

thanks,
julia

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

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-02 17:42 [Cocci] linux-kernel janitorial RFP: Mark static arrays as const Joe Perches
2021-03-02 21:41 ` Julia Lawall
2021-03-03  2:47   ` Joe Perches
2021-03-02 22:18 ` Bernd Petrovitsch
2021-03-03  8:36   ` Julia Lawall
2021-03-04  8:34   ` Andy Shevchenko
2021-03-03  9:41 ` Rasmus Villemoes
2021-03-03 14:51   ` Joe Perches
2021-03-07 19:14     ` Julia Lawall
2021-03-08  5:38       ` Joe Perches
2021-03-08  6:54         ` Julia Lawall
2021-03-03 17:06 ` Mansour Moufid
2021-03-03 17:21   ` Julia Lawall

Coccinelle Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/cocci/0 cocci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 cocci cocci/ https://lore.kernel.org/cocci \
		cocci@systeme.lip6.fr
	public-inbox-index cocci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/fr.lip6.systeme.cocci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git