Make call_srcu() available during very early boot

Event tracing is moving to SRCU in order to take advantage of the fact that SRCU may be safely used from idle and even offline CPUs. However, event tracing can invoke call_srcu() very early in the boot process, even before workqueue_init_early() is invoked (let alone rcu_init()). Therefore, call_srcu()'s attempts to queue work fail miserably.

This commit therefore detects this situation, and refrains from attempting to queue work before rcu_init() time, but does everything else that it would have done, and in addition, adds the srcu_struct to a global list. The rcu_init() function now invokes a new srcu_init() function, which is empty if CONFIG_SRCU=n. Otherwise, srcu_init() queues work for each srcu_struct on the list. This all happens early enough in boot that there is but a single CPU with interrupts disabled, which allows synchronization to be dispensed with.

Of course, the queued work won't actually be invoked until after workqueue_init() is invoked, which happens shortly after the scheduler is up and running. This means that although call_srcu() may be invoked any time after per-CPU variables have been set up, there is still a very narrow window when synchronize_srcu() won't work, and this window extends from the time that the scheduler starts until the time that workqueue_init() returns. This can be fixed in a manner similar to the fix for synchronize_rcu_expedited() and friends, but until someone actually needs to use synchronize_srcu() during this window, this fix is added churn for no benefit.

Finally, note that Tree SRCU's new srcu_init() function invokes queue_work() rather than the queue_delayed_work() function that is invoked post-boot. The reason is that queue_delayed_work() will (as you would expect) post a timer, and timers have not yet been initialized. So use of queue_delayed_work() avoids the complaints about use of uninitialized

spinlocks that would otherwise result. Besides, delay is in any case provide by the aforementioned fact that the queued work won't actually be invoked until after the scheduler is up and running.

Requested-by Steven Rostedt <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

include/linux/srcutiny.h +0 -0

index f41d2fb09f87..2b5c0822e683 100644

include/linux/srcutiny.h +1 -0

 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 	struct lockdep_map dep_map;
 #endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */

@@ -48,6 +49,7 @@ void srcu_drive_gp(struct work_struct *wp);
 	.srcu_wq = __SWAIT_QUEUE_HEAD_INITIALIZER(name.srcu_wq),	\
 	.srcu_cb_tail = &name.srcu_cb_head,				\
 	.srcu_work = __WORK_INITIALIZER(name.srcu_work, srcu_drive_gp),	\
+	.srcu_boot_entry = LIST_HEAD_INIT(name.srcu_boot_entry),	\
 	__SRCU_DEP_MAP_INIT(name)					\
 }
 

include/linux/srcutree.h +1 -0

index 745d4ca4dd50..86ad97111315 100644
@@ -94,6 +94,7 @@ struct srcu_struct {
 						/*  callback for the barrier */
 						/*  operation. */
 	struct delayed_work work;
+	struct list_head srcu_boot_entry;	/* Early-boot callbacks. */
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 	struct lockdep_map dep_map;
 #endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */

@@ -105,12 +106,13 @@ struct srcu_struct {
 #define SRCU_STATE_SCAN2	2
 
 #define __SRCU_STRUCT_INIT(name, pcpu_name)				\

Quoted 26 lines · On Tue, 14 Aug 2018 09:24:48 -0700 "Paul E. McKenney"…

Event tracing is moving to SRCU in order to take advantage of the fact that SRCU may be safely used from idle and even offline CPUs. However, event tracing can invoke call_srcu() very early in the boot process, even before workqueue_init_early() is invoked (let alone rcu_init()). Therefore, call_srcu()'s attempts to queue work fail miserably.

This commit therefore detects this situation, and refrains from attempting to queue work before rcu_init() time, but does everything else that it would have done, and in addition, adds the srcu_struct to a global list. The rcu_init() function now invokes a new srcu_init() function, which is empty if CONFIG_SRCU=n. Otherwise, srcu_init() queues work for each srcu_struct on the list. This all happens early enough in boot that there is but a single CPU with interrupts disabled, which allows synchronization to be dispensed with.

Of course, the queued work won't actually be invoked until after workqueue_init() is invoked, which happens shortly after the scheduler is up and running. This means that although call_srcu() may be invoked any time after per-CPU variables have been set up, there is still a very narrow window when synchronize_srcu() won't work, and this window extends from the time that the scheduler starts until the time that workqueue_init() returns. This can be fixed in a manner similar to the fix for synchronize_rcu_expedited() and friends, but until someone actually needs to use synchronize_srcu() during this window, this fix is added churn for no benefit.

Quoted 5 lines

Finally, note that Tree SRCU's new srcu_init() function invokes queue_work() rather than the queue_delayed_work() function that is invoked post-boot. The reason is that queue_delayed_work() will (as you would expect) post a timer, and timers have not yet been initialized. So use of queue_delayed_work() avoids the complaints about use of uninitialized

You mean "So use of queue_work() avoids .." ?

Quoted 12 lines

spinlocks that would otherwise result. Besides, delay is in any case provide by the aforementioned fact that the queued work won't actually be invoked until after the scheduler is up and running.

Requested-by Steven Rostedt <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

include/linux/srcutiny.h +0 -0

index f41d2fb09f87..2b5c0822e683 100644

include/linux/srcutiny.h struct srcu_struct { · line 39

Quoted 4 lines

include/linux/srcutiny.h +1 -0

@@ -36,6 +36,7 @@ struct srcu_struct {
 	struct rcu_head *srcu_cb_head;	/* Pending callbacks: Head. */
 	struct rcu_head **srcu_cb_tail;	/* Pending callbacks: Tail. */
 	struct work_struct srcu_work;	/* For driving grace periods. */
+	struct list_head srcu_boot_entry; /* Early-boot callbacks. */

I really don't like increasing the size of a structure for a field that is hardly ever used.

Is there a way we could make a union, or reuse one of the other fields, as we know that synchronize_srcu() can't be used yet (and if it is, either warn, or just make it a nop). And when we call srcu_init() and remove the srcu_struct from the list, we can then initialize whatever we used as the temporary boot up list field.

srcu_init is called when we are still running only one CPU correct?

Quoted 26 lines

include/linux/srcutiny.h +1 -0

 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 	struct lockdep_map dep_map;
 #endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */

@@ -48,6 +49,7 @@ void srcu_drive_gp(struct work_struct *wp);
 	.srcu_wq = __SWAIT_QUEUE_HEAD_INITIALIZER(name.srcu_wq),	\
 	.srcu_cb_tail = &name.srcu_cb_head,				\
 	.srcu_work = __WORK_INITIALIZER(name.srcu_work, srcu_drive_gp),	\
+	.srcu_boot_entry = LIST_HEAD_INIT(name.srcu_boot_entry),	\
 	__SRCU_DEP_MAP_INIT(name)					\
 }
 

include/linux/srcutree.h +1 -0

index 745d4ca4dd50..86ad97111315 100644
@@ -94,6 +94,7 @@ struct srcu_struct {
 						/*  callback for the barrier */
 						/*  operation. */
 	struct delayed_work work;
+	struct list_head srcu_boot_entry;	/* Early-boot callbacks. */
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 	struct lockdep_map dep_map;
 #endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */

@@ -105,12 +106,13 @@ struct srcu_struct {
 #define SRCU_STATE_SCAN2	2
 

include/linux/srcutree.h struct srcu_struct { · line 112

Quoted 11 lines

include/linux/srcutree.h +4 -6

 #define __SRCU_STRUCT_INIT(name, pcpu_name)				\
-	{								\
-		.sda = &pcpu_name,					\
-		.lock = __SPIN_LOCK_UNLOCKED(name.lock),		\
-		.srcu_gp_seq_needed = 0 - 1,				\
-		__SRCU_DEP_MAP_INIT(name)				\
-	}
+{									\
+	.sda = &pcpu_name,						\
+	.lock = __SPIN_LOCK_UNLOCKED(name.lock),			\
+	.srcu_gp_seq_needed = 0 - 1,					\

Interesting initialization of -1. This was there before, but still interesting none the less.

include/linux/srcutree.h struct srcu_struct {

Quoted 5 lines

include/linux/srcutree.h +3 -0

+	.srcu_boot_entry = LIST_HEAD_INIT(name.srcu_boot_entry),	\
+	__SRCU_DEP_MAP_INIT(name)					\
+}
 
 

-- Steve

Quoted 6 lines

Quoted 6 lines

#define __SRCU_STRUCT_INIT(name, pcpu_name) \

include/linux/srcutree.h +0 -5

-	{								\
-		.sda = &pcpu_name,					\
-		.lock = __SPIN_LOCK_UNLOCKED(name.lock),		\
-		.srcu_gp_seq_needed = 0 - 1,				\
-		__SRCU_DEP_MAP_INIT(name)				\

Quoted 8 lines

include/linux/srcutree.h struct srcu_struct { · line 112

Quoted 5 lines

include/linux/srcutree.h +4 -1

-	}
+{									\
+	.sda = &pcpu_name,						\
+	.lock = __SPIN_LOCK_UNLOCKED(name.lock),			\
+	.srcu_gp_seq_needed = 0 - 1,					\  

Interesting initialization of -1. This was there before, but still interesting none the less.

If I recall correctly, this subterfuge suppresses compiler complaints about initializing an unsigned long with a negative number. :-/

Quoted 15 lines · On Tue, 14 Aug 2018 10:06:18 -0700 "Paul E. McKenney"…

Quoted 14 lines

include/linux/srcutree.h struct srcu_struct { · line 112

Quoted 11 lines

#define __SRCU_STRUCT_INIT(name, pcpu_name) \

include/linux/srcutree.h +4 -6

-	{								\
-		.sda = &pcpu_name,					\
-		.lock = __SPIN_LOCK_UNLOCKED(name.lock),		\
-		.srcu_gp_seq_needed = 0 - 1,				\
-		__SRCU_DEP_MAP_INIT(name)				\
-	}
+{									\
+	.sda = &pcpu_name,						\
+	.lock = __SPIN_LOCK_UNLOCKED(name.lock),			\
+	.srcu_gp_seq_needed = 0 - 1,					\  

Interesting initialization of -1. This was there before, but still interesting none the less.

Quoted 2 lines

If I recall correctly, this subterfuge suppresses compiler complaints about initializing an unsigned long with a negative number. :-/

Did you try:

	.srcu_gp_seq_needed = -1UL,

?

Quoted 8 lines

Quoted 2 lines

If I recall correctly, this subterfuge suppresses compiler complaints about initializing an unsigned long with a negative number. :-/

Did you try:

	.srcu_gp_seq_needed = -1UL,

?

Works for my compiler, not sure what set of complaints pushed me in that direction.

Quoted 7 lines · On Tue, 14 Aug 2018 10:44:43 -0700 "Paul E. McKenney"…

Quoted 7 lines

Quoted 2 lines

If I recall correctly, this subterfuge suppresses compiler complaints about initializing an unsigned long with a negative number. :-/

Did you try:

	.srcu_gp_seq_needed = -1UL,

Quoted 4 lines

Quoted 1 line

?

Works for my compiler, not sure what set of complaints pushed me in that direction.

I've used -1UL for unsigned long initializations for pretty much my entire programming career. I've never had any issues with it.

Quoted 29 lines · Steven Rostedt

Quoted 26 lines · On Tue, 14 Aug 2018 09:24:48 -0700 "Paul E. McKenney"…

Event tracing is moving to SRCU in order to take advantage of the fact that SRCU may be safely used from idle and even offline CPUs. However, event tracing can invoke call_srcu() very early in the boot process, even before workqueue_init_early() is invoked (let alone rcu_init()). Therefore, call_srcu()'s attempts to queue work fail miserably.

This commit therefore detects this situation, and refrains from attempting to queue work before rcu_init() time, but does everything else that it would have done, and in addition, adds the srcu_struct to a global list. The rcu_init() function now invokes a new srcu_init() function, which is empty if CONFIG_SRCU=n. Otherwise, srcu_init() queues work for each srcu_struct on the list. This all happens early enough in boot that there is but a single CPU with interrupts disabled, which allows synchronization to be dispensed with.

Of course, the queued work won't actually be invoked until after workqueue_init() is invoked, which happens shortly after the scheduler is up and running. This means that although call_srcu() may be invoked any time after per-CPU variables have been set up, there is still a very narrow window when synchronize_srcu() won't work, and this window extends from the time that the scheduler starts until the time that workqueue_init() returns. This can be fixed in a manner similar to the fix for synchronize_rcu_expedited() and friends, but until someone actually needs to use synchronize_srcu() during this window, this fix is added churn for no benefit.

Quoted 7 lines

Quoted 5 lines

Finally, note that Tree SRCU's new srcu_init() function invokes queue_work() rather than the queue_delayed_work() function that is invoked post-boot. The reason is that queue_delayed_work() will (as you would expect) post a timer, and timers have not yet been initialized. So use of queue_delayed_work() avoids the complaints about use of uninitialized

You mean "So use of queue_work() avoids .." ?

Indeed I do! Fixed.

Quoted 20 lines

Quoted 12 lines

spinlocks that would otherwise result. Besides, delay is in any case provide by the aforementioned fact that the queued work won't actually be invoked until after the scheduler is up and running.

Requested-by Steven Rostedt <[email protected]>

Signed-off-by Paul E. McKenney <[email protected]>

include/linux/srcutiny.h +0 -0

index f41d2fb09f87..2b5c0822e683 100644

include/linux/srcutiny.h struct srcu_struct { · line 39

Quoted 4 lines

include/linux/srcutiny.h +1 -0

@@ -36,6 +36,7 @@ struct srcu_struct {
 	struct rcu_head *srcu_cb_head;	/* Pending callbacks: Head. */
 	struct rcu_head **srcu_cb_tail;	/* Pending callbacks: Tail. */
 	struct work_struct srcu_work;	/* For driving grace periods. */
+	struct list_head srcu_boot_entry; /* Early-boot callbacks. */

I really don't like increasing the size of a structure for a field that is hardly ever used.

Quoted 5 lines

Is there a way we could make a union, or reuse one of the other fields, as we know that synchronize_srcu() can't be used yet (and if it is, either warn, or just make it a nop). And when we call srcu_init() and remove the srcu_struct from the list, we can then initialize whatever we used as the temporary boot up list field.

I will take a look. If nothing else, I could union it with the struct work_struct, since it cannot be used that early anyway. ;-)

Or I could just use the work_struct that is already inside the struct work_struct. Tejun, would you be OK with that?

For whatever it is worth, synchronize_srcu() is perfectly legal way early in boot, at least as early as call_srcu(). The reason is that until the scheduler starts, synchronize_srcu() is a no-op.

Quoted 1 line

srcu_init is called when we are still running only one CPU correct?

Yes, single CPU interrupts disabled.

Quoted 26 lines

Quoted 26 lines

include/linux/srcutiny.h +1 -0

 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 	struct lockdep_map dep_map;
 #endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */

@@ -48,6 +49,7 @@ void srcu_drive_gp(struct work_struct *wp);
 	.srcu_wq = __SWAIT_QUEUE_HEAD_INITIALIZER(name.srcu_wq),	\
 	.srcu_cb_tail = &name.srcu_cb_head,				\
 	.srcu_work = __WORK_INITIALIZER(name.srcu_work, srcu_drive_gp),	\
+	.srcu_boot_entry = LIST_HEAD_INIT(name.srcu_boot_entry),	\
 	__SRCU_DEP_MAP_INIT(name)					\
 }
 

include/linux/srcutree.h +1 -0

index 745d4ca4dd50..86ad97111315 100644
@@ -94,6 +94,7 @@ struct srcu_struct {
 						/*  callback for the barrier */
 						/*  operation. */
 	struct delayed_work work;
+	struct list_head srcu_boot_entry;	/* Early-boot callbacks. */
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 	struct lockdep_map dep_map;
 #endif /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */

@@ -105,12 +106,13 @@ struct srcu_struct {
 #define SRCU_STATE_SCAN2	2
 

Quoted 14 lines

include/linux/srcutree.h struct srcu_struct { · line 112

Quoted 11 lines

include/linux/srcutree.h +4 -6

 #define __SRCU_STRUCT_INIT(name, pcpu_name)				\
-	{								\
-		.sda = &pcpu_name,					\
-		.lock = __SPIN_LOCK_UNLOCKED(name.lock),		\
-		.srcu_gp_seq_needed = 0 - 1,				\
-		__SRCU_DEP_MAP_INIT(name)				\
-	}
+{									\
+	.sda = &pcpu_name,						\
+	.lock = __SPIN_LOCK_UNLOCKED(name.lock),			\
+	.srcu_gp_seq_needed = 0 - 1,					\

Interesting initialization of -1. This was there before, but still interesting none the less.

If I recall correctly, this subterfuge suppresses compiler complaints about initializing an unsigned long with a negative number. :-/

Thanx, Paul

Quoted 8 lines

include/linux/srcutree.h struct srcu_struct {

Quoted 5 lines

include/linux/srcutree.h +3 -0

+	.srcu_boot_entry = LIST_HEAD_INIT(name.srcu_boot_entry),	\
+	__SRCU_DEP_MAP_INIT(name)					\
+}
 
 

-- Steve

Quoted 5 lines

Is there a way we could make a union, or reuse one of the other fields, as we know that synchronize_srcu() can't be used yet (and if it is, either warn, or just make it a nop). And when we call srcu_init() and remove the srcu_struct from the list, we can then initialize whatever we used as the temporary boot up list field.

I will take a look. If nothing else, I could union it with the struct work_struct, since it cannot be used that early anyway. ;-)

Or I could just use the work_struct that is already inside the struct work_struct. Tejun, would you be OK with that?

Hello,

Quoted 6 lines · Paul E. McKenney

Quoted 5 lines

Is there a way we could make a union, or reuse one of the other fields, as we know that synchronize_srcu() can't be used yet (and if it is, either warn, or just make it a nop). And when we call srcu_init() and remove the srcu_struct from the list, we can then initialize whatever we used as the temporary boot up list field.

Quoted 5 lines

I will take a look. If nothing else, I could union it with the struct work_struct, since it cannot be used that early anyway. ;-)

Or I could just use the work_struct that is already inside the struct work_struct. Tejun, would you be OK with that?

Hmm... not super against it given how specialized the whole thing is but maybe just making it a union is cleaner?