Skip to contentSkip to conversation

srcu: Isolate srcu sections using CONFIG_SRCU

PATCH Superseded Pranith Kumar 4 participants 9 messages Dec 27, 2014 – Dec 30, 2014

Subsystems (3)
PowerPC s390 Sleepable read-copy update (SRCU)

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Pranith Kumar

author
#0 lore raw

PATCH 1/1 srcu: Isolate srcu sections using CONFIG_SRCU

Isolate the SRCU functions and data structures within CONFIG_SRCU so that there is a compile time failure if srcu is used when not enabled. This was decided to be better than waiting until link time for a failure to occur.

There are places which include kvm headers and utilize kvm data structures without checking if KVM is enabled. In two such archs(s390, ppc64), the current patch makes the uses of KVM conditional on KVM being enabled. The other option, which is to enable KVM unconditionally seemed a bit too much as we could easily figure out KVM only parts and enclose them in ifdefs.

Signed-off-by Pranith Kumar <[email protected]>
---
 arch/powerpc/kernel/setup_64.c |  7 ++++++-
 arch/s390/kernel/asm-offsets.c |  7 ++++++-
 include/linux/notifier.h       | 47 ++++++++++++++++++++++++------------------
 include/linux/srcu.h           |  6 +++++-
 4 files changed, 44 insertions(+), 23 deletions(-)
arch/powerpc/kernel/setup_64.c +6 -1
index 4f3cfe1..f55302f 100644
@@ -65,10 +65,13 @@
 #include <asm/kexec.h>
 #include <asm/mmu_context.h>
 #include <asm/code-patching.h>
-#include <asm/kvm_ppc.h>
 #include <asm/hugetlb.h>
 #include <asm/epapr_hcalls.h>
 
+#if IS_ENABLED(CONFIG_KVM)
+#include <asm/kvm_ppc.h>
+#endif
+
 #ifdef DEBUG
 #define DBG(fmt...) udbg_printf(fmt)
 #else

@@ -286,8 +289,10 @@ void __init early_setup(unsigned long dt_ptr)
 	 */
 	cpu_ready_for_interrupts();
 
+#if IS_ENABLED(CONFIG_KVM)
 	/* Reserve large chunks of memory for use by CMA for KVM */
 	kvm_cma_reserve();
+#endif
 
 	/*
 	 * Reserve any gigantic pages requested on the command line.
arch/s390/kernel/asm-offsets.c +6 -1
index ef279a1..2813a3c 100644
@@ -7,12 +7,15 @@
 #define ASM_OFFSETS_C
 
 #include <linux/kbuild.h>
-#include <linux/kvm_host.h>
 #include <linux/sched.h>
 #include <asm/idle.h>
 #include <asm/vdso.h>
 #include <asm/pgtable.h>
 
+#if IS_ENABLED(CONFIG_KVM)
+#include <linux/kvm_host.h>
+#endif
+
 /*
  * Make sure that the compiler is new enough. We want a compiler that
  * is known to work with the "Q" assembler constraint.

@@ -182,8 +185,10 @@ int main(void)
 	DEFINE(__LC_PGM_TDB, offsetof(struct _lowcore, pgm_tdb));
 	DEFINE(__THREAD_trap_tdb, offsetof(struct task_struct, thread.trap_tdb));
 	DEFINE(__GMAP_ASCE, offsetof(struct gmap, asce));
+#if IS_ENABLED(CONFIG_KVM)
 	DEFINE(__SIE_PROG0C, offsetof(struct kvm_s390_sie_block, prog0c));
 	DEFINE(__SIE_PROG20, offsetof(struct kvm_s390_sie_block, prog20));
+#endif /* CONFIG_KVM */
 #endif /* CONFIG_32BIT */
 	return 0;
 }
include/linux/notifier.h +27 -20

94 lines not on this page · raw message

include/linux/srcu.h +5 -1
index 9cfd962..ed9c389 100644
@@ -26,6 +26,8 @@
  *
  */
 
+#ifdef CONFIG_SRCU
+
 #ifndef _LINUX_SRCU_H
 #define _LINUX_SRCU_H
 

@@ -249,4 +251,6 @@ static inline void smp_mb__after_srcu_read_unlock(void)
 	/* __srcu_read_unlock has smp_mb() internally so nothing to do here. */
 }
 
-#endif
+#endif /* _LINUX_SRCU_H */
+
+#endif /* CONFIG_SRCU */
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH 1/1 powerpc/powernv: Select CONFIG_PRINTK to fix build failure

In an allnoconfig we get the following build failure:

arch/powerpc/platforms/built-in.o: In function `.__machine_initcall_powernv_opal_init':
opal.c:(.init.text+0x468): undefined reference to .log_buf_addr_get' opal.c:(.init.text+0x474): undefined reference to .log_buf_len_get'
make: *** [vmlinux] Error 1

This happens because powernv requires printk() support. Enable it in the config file.

Signed-off-by Pranith Kumar <[email protected]>
---
 arch/powerpc/platforms/powernv/Kconfig | 1 +
 1 file changed, 1 insertion(+)
arch/powerpc/platforms/powernv/Kconfig +1 -0
index 45a8ed0..be4f340 100644
@@ -18,6 +18,7 @@ config PPC_POWERNV
 	select CPU_FREQ_GOV_ONDEMAND
 	select CPU_FREQ_GOV_CONSERVATIVE
 	select PPC_DOORBELL
+	select PRINTK
 	default y
 
 config PPC_POWERNV_RTAS
Signature · 1 line
1.9.1
reviewer
#2 lore raw

Re: [PATCH] powerpc/powernv: Select CONFIG_PRINTK to fix build failure

Quoted 1 line · Pranith Kumar

In an allnoconfig we get the following build failure:

An allnoconfig doesn't include CONFIG_PPC_POWERNV? But I think I know what you mean.

Quoted 7 lines

arch/powerpc/platforms/built-in.o: In function `.__machine_initcall_powernv_opal_init':
opal.c:(.init.text+0x468): undefined reference to .log_buf_addr_get' opal.c:(.init.text+0x474): undefined reference to .log_buf_len_get'
make: *** [vmlinux] Error 1

This happens because powernv requires printk() support. Enable it in the config file.

Sort of. It just requires those two routines. Or is there a stronger dependency?

I think the better fix is for those two routines to be defined for CONFIG_PRINTK=n, but return NULL and zero respectively.

And the opal code could skip registering the region when they return NULL/zero.

Care to do a couple of patches?

cheers

author
#3 lore raw

Re: [PATCH] powerpc/powernv: Select CONFIG_PRINTK to fix build failure

Quoted 17 lines · Michael Ellerman
Quoted 1 line · Pranith Kumar

In an allnoconfig we get the following build failure:

An allnoconfig doesn't include CONFIG_PPC_POWERNV? But I think I know what you mean.

Quoted 7 lines

arch/powerpc/platforms/built-in.o: In function `.__machine_initcall_powernv_opal_init':
opal.c:(.init.text+0x468): undefined reference to .log_buf_addr_get' opal.c:(.init.text+0x474): undefined reference to .log_buf_len_get'
make: *** [vmlinux] Error 1

This happens because powernv requires printk() support. Enable it in the config file.

Sort of. It just requires those two routines. Or is there a stronger dependency?

Quoted 7 lines

I think the better fix is for those two routines to be defined for CONFIG_PRINTK=n, but return NULL and zero respectively.

And the opal code could skip registering the region when they return NULL/zero.

Care to do a couple of patches?

Sure, I will give it a try. Thanks for the review!

Signature · 1 line
Pranith
#4 lore raw
In reply to Pranith Kumar
Quoted 5 lines · On Sat, 27 Dec 2014 12:17:43 -0500 Pranith Kumar <[email protected]> wrote:
arch/powerpc/kernel/setup_64.c +0 -1
@@ -65,10 +65,13 @@
 #include <asm/kexec.h>
 #include <asm/mmu_context.h>
 #include <asm/code-patching.h>
-#include <asm/kvm_ppc.h>

arch/powerpc/kernel/setup_64.c line 77

Quoted 10 lines
arch/powerpc/kernel/setup_64.c +4 -0
 #include <asm/hugetlb.h>
 #include <asm/epapr_hcalls.h>
 
+#if IS_ENABLED(CONFIG_KVM)
+#include <asm/kvm_ppc.h>
+#endif
+
 #ifdef DEBUG
 #define DBG(fmt...) udbg_printf(fmt)
 #else

I always cringe when I see an include protected by an #ifdef. Is this really necessary? All that is done in asm-offsets.c is to calculate offsets, the code where the two offsets in question are used (entry64.S) does have the #ifdef for CONFIG_KVM.

Signature · 4 lines
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.
author
#5 lore raw
Quoted 11 lines · Martin Schwidefsky
Quoted 8 lines · On Sat, 27 Dec 2014 12:17:43 -0500 Pranith Kumar <[email protected]> wrote:
arch/powerpc/kernel/setup_64.c +0 -1
@@ -65,10 +65,13 @@
 #include <asm/kexec.h>
 #include <asm/mmu_context.h>
 #include <asm/code-patching.h>
-#include <asm/kvm_ppc.h>
 #include <asm/hugetlb.h>
 #include <asm/epapr_hcalls.h>
Quoted 12 lines

arch/powerpc/kernel/setup_64.c line 77

Quoted 7 lines
arch/powerpc/kernel/setup_64.c +4 -0
+#if IS_ENABLED(CONFIG_KVM)
+#include <asm/kvm_ppc.h>
+#endif
+
 #ifdef DEBUG
 #define DBG(fmt...) udbg_printf(fmt)
 #else

I always cringe when I see an include protected by an #ifdef. Is this really necessary? All that is done in asm-offsets.c is to calculate offsets, the code where the two offsets in question are used (entry64.S) does have the #ifdef for CONFIG_KVM.

I agree that this is not the ideal way to do this. But, it has been the way things were already being done. If you see arch/powerpc/kernel/asm-offsets.c, there are quite some includes which are within ifdefs.

I've considered other alternatives (though not in-depth) and found that they will require quite some refactoring. One simple idea is to move this #ifdef to within kvm_ppc.h. That should make the inclusion of this file a no-op in all the places where this is being included without KVM being enabled. But I am not 100% sure of that approach.

Any suggestions are welcome.

Signature · 1 line
Pranith
#6 lore raw
Quoted 26 lines · Pranith Kumar
Quoted 11 lines · Martin Schwidefsky
Quoted 8 lines · On Sat, 27 Dec 2014 12:17:43 -0500 Pranith Kumar <[email protected]> wrote:
arch/powerpc/kernel/setup_64.c +0 -1
@@ -65,10 +65,13 @@
 #include <asm/kexec.h>
 #include <asm/mmu_context.h>
 #include <asm/code-patching.h>
-#include <asm/kvm_ppc.h>
 #include <asm/hugetlb.h>
 #include <asm/epapr_hcalls.h>
Quoted 12 lines

arch/powerpc/kernel/setup_64.c line 77

Quoted 7 lines
arch/powerpc/kernel/setup_64.c +4 -0
+#if IS_ENABLED(CONFIG_KVM)
+#include <asm/kvm_ppc.h>
+#endif
+
 #ifdef DEBUG
 #define DBG(fmt...) udbg_printf(fmt)
 #else

I always cringe when I see an include protected by an #ifdef. Is this really necessary? All that is done in asm-offsets.c is to calculate offsets, the code where the two offsets in question are used (entry64.S) does have the #ifdef for CONFIG_KVM.

Quoted 4 lines

I agree that this is not the ideal way to do this. But, it has been the way things were already being done. If you see arch/powerpc/kernel/asm-offsets.c, there are quite some includes which are within ifdefs.

asm-offsets.c is unusual in that respect, and I think most of those ifdefs could go away without breaking anything (head_booke.h is not a normal header file, and kvm_book3s.h should just be removed as it will be pulled in by kvm_ppc.h if applicable).

Quoted 7 lines

I've considered other alternatives (though not in-depth) and found that they will require quite some refactoring. One simple idea is to move this #ifdef to within kvm_ppc.h. That should make the inclusion of this file a no-op in all the places where this is being included without KVM being enabled. But I am not 100% sure of that approach.

Any suggestions are welcome.

As I suggested elsewhere in the thread, why not be more fine-grained in what you ifdef in the srcu header? How will that require excessive refactoring?

Or, just stick with the linker error.

I also wonder if this is worthwhile just to save around 2000 bytes. What other core synchronization mechanisms are optional? What real-world configs will actually have this disabled?

-Scott

#7 lore raw
In reply to Pranith Kumar
Quoted 3 lines · Pranith Kumar

Isolate the SRCU functions and data structures within CONFIG_SRCU so that there is a compile time failure if srcu is used when not enabled. This was decided to be better than waiting until link time for a failure to occur.

Yes, false positives and extra ifdefs are so much better. :-P

Why not just ifdef the functions/macros, and leave the types alone? If you're worried about direct access to struct members, you could even ifdef the members away while leaving the struct itself. It is not normal practice in Linux to need ifdefs around #includes.

Quoted 5 lines

There are places which include kvm headers and utilize kvm data structures without checking if KVM is enabled. In two such archs(s390, ppc64), the current patch makes the uses of KVM conditional on KVM being enabled. The other option, which is to enable KVM unconditionally seemed a bit too much as we could easily figure out KVM only parts and enclose them in ifdefs.

Maybe not so easy (mpc85xx_smp_defconfig with NOTIFY stuff turned off so that SRCU gets deselected):

In file included from /home/scott/fsl/git/linux/upstream/arch/powerpc/include/asm/kvm_ppc.h:30:0,
from /home/scott/fsl/git/linux/upstream/arch/powerpc/kernel/smp.c:39:
/home/scott/fsl/git/linux/upstream/include/linux/kvm_host.h:366:21: error: field 'srcu' has incomplete type /home/scott/fsl/git/linux/upstream/include/linux/kvm_host.h:367:21: error: field 'irq_srcu' has incomplete type /home/scott/fsl/git/linux/upstream/scripts/Makefile.build:257: recipe for target 'arch/powerpc/kernel/smp.o' failed make[2]: *** [arch/powerpc/kernel/smp.o] Error 1 /home/scott/fsl/git/linux/upstream/Makefile:955: recipe for target 'arch/powerpc/kernel' failed make[1]: *** [arch/powerpc/kernel] Error 2
make[1]: *** Waiting for unfinished jobs....

Are you sure KVM is the only SRCU user so impacted? It's also likely that new such problems get introduced, because most people are going to have SRCU enabled and thus not notice the breakage they're adding.

There's also at least one place that needs to be fixed, that currently expects to get other headers indirectly via srcu.h:

/home/scott/fsl/git/linux/upstream/lib/assoc_array.c: In function 'assoc_array_apply_edit':
/home/scott/fsl/git/linux/upstream/lib/assoc_array.c:1425:2: error: implicit declaration of function 'call_rcu' [-Werror=implicit-function-declaration]
cc1: some warnings being treated as errors
/home/scott/fsl/git/linux/upstream/scripts/Makefile.build:257: recipe for target 'lib/assoc_array.o' failed

-Scott

author
#8 lore raw
Quoted 11 lines · Scott Wood
Quoted 3 lines · Pranith Kumar

Isolate the SRCU functions and data structures within CONFIG_SRCU so that there is a compile time failure if srcu is used when not enabled. This was decided to be better than waiting until link time for a failure to occur.

Yes, false positives and extra ifdefs are so much better. :-P

Why not just ifdef the functions/macros, and leave the types alone? If you're worried about direct access to struct members, you could even ifdef the members away while leaving the struct itself. It is not normal practice in Linux to need ifdefs around #includes.

Yup, totally agree that this is not ideal. The idea here is to not even compile the structure for tinification purposes. ifdefs for headers are ugly, but given the current code structure, I was not able to figure out any other way around it without major overhaul.

Quoted 20 lines
Quoted 6 lines

There are places which include kvm headers and utilize kvm data structures without checking if KVM is enabled. In two such archs(s390, ppc64), the current patch makes the uses of KVM conditional on KVM being enabled. The other option, which is to enable KVM unconditionally seemed a bit too much as we could easily figure out KVM only parts and enclose them in ifdefs.

Maybe not so easy (mpc85xx_smp_defconfig with NOTIFY stuff turned off so that SRCU gets deselected):

In file included from /home/scott/fsl/git/linux/upstream/arch/powerpc/include/asm/kvm_ppc.h:30:0,
from /home/scott/fsl/git/linux/upstream/arch/powerpc/kernel/smp.c:39:
/home/scott/fsl/git/linux/upstream/include/linux/kvm_host.h:366:21: error: field 'srcu' has incomplete type /home/scott/fsl/git/linux/upstream/include/linux/kvm_host.h:367:21: error: field 'irq_srcu' has incomplete type /home/scott/fsl/git/linux/upstream/scripts/Makefile.build:257: recipe for target 'arch/powerpc/kernel/smp.o' failed make[2]: *** [arch/powerpc/kernel/smp.o] Error 1 /home/scott/fsl/git/linux/upstream/Makefile:955: recipe for target 'arch/powerpc/kernel' failed make[1]: *** [arch/powerpc/kernel] Error 2
make[1]: *** Waiting for unfinished jobs....

Quoted 3 lines

Are you sure KVM is the only SRCU user so impacted? It's also likely that new such problems get introduced, because most people are going to have SRCU enabled and thus not notice the breakage they're adding.

Well, it is the major one which I encountered until now. There might be other problems lurking which I will gladly try to fix if and when they are reported.

Quoted 8 lines

There's also at least one place that needs to be fixed, that currently expects to get other headers indirectly via srcu.h:

/home/scott/fsl/git/linux/upstream/lib/assoc_array.c: In function 'assoc_array_apply_edit':
/home/scott/fsl/git/linux/upstream/lib/assoc_array.c:1425:2: error: implicit declaration of function 'call_rcu' [-Werror=implicit-function-declaration]
cc1: some warnings being treated as errors
/home/scott/fsl/git/linux/upstream/scripts/Makefile.build:257: recipe for target 'lib/assoc_array.o' failed

I will send a patch fixing this(need to use rcupdate.h here explicitly). Thanks for reporting these!

Quoted 4 lines

-Scott

Signature · 1 line
Pranith