Skip to contentSkip to conversation

Don't let LATENCYTOP and LOCKDEP select KALLSYMS_ALL

PATCH In review Andi Kleen 4 participants 9 messages Apr 8, 2015 – Apr 16, 2015

Subsystems (2)
Library code Checkpatch
lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

PATCH 1/2 Don't let LATENCYTOP and LOCKDEP select KALLSYMS_ALL

From: Andi Kleen <[email protected]>

KALLSYMS_ALL enables including data variables into KALLSYMS. With plain KALLSYMS only functions are included.

LATENCYTOP and LOCKDEP select KALLSYMS_ALL in addition to KALLSYMS. It's unclear what they actually need _ALL for; they should only need function backtraces and afaik never touch variables.

LTO currently does not support KALLSYMS_ALL, which prevents LATENCYTOP and LOCKDEP from working and gives Kconfig errors. Disable the requirement for KALLSYMS_ALL for them, just use KALLSYMS.

Signed-off-by Andi Kleen <[email protected]>
---
 lib/Kconfig.debug | 2 --
 1 file changed, 2 deletions(-)
lib/Kconfig.debug +0 -2
index 3e0289e..062165c 100644
@@ -1001,7 +1001,6 @@ config LOCKDEP
 	select STACKTRACE
 	select FRAME_POINTER if !MIPS && !PPC && !ARM_UNWIND && !S390 && !MICROBLAZE && !ARC && !SCORE
 	select KALLSYMS
-	select KALLSYMS_ALL
 
 config LOCK_STAT
 	bool "Lock usage statistics"

@@ -1469,7 +1468,6 @@ config LATENCYTOP
 	depends on PROC_FS
 	select FRAME_POINTER if !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM_UNWIND && !ARC
 	select KALLSYMS
-	select KALLSYMS_ALL
 	select STACKTRACE
 	select SCHEDSTATS
 	select SCHED_DEBUG
Signature · 1 line
2.3.3
author
#1 lore raw

PATCH 2/2 test-hexdump.c: Fix initconst confusion

From: Andi Kleen <[email protected]>

const char *...[] is not const, but an array of pointer to const. So these arrays cannot be __initconst, but must be __initdata

This fixes section conflicts with LTO.

Signed-off-by Andi Kleen <[email protected]>
---
 lib/test-hexdump.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
lib/test-hexdump.c +4 -4
index daf29a39..9846ff7 100644
@@ -18,26 +18,26 @@ static const unsigned char data_b[] = {
 
 static const unsigned char data_a[] = ".2.{....p..$}.4...1.....L...C...";
 
-static const char *test_data_1_le[] __initconst = {
+static const char * const test_data_1_le[] __initconst = {
 	"be", "32", "db", "7b", "0a", "18", "93", "b2",
 	"70", "ba", "c4", "24", "7d", "83", "34", "9b",
 	"a6", "9c", "31", "ad", "9c", "0f", "ac", "e9",
 	"4c", "d1", "19", "99", "43", "b1", "af", "0c",
 };
 
-static const char *test_data_2_le[] __initconst = {
+static const char *test_data_2_le[] __initdata = {
 	"32be", "7bdb", "180a", "b293",
 	"ba70", "24c4", "837d", "9b34",
 	"9ca6", "ad31", "0f9c", "e9ac",
 	"d14c", "9919", "b143", "0caf",
 };
 
-static const char *test_data_4_le[] __initconst = {
+static const char *test_data_4_le[] __initdata = {
 	"7bdb32be", "b293180a", "24c4ba70", "9b34837d",
 	"ad319ca6", "e9ac0f9c", "9919d14c", "0cafb143",
 };
 
-static const char *test_data_8_le[] __initconst = {
+static const char *test_data_8_le[] __initdata = {
 	"b293180a7bdb32be", "9b34837d24c4ba70",
 	"e9ac0f9cad319ca6", "0cafb1439919d14c",
 };
Signature · 1 line
2.3.3
maintainer
#2 lore raw

Re: [PATCH 2/2] test-hexdump.c: Fix initconst confusion

Quoted 10 lines · On Wed, 8 Apr 2015 06:06:45 -0700 Andi Kleen <[email protected]> wrote:

From: Andi Kleen <[email protected]>

const char *...[] is not const, but an array of pointer to const. So these arrays cannot be __initconst, but must be __initdata

This fixes section conflicts with LTO.

lib/test-hexdump.c static const unsigned char data_b[] = { · line 21

Quoted 5 lines
lib/test-hexdump.c +1 -1
@@ -18,26 +18,26 @@ static const unsigned char data_b[] = {
 
 static const unsigned char data_a[] = ".2.{....p..$}.4...1.....L...C...";
 
-static const char *test_data_1_le[] __initconst = {
+static const char * const test_data_1_le[] __initconst = {

const char * const __initconst

lib/test-hexdump.c static const unsigned char data_b[] = { · line 40

Quoted 9 lines
lib/test-hexdump.c +3 -0
 	"be", "32", "db", "7b", "0a", "18", "93", "b2",
 	"70", "ba", "c4", "24", "7d", "83", "34", "9b",
 	"a6", "9c", "31", "ad", "9c", "0f", "ac", "e9",
 	"4c", "d1", "19", "99", "43", "b1", "af", "0c",
 };
 
+static const char *test_data_2_le[] __initdata = {
+static const char *test_data_4_le[] __initdata = {
+static const char *test_data_8_le[] __initdata = {

const char * __initdata

Why is test_data_1_le[] different?

Can we make them all "const char * const __initconst"? That would make checkpatch happy ;)

author
#3 lore raw

Re: [PATCH 2/2] test-hexdump.c: Fix initconst confusion

Quoted 11 lines · Andrew Morton

lib/test-hexdump.c static const unsigned char data_b[] = { · line 21

Quoted 9 lines
lib/test-hexdump.c +1 -1
@@ -18,26 +18,26 @@ static const unsigned char data_b[] = {
 
 static const unsigned char data_a[] = ".2.{....p..$}.4...1.....L...C...";
 
-static const char *test_data_1_le[] __initconst = {
+static const char * const test_data_1_le[] __initconst = {

const char * const __initconst

This one didn't cause any warnings elsewhere.

Quoted 13 lines

lib/test-hexdump.c static const unsigned char data_b[] = { · line 40

Quoted 10 lines
lib/test-hexdump.c +3 -0
 	"be", "32", "db", "7b", "0a", "18", "93", "b2",
 	"70", "ba", "c4", "24", "7d", "83", "34", "9b",
 	"a6", "9c", "31", "ad", "9c", "0f", "ac", "e9",
 	"4c", "d1", "19", "99", "43", "b1", "af", "0c",
 };
 
+static const char *test_data_2_le[] __initdata = {
+static const char *test_data_4_le[] __initdata = {
+static const char *test_data_8_le[] __initdata = {

const char * __initdata

Quoted 4 lines

Why is test_data_1_le[] different?

Can we make them all "const char * const __initconst"? That would make checkpatch happy ;)

I tried it, but it would have needed a lot more changes to shut up warnings later in the code. This was the least intrusive.

checkpatch is a bit stupid about this, but then C declarations are difficult to parse...

-Andi

Signature · 1 line
[email protected] -- Speaking for myself only.
maintainer
#4 lore raw

PATCH 1/1 checkpatch: Add a test for const with __read_mostly uses

const objects shouldn't be __read_mostly. They are read-only.

Marking these objects as __read_mostly causes section conflicts with LTO linking.

So add a test to try to avoid this issue.

Signed-off-by Joe Perches <[email protected]>
---
 scripts/checkpatch.pl | 10 ++++++++++
 1 file changed, 10 insertions(+)
scripts/checkpatch.pl +10 -0
index 9a8b2bd..199d81b 100755
@@ -4777,6 +4777,16 @@ sub process {
 			}
 		}
 
+# check for __read_mostly with const non-pointer (should just be const)
+		if ($line =~ /\b__read_mostly\b/ &&
+		    $line =~ /($Type)\s*$Ident/ && $1 !~ /\*\s*$/ && $1 =~ /\bconst\b/) {
+			if (ERROR("CONST_READ_MOSTLY",
+				  "Invalid use of __read_mostly with const type\n" . $herecurr) &&
+			    $fix) {
+				$fixed[$fixlinenr] =~ s/\s+__read_mostly\b//;
+			}
+		}
+
 # don't use __constant_<foo> functions outside of include/uapi/
 		if ($realfile !~ m@^include/uapi/@ &&
 		    $line =~ /(__constant_(?:htons|ntohs|[bl]e(?:16|32|64)_to_cpu|cpu_to_[bl]e(?:16|32|64)))\s*\(/) {
author
#5 lore raw

Re: [PATCH] checkpatch: Add a test for const with __read_mostly uses

Quoted 6 lines · Joe Perches

const objects shouldn't be __read_mostly. They are read-only.

Marking these objects as __read_mostly causes section conflicts with LTO linking.

So add a test to try to avoid this issue.

Thanks Joe. Looks good thanks. I suspect excluding * will miss quite a few cases, but there's no way around it.

-Andi

maintainer
#6 lore raw

Re: [PATCH] checkpatch: Add a test for const with __read_mostly uses

Quoted 10 lines · Andi Kleen
Quoted 6 lines · Joe Perches

const objects shouldn't be __read_mostly. They are read-only.

Marking these objects as __read_mostly causes section conflicts with LTO linking.

So add a test to try to avoid this issue.

I suspect excluding *
will miss quite a few cases, but there's no way around it.

That excludes only "const <foo> *" not "const foo * const" so I believe it shouldn't exclude anything that's not pointer to const.

#7 lore raw

Re: [PATCH 2/2] test-hexdump.c: Fix initconst confusion

In reply to Andi Kleen
Quoted 28 lines · Andi Kleen
Quoted 11 lines · Andrew Morton

lib/test-hexdump.c static const unsigned char data_b[] = { · line 21

Quoted 9 lines
lib/test-hexdump.c +1 -1
@@ -18,26 +18,26 @@ static const unsigned char data_b[] = {
 
 static const unsigned char data_a[] = ".2.{....p..$}.4...1.....L...C...";
 
-static const char *test_data_1_le[] __initconst = {
+static const char * const test_data_1_le[] __initconst = {

const char * const __initconst

This one didn't cause any warnings elsewhere.

Quoted 13 lines

lib/test-hexdump.c static const unsigned char data_b[] = { · line 40

Quoted 10 lines
lib/test-hexdump.c +3 -0
    "be", "32", "db", "7b", "0a", "18", "93", "b2",
    "70", "ba", "c4", "24", "7d", "83", "34", "9b",
    "a6", "9c", "31", "ad", "9c", "0f", "ac", "e9",
    "4c", "d1", "19", "99", "43", "b1", "af", "0c",
 };
 
+static const char *test_data_2_le[] __initdata = {
+static const char *test_data_4_le[] __initdata = {
+static const char *test_data_8_le[] __initdata = {

const char * __initdata

Quoted 7 lines
Quoted 4 lines

Why is test_data_1_le[] different?

Can we make them all "const char * const __initconst"? That would make checkpatch happy ;)

I tried it, but it would have needed a lot more changes to shut up warnings later in the code. This was the least intrusive.

"a lot more changes" is "one more change"? (patch sent)

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

author
#8 lore raw

Re: [PATCH 2/2] test-hexdump.c: Fix initconst confusion

Quoted 4 lines
Quoted 2 lines

I tried it, but it would have needed a lot more changes to shut up warnings later in the code. This was the least intrusive.

"a lot more changes" is "one more change"? (patch sent)

Thanks Geert. You're right of course.
-Andi