Skip to contentSkip to conversation

FAILED: patch "[PATCH] tracepoint: Use rcu get state and cond sync for static call" failed to apply to 5.10-stable tree

PATCH [email protected] 4 participants 8 messages Aug 9, 2021 – Sep 1, 2021

Subsystems (2)
Stable branch Tracing
lore
Share & tools
lore

[email protected]

author maintainer
#0 lore raw

The patch below does not apply to the 5.10-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

thanks,

greg k-h

------------------ original commit in Linus's tree ------------------

From 7b40066c97ec66a44e388f82fcf694987451768f Mon Sep 17 00:00:00 2001
From: Mathieu Desnoyers <[email protected]>
Date: Thu, 5 Aug 2021 15:29:54 -0400
Subject: [PATCH] tracepoint: Use rcu get state and cond sync for static call
updates

State transitions from 1->0->1 and N->2->1 callbacks require RCU synchronization. Rather than performing the RCU synchronization every time the state change occurs, which is quite slow when many tracepoints are registered in batch, instead keep a snapshot of the RCU state on the most recent transitions which belong to a chain, and conditionally wait for a grace period on the last transition of the chain if one g.p. has not elapsed since the last snapshot.

This applies to both RCU and SRCU.

This brings the performance regression caused by commit 231264d6927f ("Fix: tracepoint: static call function vs data state mismatch") back to what it was originally.

Before this commit:

  # trace-cmd start -e all
  # time trace-cmd start -p nop

real 0m10.593s
user 0m0.017s
sys 0m0.259s

After this commit:

  # trace-cmd start -e all
  # time trace-cmd start -p nop

real 0m0.878s
user 0m0.000s
sys 0m0.103s

Link https://lkml.kernel.org/r/[email protected] open here
Link https://lore.kernel.org/io-uring/[email protected]/ open here
Cc [email protected]
Cc Ingo Molnar <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Andrew Morton <[email protected]>
Cc "Paul E. McKenney" <[email protected]>
Cc Stefan Metzmacher <[email protected]>
Fixes 231264d6927f ("Fix: tracepoint: static call function vs data state mismatch")
Signed-off-by Mathieu Desnoyers <[email protected]>
Reviewed-by Paul E. McKenney <[email protected]>
Signed-off-by Steven Rostedt (VMware) <[email protected]>
kernel/tracepoint.c +67 -14
index 8d772bd6894d..efd14c79fab4 100644
@@ -28,6 +28,44 @@ extern tracepoint_ptr_t __stop___tracepoints_ptrs[];
 DEFINE_SRCU(tracepoint_srcu);
 EXPORT_SYMBOL_GPL(tracepoint_srcu);
 
+enum tp_transition_sync {
+	TP_TRANSITION_SYNC_1_0_1,
+	TP_TRANSITION_SYNC_N_2_1,
+
+	_NR_TP_TRANSITION_SYNC,
+};
+
+struct tp_transition_snapshot {
+	unsigned long rcu;
+	unsigned long srcu;
+	bool ongoing;
+};
+
+/* Protected by tracepoints_mutex */
+static struct tp_transition_snapshot tp_transition_snapshot[_NR_TP_TRANSITION_SYNC];
+
+static void tp_rcu_get_state(enum tp_transition_sync sync)
+{
+	struct tp_transition_snapshot *snapshot = &tp_transition_snapshot[sync];
+
+	/* Keep the latest get_state snapshot. */
+	snapshot->rcu = get_state_synchronize_rcu();
+	snapshot->srcu = start_poll_synchronize_srcu(&tracepoint_srcu);
+	snapshot->ongoing = true;
+}
+
+static void tp_rcu_cond_sync(enum tp_transition_sync sync)
+{
+	struct tp_transition_snapshot *snapshot = &tp_transition_snapshot[sync];
+
+	if (!snapshot->ongoing)
+		return;
+	cond_synchronize_rcu(snapshot->rcu);
+	if (!poll_state_synchronize_srcu(&tracepoint_srcu, snapshot->srcu))
+		synchronize_srcu(&tracepoint_srcu);
+	snapshot->ongoing = false;
+}
+
 /* Set to 1 to enable tracepoint debug output */
 static const int tracepoint_debug;
 

@@ -311,6 +349,11 @@ static int tracepoint_add_func(struct tracepoint *tp,
 	 */
 	switch (nr_func_state(tp_funcs)) {
 	case TP_FUNC_1:		/* 0->1 */
+		/*
+		 * Make sure new static func never uses old data after a
+		 * 1->0->1 transition sequence.
+		 */
+		tp_rcu_cond_sync(TP_TRANSITION_SYNC_1_0_1);
 		/* Set static call to first function */
 		tracepoint_update_call(tp, tp_funcs);
 		/* Both iterator and static call handle NULL tp->funcs */

@@ -325,10 +368,15 @@ static int tracepoint_add_func(struct tracepoint *tp,
 		 * Requires ordering between RCU assign/dereference and
 		 * static call update/call.
 		 */
-		rcu_assign_pointer(tp->funcs, tp_funcs);
-		break;
+		fallthrough;
 	case TP_FUNC_N:		/* N->N+1 (N>1) */
 		rcu_assign_pointer(tp->funcs, tp_funcs);
+		/*
+		 * Make sure static func never uses incorrect data after a
+		 * N->...->2->1 (N>1) transition sequence.
+		 */
+		if (tp_funcs[0].data != old[0].data)
+			tp_rcu_get_state(TP_TRANSITION_SYNC_N_2_1);
 		break;
 	default:
 		WARN_ON_ONCE(1);

@@ -372,24 +420,23 @@ static int tracepoint_remove_func(struct tracepoint *tp,
 		/* Both iterator and static call handle NULL tp->funcs */
 		rcu_assign_pointer(tp->funcs, NULL);
 		/*
-		 * Make sure new func never uses old data after a 1->0->1
-		 * transition sequence.
-		 * Considering that transition 0->1 is the common case
-		 * and don't have rcu-sync, issue rcu-sync after
-		 * transition 1->0 to break that sequence by waiting for
-		 * readers to be quiescent.
+		 * Make sure new static func never uses old data after a
+		 * 1->0->1 transition sequence.
 		 */
-		tracepoint_synchronize_unregister();
+		tp_rcu_get_state(TP_TRANSITION_SYNC_1_0_1);
 		break;
 	case TP_FUNC_1:		/* 2->1 */
 		rcu_assign_pointer(tp->funcs, tp_funcs);
 		/*
-		 * On 2->1 transition, RCU sync is needed before setting
-		 * static call to first callback, because the observer
-		 * may have loaded any prior tp->funcs after the last one
-		 * associated with an rcu-sync.
+		 * Make sure static func never uses incorrect data after a
+		 * N->...->2->1 (N>2) transition sequence. If the first
+		 * element's data has changed, then force the synchronization
+		 * to prevent current readers that have loaded the old data
+		 * from calling the new function.
 		 */
-		tracepoint_synchronize_unregister();
+		if (tp_funcs[0].data != old[0].data)
+			tp_rcu_get_state(TP_TRANSITION_SYNC_N_2_1);
+		tp_rcu_cond_sync(TP_TRANSITION_SYNC_N_2_1);
 		/* Set static call to first function */
 		tracepoint_update_call(tp, tp_funcs);
 		break;

@@ -397,6 +444,12 @@ static int tracepoint_remove_func(struct tracepoint *tp,
 		fallthrough;
 	case TP_FUNC_N:
 		rcu_assign_pointer(tp->funcs, tp_funcs);
+		/*
+		 * Make sure static func never uses incorrect data after a
+		 * N->...->2->1 (N>2) transition sequence.
+		 */
+		if (tp_funcs[0].data != old[0].data)
+			tp_rcu_get_state(TP_TRANSITION_SYNC_N_2_1);
 		break;
 	default:
 		WARN_ON_ONCE(1);
maintainer
#1 lore raw
Quoted 5 lines · On Mon, 09 Aug 2021 11:20:32 +0200 <[email protected]> wrote:

The patch below does not apply to the 5.10-stable tree.
If someone wants it applied there, or to any other stable or longterm tree, then please email the backport, including the original git commit id to <[email protected]>.

Crap.

Mathieu, seems that the "slow down 10x" patch was able to be backported to 5.10, where as this patch was not. Reason being is that start_poll_synchronize_rcu() was added in 5.13.

This means because the other patch was able to be backported without this patch, we just slowed down disabling all events! Which may be a surprise to some people not expecting a stable branch from having such a big performance regression :-/

-- Steve

maintainer
#2 lore raw
Quoted 3 lines · On Thu, 19 Aug 2021 17:09:33 -0400 Steven Rostedt <[email protected]> wrote:

Mathieu, seems that the "slow down 10x" patch was able to be backported to 5.10, where as this patch was not. Reason being is that start_poll_synchronize_rcu() was added in 5.13.

I can get this to work if I backport the following RCU patches:

29d2bb94a8a126ce80ffbb433b648b32fdea524e
srcu: Provide internal interface to start a Tree SRCU grace period

5358c9fa54b09b5d3d7811b033aa0838c1bbaaf2
srcu: Provide polling interfaces for Tree SRCU grace periods

1a893c711a600ab57526619b56e6f6b7be00956e
srcu: Provide internal interface to start a Tiny SRCU grace period

8b5bd67cf6422b63ee100d76d8de8960ca2df7f0
srcu: Provide polling interfaces for Tiny SRCU grace periods

The first three can be cherry-picked without issue. The last one has a small conflict, of:

include/linux/srcutiny.h.rej:

linux/srcutiny.h +1 -0
@@ -16,6 +16,7 @@
 struct srcu_struct {
        short srcu_lock_nesting[2];     /* srcu_read_lock() nesting depth. */
        unsigned short srcu_idx;        /* Current reader array element in bit 0x2. */
+       unsigned short srcu_idx_max;    /* Furthest future srcu_idx request. */
        u8 srcu_gp_running;             /* GP workqueue running? */
        u8 srcu_gp_waiting;             /* GP waiting for readers? */
        struct swait_queue_head srcu_wq;

Which I just added that line, and everything worked.

Paul, do you have any issues with these four patches getting backported?

Greg, Are you OK with them too?

Once those are backported, this patch can be backported as well, and everything should work. This patch really needs to stay with:

231264d6927f6740af36855a622d0e240be9d94c
tracepoint: Fix static call function vs data state mismatch

Otherwise I would say to revert it if this one can't be backported with it.

-- Steve

maintainer
#3 lore raw

Greg, Paul,

Any comment on this?

-- Steve

Quoted 53 lines · On Thu, 19 Aug 2021 20:42:04 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 3 lines · On Thu, 19 Aug 2021 17:09:33 -0400 Steven Rostedt <[email protected]> wrote:

Mathieu, seems that the "slow down 10x" patch was able to be backported to 5.10, where as this patch was not. Reason being is that start_poll_synchronize_rcu() was added in 5.13.

I can get this to work if I backport the following RCU patches:

29d2bb94a8a126ce80ffbb433b648b32fdea524e
srcu: Provide internal interface to start a Tree SRCU grace period

5358c9fa54b09b5d3d7811b033aa0838c1bbaaf2
srcu: Provide polling interfaces for Tree SRCU grace periods

1a893c711a600ab57526619b56e6f6b7be00956e
srcu: Provide internal interface to start a Tiny SRCU grace period

8b5bd67cf6422b63ee100d76d8de8960ca2df7f0
srcu: Provide polling interfaces for Tiny SRCU grace periods

The first three can be cherry-picked without issue. The last one has a small conflict, of:

include/linux/srcutiny.h.rej:

linux/srcutiny.h +1 -0
@@ -16,6 +16,7 @@
 struct srcu_struct {
        short srcu_lock_nesting[2];     /* srcu_read_lock() nesting depth. */
        unsigned short srcu_idx;        /* Current reader array element in bit 0x2. */
+       unsigned short srcu_idx_max;    /* Furthest future srcu_idx request. */
        u8 srcu_gp_running;             /* GP workqueue running? */
        u8 srcu_gp_waiting;             /* GP waiting for readers? */
        struct swait_queue_head srcu_wq;

Which I just added that line, and everything worked.

Paul, do you have any issues with these four patches getting backported?

Greg, Are you OK with them too?

Once those are backported, this patch can be backported as well, and everything should work. This patch really needs to stay with:

231264d6927f6740af36855a622d0e240be9d94c
tracepoint: Fix static call function vs data state mismatch

Otherwise I would say to revert it if this one can't be backported with it.

-- Steve

reviewer
#4 lore raw
In reply to Steven Rostedt
Quoted 43 lines · ----- On Aug 19, 2021, at 8:42 PM, rostedt [email protected] wrote:
Quoted 3 lines · On Thu, 19 Aug 2021 17:09:33 -0400 Steven Rostedt <[email protected]> wrote:

Mathieu, seems that the "slow down 10x" patch was able to be backported to 5.10, where as this patch was not. Reason being is that start_poll_synchronize_rcu() was added in 5.13.

I can get this to work if I backport the following RCU patches:

29d2bb94a8a126ce80ffbb433b648b32fdea524e
srcu: Provide internal interface to start a Tree SRCU grace period

5358c9fa54b09b5d3d7811b033aa0838c1bbaaf2
srcu: Provide polling interfaces for Tree SRCU grace periods

1a893c711a600ab57526619b56e6f6b7be00956e
srcu: Provide internal interface to start a Tiny SRCU grace period

8b5bd67cf6422b63ee100d76d8de8960ca2df7f0
srcu: Provide polling interfaces for Tiny SRCU grace periods

The first three can be cherry-picked without issue. The last one has a small conflict, of:

include/linux/srcutiny.h.rej:

linux/srcutiny.h +1 -0
@@ -16,6 +16,7 @@
 struct srcu_struct {
       short srcu_lock_nesting[2];     /* srcu_read_lock() nesting depth. */
       unsigned short srcu_idx;        /* Current reader array element in bit 0x2. */
+       unsigned short srcu_idx_max;    /* Furthest future srcu_idx request. */
       u8 srcu_gp_running;             /* GP workqueue running? */
       u8 srcu_gp_waiting;             /* GP waiting for readers? */
       struct swait_queue_head srcu_wq;

Which I just added that line, and everything worked.

Paul, do you have any issues with these four patches getting backported?

Greg, Are you OK with them too?

Quoted 8 lines

Once those are backported, this patch can be backported as well, and everything should work. This patch really needs to stay with:

231264d6927f6740af36855a622d0e240be9d94c
tracepoint: Fix static call function vs data state mismatch

Otherwise I would say to revert it if this one can't be backported with it.

In my opinion backporting those patches to stable is important, because the tracepoint fix which causes the slowdown is really fixing a correctness issue: it ensures that the kernel does not crash in specific race scenarios.

Indeed the slowdown associated with that patch is quite big on typical use-cases of tracepoint registration/unregistration, so the second fixing the speed regression is also important, and that last fix requires the SRCU backports.

Thanks,

Mathieu

Signature · 3 lines
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
#5 lore raw
In reply to Steven Rostedt
Quoted 37 lines · Steven Rostedt
Quoted 3 lines · On Thu, 19 Aug 2021 17:09:33 -0400 Steven Rostedt <[email protected]> wrote:

Mathieu, seems that the "slow down 10x" patch was able to be backported to 5.10, where as this patch was not. Reason being is that start_poll_synchronize_rcu() was added in 5.13.

I can get this to work if I backport the following RCU patches:

29d2bb94a8a126ce80ffbb433b648b32fdea524e
srcu: Provide internal interface to start a Tree SRCU grace period

5358c9fa54b09b5d3d7811b033aa0838c1bbaaf2
srcu: Provide polling interfaces for Tree SRCU grace periods

1a893c711a600ab57526619b56e6f6b7be00956e
srcu: Provide internal interface to start a Tiny SRCU grace period

8b5bd67cf6422b63ee100d76d8de8960ca2df7f0
srcu: Provide polling interfaces for Tiny SRCU grace periods

The first three can be cherry-picked without issue. The last one has a small conflict, of:

include/linux/srcutiny.h.rej:

linux/srcutiny.h +1 -0
@@ -16,6 +16,7 @@
 struct srcu_struct {
        short srcu_lock_nesting[2];     /* srcu_read_lock() nesting depth. */
        unsigned short srcu_idx;        /* Current reader array element in bit 0x2. */
+       unsigned short srcu_idx_max;    /* Furthest future srcu_idx request. */
        u8 srcu_gp_running;             /* GP workqueue running? */
        u8 srcu_gp_waiting;             /* GP waiting for readers? */
        struct swait_queue_head srcu_wq;
Quoted 3 lines

Which I just added that line, and everything worked.

Paul, do you have any issues with these four patches getting backported?

I believe that you also need to backport 74612a07b83f ("srcu: Make Tiny SRCU use multi-bit grace-period counter"). Otherwise, Tiny SRCU polling grace periods will be at best working by accident.

This will also make your small conflict go away.

Thanx, Paul

Quoted 12 lines

Greg, Are you OK with them too?

Once those are backported, this patch can be backported as well, and everything should work. This patch really needs to stay with:

231264d6927f6740af36855a622d0e240be9d94c
tracepoint: Fix static call function vs data state mismatch

Otherwise I would say to revert it if this one can't be backported with it.

-- Steve

maintainer
#6 lore raw
Quoted 7 lines · On Fri, 20 Aug 2021 10:57:38 -0700 "Paul E. McKenney" <[email protected]>…
Quoted 1 line

Paul, do you have any issues with these four patches getting backported?

I believe that you also need to backport 74612a07b83f ("srcu: Make Tiny SRCU use multi-bit grace-period counter"). Otherwise, Tiny SRCU polling grace periods will be at best working by accident.

This will also make your small conflict go away.

Thanks for looking at this Paul. Yes, that commit helps where I don't have to do any fixes.

Greg,

Can you please backport the following commits to 5.10, and then reapply this patch?

29d2bb94a8a1 ("srcu: Provide internal interface to start a Tree SRCU grace period") 5358c9fa54b0 ("srcu: Provide polling interfaces for Tree SRCU grace periods") 1a893c711a60 ("srcu: Provide internal interface to start a Tiny SRCU grace period") 74612a07b83f ("srcu: Make Tiny SRCU use multi-bit grace-period counter") 8b5bd67cf642 ("srcu: Provide polling interfaces for Tiny SRCU grace periods")

Thanks,

-- Steve

author maintainer
#7 lore raw
Quoted 16 lines · Steven Rostedt
Quoted 7 lines · On Fri, 20 Aug 2021 10:57:38 -0700 "Paul E. McKenney" <[email protected]>…
Quoted 1 line

Paul, do you have any issues with these four patches getting backported?

I believe that you also need to backport 74612a07b83f ("srcu: Make Tiny SRCU use multi-bit grace-period counter"). Otherwise, Tiny SRCU polling grace periods will be at best working by accident.

This will also make your small conflict go away.

Thanks for looking at this Paul. Yes, that commit helps where I don't have to do any fixes.

Greg,

Quoted 8 lines

Can you please backport the following commits to 5.10, and then reapply this patch?

29d2bb94a8a1 ("srcu: Provide internal interface to start a Tree SRCU grace period") 5358c9fa54b0 ("srcu: Provide polling interfaces for Tree SRCU grace periods") 1a893c711a60 ("srcu: Provide internal interface to start a Tiny SRCU grace period") 74612a07b83f ("srcu: Make Tiny SRCU use multi-bit grace-period counter") 8b5bd67cf642 ("srcu: Provide polling interfaces for Tiny SRCU grace periods")

Now done, thanks.

greg k-h