[PATCH rcu 0/5] Tasks RCU updates for v6.6

[PATCH rcu 0/5] Tasks RCU updates for v6.6

[Paul E. McKenney]

Hello!

This series contains Tasks RCU updates.

1.	rcu-tasks: Treat only synchronous grace periods urgently.

2.	rcu-tasks: Remove redundant #ifdef CONFIG_TASKS_RCU.

3.	rcu-tasks: Add kernel boot parameters for callback laziness.

4.	rcu-tasks: Cancel callback laziness if too many callbacks.

5.	Complain about unexpected uses of RCU Tasks Trace.

						Thanx, Paul

------------------------------------------------------------------------

 Documentation/admin-guide/kernel-parameters.txt   |    7 +
 b/Documentation/admin-guide/kernel-parameters.txt |   23 ++++++
 b/kernel/rcu/tasks.h                              |   81 +++++++++++++++++++---
 b/scripts/checkpatch.pl                           |   18 ++++
 kernel/rcu/tasks.h                                |   24 +++++-
 5 files changed, 141 insertions(+), 12 deletions(-)

[PATCH rcu 1/5] rcu-tasks: Treat only synchronous grace periods urgently

[Paul E. McKenney]

The performance requirements on RCU Tasks, and in particular on RCU
Tasks Trace, have evolved over time as the workloads have evolved.
The current implementation is designed to provide low grace-period
latencies, and also to accommodate short-duration floods of callbacks.

However, current workloads can also provide a constant background
callback-queuing rate of a few hundred call_rcu_tasks_trace() invocations
per second.  This results in continuous back-to-back RCU Tasks Trace
grace periods, which in turn can consume the better part of 10% of a CPU.
One could take the attitude that there are several tens of other CPUs on
the systems running such workloads, but energy efficiency is a thing.
On these systems, although asynchronous grace-period requests happen
every few milliseconds, synchronous grace-period requests are quite rare.

This commit therefore arrranges for grace periods to be initiated
immediately in response to calls to synchronize_rcu_tasks*() and
also to calls to synchronize_rcu_mult() that are passed one of the
call_rcu_tasks*() functions.  These are recognized by the tell-tale
wakeme_after_rcu callback function.

In other cases, callbacks are gathered up for up to about 250 milliseconds
before a grace period is initiated.  This results in more than an order of
magnitude reduction in RCU Tasks Trace grace periods, with corresponding
reduction in consumption of CPU time.

Reported-by: Alexei Starovoitov <ast@kernel.org>
Reported-by: Martin KaFai Lau <kafai@fb.com>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/rcu/tasks.h | 81 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 73 insertions(+), 8 deletions(-)

diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index b770add3f843..c88d7fe29d92 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -25,6 +25,8 @@ typedef void (*postgp_func_t)(struct rcu_tasks *rtp);
  * @cblist: Callback list.
  * @lock: Lock protecting per-CPU callback list.
  * @rtp_jiffies: Jiffies counter value for statistics.
+ * @lazy_timer: Timer to unlazify callbacks.
+ * @urgent_gp: Number of additional non-lazy grace periods.
  * @rtp_n_lock_retries: Rough lock-contention statistic.
  * @rtp_work: Work queue for invoking callbacks.
  * @rtp_irq_work: IRQ work queue for deferred wakeups.
@@ -38,6 +40,8 @@ struct rcu_tasks_percpu {
 	raw_spinlock_t __private lock;
 	unsigned long rtp_jiffies;
 	unsigned long rtp_n_lock_retries;
+	struct timer_list lazy_timer;
+	unsigned int urgent_gp;
 	struct work_struct rtp_work;
 	struct irq_work rtp_irq_work;
 	struct rcu_head barrier_q_head;
@@ -51,7 +55,6 @@ struct rcu_tasks_percpu {
  * @cbs_wait: RCU wait allowing a new callback to get kthread's attention.
  * @cbs_gbl_lock: Lock protecting callback list.
  * @tasks_gp_mutex: Mutex protecting grace period, needed during mid-boot dead zone.
- * @kthread_ptr: This flavor's grace-period/callback-invocation kthread.
  * @gp_func: This flavor's grace-period-wait function.
  * @gp_state: Grace period's most recent state transition (debugging).
  * @gp_sleep: Per-grace-period sleep to prevent CPU-bound looping.
@@ -61,6 +64,8 @@ struct rcu_tasks_percpu {
  * @tasks_gp_seq: Number of grace periods completed since boot.
  * @n_ipis: Number of IPIs sent to encourage grace periods to end.
  * @n_ipis_fails: Number of IPI-send failures.
+ * @kthread_ptr: This flavor's grace-period/callback-invocation kthread.
+ * @lazy_jiffies: Number of jiffies to allow callbacks to be lazy.
  * @pregp_func: This flavor's pre-grace-period function (optional).
  * @pertask_func: This flavor's per-task scan function (optional).
  * @postscan_func: This flavor's post-task scan function (optional).
@@ -92,6 +97,7 @@ struct rcu_tasks {
 	unsigned long n_ipis;
 	unsigned long n_ipis_fails;
 	struct task_struct *kthread_ptr;
+	unsigned long lazy_jiffies;
 	rcu_tasks_gp_func_t gp_func;
 	pregp_func_t pregp_func;
 	pertask_func_t pertask_func;
@@ -127,6 +133,7 @@ static struct rcu_tasks rt_name =							\
 	.gp_func = gp,									\
 	.call_func = call,								\
 	.rtpcpu = &rt_name ## __percpu,							\
+	.lazy_jiffies = DIV_ROUND_UP(HZ, 4),						\
 	.name = n,									\
 	.percpu_enqueue_shift = order_base_2(CONFIG_NR_CPUS),				\
 	.percpu_enqueue_lim = 1,							\
@@ -276,6 +283,33 @@ static void cblist_init_generic(struct rcu_tasks *rtp)
 			data_race(rtp->percpu_enqueue_shift), data_race(rtp->percpu_enqueue_lim), rcu_task_cb_adjust);
 }
 
+// Compute wakeup time for lazy callback timer.
+static unsigned long rcu_tasks_lazy_time(struct rcu_tasks *rtp)
+{
+	return jiffies + rtp->lazy_jiffies;
+}
+
+// Timer handler that unlazifies lazy callbacks.
+static void call_rcu_tasks_generic_timer(struct timer_list *tlp)
+{
+	unsigned long flags;
+	bool needwake = false;
+	struct rcu_tasks *rtp;
+	struct rcu_tasks_percpu *rtpcp = from_timer(rtpcp, tlp, lazy_timer);
+
+	rtp = rtpcp->rtpp;
+	raw_spin_lock_irqsave_rcu_node(rtpcp, flags);
+	if (!rcu_segcblist_empty(&rtpcp->cblist) && rtp->lazy_jiffies) {
+		if (!rtpcp->urgent_gp)
+			rtpcp->urgent_gp = 1;
+		needwake = true;
+		mod_timer(&rtpcp->lazy_timer, rcu_tasks_lazy_time(rtp));
+	}
+	raw_spin_unlock_irqrestore_rcu_node(rtpcp, flags);
+	if (needwake)
+		rcuwait_wake_up(&rtp->cbs_wait);
+}
+
 // IRQ-work handler that does deferred wakeup for call_rcu_tasks_generic().
 static void call_rcu_tasks_iw_wakeup(struct irq_work *iwp)
 {
@@ -292,6 +326,7 @@ static void call_rcu_tasks_generic(struct rcu_head *rhp, rcu_callback_t func,
 {
 	int chosen_cpu;
 	unsigned long flags;
+	bool havekthread = smp_load_acquire(&rtp->kthread_ptr);
 	int ideal_cpu;
 	unsigned long j;
 	bool needadjust = false;
@@ -321,7 +356,15 @@ static void call_rcu_tasks_generic(struct rcu_head *rhp, rcu_callback_t func,
 		cblist_init_generic(rtp);
 		raw_spin_lock_rcu_node(rtpcp); // irqs already disabled.
 	}
-	needwake = rcu_segcblist_empty(&rtpcp->cblist);
+	needwake = func == wakeme_after_rcu;
+	if (havekthread && !timer_pending(&rtpcp->lazy_timer)) {
+		if (rtp->lazy_jiffies)
+			mod_timer(&rtpcp->lazy_timer, rcu_tasks_lazy_time(rtp));
+		else
+			needwake = rcu_segcblist_empty(&rtpcp->cblist);
+	}
+	if (needwake)
+		rtpcp->urgent_gp = 3;
 	rcu_segcblist_enqueue(&rtpcp->cblist, rhp);
 	raw_spin_unlock_irqrestore_rcu_node(rtpcp, flags);
 	if (unlikely(needadjust)) {
@@ -415,9 +458,14 @@ static int rcu_tasks_need_gpcb(struct rcu_tasks *rtp)
 		}
 		rcu_segcblist_advance(&rtpcp->cblist, rcu_seq_current(&rtp->tasks_gp_seq));
 		(void)rcu_segcblist_accelerate(&rtpcp->cblist, rcu_seq_snap(&rtp->tasks_gp_seq));
-		if (rcu_segcblist_pend_cbs(&rtpcp->cblist))
+		if (rtpcp->urgent_gp > 0 && rcu_segcblist_pend_cbs(&rtpcp->cblist)) {
+			if (rtp->lazy_jiffies)
+				rtpcp->urgent_gp--;
 			needgpcb |= 0x3;
-		if (!rcu_segcblist_empty(&rtpcp->cblist))
+		} else if (rcu_segcblist_empty(&rtpcp->cblist)) {
+			rtpcp->urgent_gp = 0;
+		}
+		if (rcu_segcblist_ready_cbs(&rtpcp->cblist))
 			needgpcb |= 0x1;
 		raw_spin_unlock_irqrestore_rcu_node(rtpcp, flags);
 	}
@@ -549,11 +597,19 @@ static void rcu_tasks_one_gp(struct rcu_tasks *rtp, bool midboot)
 // RCU-tasks kthread that detects grace periods and invokes callbacks.
 static int __noreturn rcu_tasks_kthread(void *arg)
 {
+	int cpu;
 	struct rcu_tasks *rtp = arg;
 
+	for_each_possible_cpu(cpu) {
+		struct rcu_tasks_percpu *rtpcp = per_cpu_ptr(rtp->rtpcpu, cpu);
+
+		timer_setup(&rtpcp->lazy_timer, call_rcu_tasks_generic_timer, 0);
+		rtpcp->urgent_gp = 1;
+	}
+
 	/* Run on housekeeping CPUs by default.  Sysadm can move if desired. */
 	housekeeping_affine(current, HK_TYPE_RCU);
-	WRITE_ONCE(rtp->kthread_ptr, current); // Let GPs start!
+	smp_store_release(&rtp->kthread_ptr, current); // Let GPs start!
 
 	/*
 	 * Each pass through the following loop makes one check for
@@ -635,16 +691,22 @@ static void show_rcu_tasks_generic_gp_kthread(struct rcu_tasks *rtp, char *s)
 {
 	int cpu;
 	bool havecbs = false;
+	bool haveurgent = false;
+	bool haveurgentcbs = false;
 
 	for_each_possible_cpu(cpu) {
 		struct rcu_tasks_percpu *rtpcp = per_cpu_ptr(rtp->rtpcpu, cpu);
 
-		if (!data_race(rcu_segcblist_empty(&rtpcp->cblist))) {
+		if (!data_race(rcu_segcblist_empty(&rtpcp->cblist)))
 			havecbs = true;
+		if (data_race(rtpcp->urgent_gp))
+			haveurgent = true;
+		if (!data_race(rcu_segcblist_empty(&rtpcp->cblist)) && data_race(rtpcp->urgent_gp))
+			haveurgentcbs = true;
+		if (havecbs && haveurgent && haveurgentcbs)
 			break;
-		}
 	}
-	pr_info("%s: %s(%d) since %lu g:%lu i:%lu/%lu %c%c %s\n",
+	pr_info("%s: %s(%d) since %lu g:%lu i:%lu/%lu %c%c%c%c l:%lu %s\n",
 		rtp->kname,
 		tasks_gp_state_getname(rtp), data_race(rtp->gp_state),
 		jiffies - data_race(rtp->gp_jiffies),
@@ -652,6 +714,9 @@ static void show_rcu_tasks_generic_gp_kthread(struct rcu_tasks *rtp, char *s)
 		data_race(rtp->n_ipis_fails), data_race(rtp->n_ipis),
 		".k"[!!data_race(rtp->kthread_ptr)],
 		".C"[havecbs],
+		".u"[haveurgent],
+		".U"[haveurgentcbs],
+		rtp->lazy_jiffies,
 		s);
 }
 #endif // #ifndef CONFIG_TINY_RCU
-- 
2.40.1

[PATCH rcu 2/5] rcu-tasks: Remove redundant #ifdef CONFIG_TASKS_RCU

[Paul E. McKenney]

The kernel/rcu/tasks.h file has a #endif immediately followed by an

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 kernel/rcu/tasks.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index c88d7fe29d92..7eae67fbe47c 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -146,9 +146,7 @@ static struct rcu_tasks rt_name =							\
 #ifdef CONFIG_TASKS_RCU
 /* Track exiting tasks in order to allow them to be waited for. */
 DEFINE_STATIC_SRCU(tasks_rcu_exit_srcu);
-#endif
 
-#ifdef CONFIG_TASKS_RCU
 /* Report delay in synchronize_srcu() completion in rcu_tasks_postscan(). */
 static void tasks_rcu_exit_srcu_stall(struct timer_list *unused);
 static DEFINE_TIMER(tasks_rcu_exit_srcu_stall_timer, tasks_rcu_exit_srcu_stall);
-- 
2.40.1

[PATCH rcu 3/5] rcu-tasks: Add kernel boot parameters for callback laziness

[Paul E. McKenney]

This commit adds kernel boot parameters for callback laziness, allowing
the RCU Tasks flavors to be individually adjusted.

Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 .../admin-guide/kernel-parameters.txt         | 23 +++++++++++++++++++
 kernel/rcu/tasks.h                            | 15 ++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index a1457995fd41..a0f427e1a562 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5293,6 +5293,29 @@
 			A change in value does not take effect until
 			the beginning of the next grace period.
 
+	rcupdate.rcu_tasks_lazy_ms= [KNL]
+			Set timeout in milliseconds RCU Tasks asynchronous
+			callback batching for call_rcu_tasks().
+			A negative value will take the default.  A value
+			of zero will disable batching.	Batching is
+			always disabled for synchronize_rcu_tasks().
+
+	rcupdate.rcu_tasks_rude_lazy_ms= [KNL]
+			Set timeout in milliseconds RCU Tasks
+			Rude asynchronous callback batching for
+			call_rcu_tasks_rude().	A negative value
+			will take the default.	A value of zero will
+			disable batching.  Batching is always disabled
+			for synchronize_rcu_tasks_rude().
+
+	rcupdate.rcu_tasks_trace_lazy_ms= [KNL]
+			Set timeout in milliseconds RCU Tasks
+			Trace asynchronous callback batching for
+			call_rcu_tasks_trace().  A negative value
+			will take the default.	A value of zero will
+			disable batching.  Batching is always disabled
+			for synchronize_rcu_tasks_trace().
+
 	rcupdate.rcu_self_test= [KNL]
 			Run the RCU early boot self tests
 
diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index 7eae67fbe47c..28e986627e3f 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -1083,11 +1083,16 @@ void rcu_barrier_tasks(void)
 }
 EXPORT_SYMBOL_GPL(rcu_barrier_tasks);
 
+int rcu_tasks_lazy_ms = -1;
+module_param(rcu_tasks_lazy_ms, int, 0444);
+
 static int __init rcu_spawn_tasks_kthread(void)
 {
 	cblist_init_generic(&rcu_tasks);
 	rcu_tasks.gp_sleep = HZ / 10;
 	rcu_tasks.init_fract = HZ / 10;
+	if (rcu_tasks_lazy_ms >= 0)
+		rcu_tasks.lazy_jiffies = msecs_to_jiffies(rcu_tasks_lazy_ms);
 	rcu_tasks.pregp_func = rcu_tasks_pregp_step;
 	rcu_tasks.pertask_func = rcu_tasks_pertask;
 	rcu_tasks.postscan_func = rcu_tasks_postscan;
@@ -1236,10 +1241,15 @@ void rcu_barrier_tasks_rude(void)
 }
 EXPORT_SYMBOL_GPL(rcu_barrier_tasks_rude);
 
+int rcu_tasks_rude_lazy_ms = -1;
+module_param(rcu_tasks_rude_lazy_ms, int, 0444);
+
 static int __init rcu_spawn_tasks_rude_kthread(void)
 {
 	cblist_init_generic(&rcu_tasks_rude);
 	rcu_tasks_rude.gp_sleep = HZ / 10;
+	if (rcu_tasks_rude_lazy_ms >= 0)
+		rcu_tasks_rude.lazy_jiffies = msecs_to_jiffies(rcu_tasks_rude_lazy_ms);
 	rcu_spawn_tasks_kthread_generic(&rcu_tasks_rude);
 	return 0;
 }
@@ -1856,6 +1866,9 @@ void rcu_barrier_tasks_trace(void)
 }
 EXPORT_SYMBOL_GPL(rcu_barrier_tasks_trace);
 
+int rcu_tasks_trace_lazy_ms = -1;
+module_param(rcu_tasks_trace_lazy_ms, int, 0444);
+
 static int __init rcu_spawn_tasks_trace_kthread(void)
 {
 	cblist_init_generic(&rcu_tasks_trace);
@@ -1870,6 +1883,8 @@ static int __init rcu_spawn_tasks_trace_kthread(void)
 		if (rcu_tasks_trace.init_fract <= 0)
 			rcu_tasks_trace.init_fract = 1;
 	}
+	if (rcu_tasks_trace_lazy_ms >= 0)
+		rcu_tasks_trace.lazy_jiffies = msecs_to_jiffies(rcu_tasks_trace_lazy_ms);
 	rcu_tasks_trace.pregp_func = rcu_tasks_trace_pregp_step;
 	rcu_tasks_trace.postscan_func = rcu_tasks_trace_postscan;
 	rcu_tasks_trace.holdouts_func = check_all_holdout_tasks_trace;
-- 
2.40.1

[PATCH rcu 4/5] rcu-tasks: Cancel callback laziness if too many callbacks

[Paul E. McKenney]

The various RCU Tasks flavors now do lazy grace periods when there are
only asynchronous grace period requests.  By default, the system will let
250 milliseconds elapse after the first call_rcu_tasks*() callbacki is
queued before starting a grace period.  In contrast, synchronous grace
period requests such as synchronize_rcu_tasks*() will start a grace
period immediately.

However, invoking one of the call_rcu_tasks*() functions in a too-tight
loop can result in a callback flood, which in turn can exhaust memory
if grace periods are delayed for too long.

This commit therefore sets a limit so that the grace-period kthread
will be awakened when any CPU's callback list expands to contain
rcupdate.rcu_task_lazy_lim callbacks elements (defaulting to 32, set to -1
to disable), the grace-period kthread will be awakened, thus cancelling
any ongoing laziness and getting out in front of the potential callback
flood.

Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Martin KaFai Lau <kafai@fb.com>
Cc: Andrii Nakryiko <andrii@kernel.org>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 Documentation/admin-guide/kernel-parameters.txt | 7 +++++++
 kernel/rcu/tasks.h                              | 7 +++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index a0f427e1a562..44705a3618b9 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -5264,6 +5264,13 @@
 			number avoids disturbing real-time workloads,
 			but lengthens grace periods.
 
+	rcupdate.rcu_task_lazy_lim= [KNL]
+			Number of callbacks on a given CPU that will
+			cancel laziness on that CPU.  Use -1 to disable
+			cancellation of laziness, but be advised that
+			doing so increases the danger of OOM due to
+			callback flooding.
+
 	rcupdate.rcu_task_stall_info= [KNL]
 			Set initial timeout in jiffies for RCU task stall
 			informational messages, which give some indication
diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index 28e986627e3f..291d536b1789 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -176,6 +176,8 @@ static int rcu_task_contend_lim __read_mostly = 100;
 module_param(rcu_task_contend_lim, int, 0444);
 static int rcu_task_collapse_lim __read_mostly = 10;
 module_param(rcu_task_collapse_lim, int, 0444);
+static int rcu_task_lazy_lim __read_mostly = 32;
+module_param(rcu_task_lazy_lim, int, 0444);
 
 /* RCU tasks grace-period state for debugging. */
 #define RTGS_INIT		 0
@@ -354,8 +356,9 @@ static void call_rcu_tasks_generic(struct rcu_head *rhp, rcu_callback_t func,
 		cblist_init_generic(rtp);
 		raw_spin_lock_rcu_node(rtpcp); // irqs already disabled.
 	}
-	needwake = func == wakeme_after_rcu;
-	if (havekthread && !timer_pending(&rtpcp->lazy_timer)) {
+	needwake = (func == wakeme_after_rcu) ||
+		   (rcu_segcblist_n_cbs(&rtpcp->cblist) == rcu_task_lazy_lim);
+	if (havekthread && !needwake && !timer_pending(&rtpcp->lazy_timer)) {
 		if (rtp->lazy_jiffies)
 			mod_timer(&rtpcp->lazy_timer, rcu_tasks_lazy_time(rtp));
 		else
-- 
2.40.1

[PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace

[Paul E. McKenney]

RCU Tasks Trace is quite specialized, having been created specifically
for sleepable BPF programs.  Because it allows general blocking within
readers, any new use of RCU Tasks Trace must take current use cases into
account.  Therefore, update checkpatch.pl to complain about use of any of
the RCU Tasks Trace API members outside of BPF and outside of RCU itself.

Cc: Andy Whitcroft <apw@canonical.com> (maintainer:CHECKPATCH)
Cc: Joe Perches <joe@perches.com> (maintainer:CHECKPATCH)
Cc: Dwaipayan Ray <dwaipayanray1@gmail.com> (reviewer:CHECKPATCH)
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Daniel Borkmann <daniel@iogearbox.net>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: <bpf@vger.kernel.org>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
---
 scripts/checkpatch.pl | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 880fde13d9b8..24bab980bc6f 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7457,6 +7457,24 @@ sub process {
 			}
 		}
 
+# Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
+		if ($line =~ /\brcu_read_lock_trace\s*\(/ ||
+		    $line =~ /\brcu_read_lock_trace_held\s*\(/ ||
+		    $line =~ /\brcu_read_unlock_trace\s*\(/ ||
+		    $line =~ /\bcall_rcu_tasks_trace\s*\(/ ||
+		    $line =~ /\bsynchronize_rcu_tasks_trace\s*\(/ ||
+		    $line =~ /\brcu_barrier_tasks_trace\s*\(/ ||
+		    $line =~ /\brcu_request_urgent_qs_task\s*\(/) {
+			if ($realfile !~ m@^kernel/bpf@ &&
+			    $realfile !~ m@^include/linux/bpf@ &&
+			    $realfile !~ m@^net/bpf@ &&
+			    $realfile !~ m@^kernel/rcu@ &&
+			    $realfile !~ m@^include/linux/rcu@) {
+				WARN("RCU_TASKS_TRACE",
+				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
+			}
+		}
+
 # check for lockdep_set_novalidate_class
 		if ($line =~ /^.\s*lockdep_set_novalidate_class\s*\(/ ||
 		    $line =~ /__lockdep_no_validate__\s*\)/ ) {
-- 
2.40.1

Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace

[Joe Perches]

On Mon, 2023-07-17 at 11:04 -0700, Paul E. McKenney wrote:
> RCU Tasks Trace is quite specialized, having been created specifically
> for sleepable BPF programs.  Because it allows general blocking within
> readers, any new use of RCU Tasks Trace must take current use cases into
> account.  Therefore, update checkpatch.pl to complain about use of any of
> the RCU Tasks Trace API members outside of BPF and outside of RCU itself.
> 
> Cc: Andy Whitcroft <apw@canonical.com> (maintainer:CHECKPATCH)
> Cc: Joe Perches <joe@perches.com> (maintainer:CHECKPATCH)
> Cc: Dwaipayan Ray <dwaipayanray1@gmail.com> (reviewer:CHECKPATCH)
> Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: <bpf@vger.kernel.org>
> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> ---
>  scripts/checkpatch.pl | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -7457,6 +7457,24 @@ sub process {
>  			}
>  		}
>  
> +# Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> +		if ($line =~ /\brcu_read_lock_trace\s*\(/ ||
> +		    $line =~ /\brcu_read_lock_trace_held\s*\(/ ||
> +		    $line =~ /\brcu_read_unlock_trace\s*\(/ ||
> +		    $line =~ /\bcall_rcu_tasks_trace\s*\(/ ||
> +		    $line =~ /\bsynchronize_rcu_tasks_trace\s*\(/ ||
> +		    $line =~ /\brcu_barrier_tasks_trace\s*\(/ ||
> +		    $line =~ /\brcu_request_urgent_qs_task\s*\(/) {
> +			if ($realfile !~ m@^kernel/bpf@ &&
> +			    $realfile !~ m@^include/linux/bpf@ &&
> +			    $realfile !~ m@^net/bpf@ &&
> +			    $realfile !~ m@^kernel/rcu@ &&
> +			    $realfile !~ m@^include/linux/rcu@) {

Functions and paths like these tend to be accreted.

Please use a variable or 2 like:

our $rcu_trace_funcs = qr{(?x:
	rcu_read_lock_trace |
	rcu_read_lock_trace_held |
	rcu_read_unlock_trace |
	call_rcu_tasks_trace |
	synchronize_rcu_tasks_trace |
	rcu_barrier_tasks_trace |
	rcu_request_urgent_qs_task
)};
our $rcu_trace_paths = qr{(?x:
	kernel/bfp/ |
	include/linux/bpf |
	net/bpf/ |
	kernel/rcu/ |
	include/linux/rcu
)};

...
	
		if ($line =~ /\b($rcu_trace_funcs)\s*\(/ &&
		    $realfile !~ m{^$rcu_trace_paths}) {
			WARN("RCU_TASKS_TRACE",
			     "use of RCU tasks trace '$1' is incorrect outside BPF or core RCU code\n" . $herecurr);			}
		}

Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace

[Paul E. McKenney]

On Mon, Jul 17, 2023 at 03:34:14PM -0700, Joe Perches wrote:
> On Mon, 2023-07-17 at 11:04 -0700, Paul E. McKenney wrote:
> > RCU Tasks Trace is quite specialized, having been created specifically
> > for sleepable BPF programs.  Because it allows general blocking within
> > readers, any new use of RCU Tasks Trace must take current use cases into
> > account.  Therefore, update checkpatch.pl to complain about use of any of
> > the RCU Tasks Trace API members outside of BPF and outside of RCU itself.
> > 
> > Cc: Andy Whitcroft <apw@canonical.com> (maintainer:CHECKPATCH)
> > Cc: Joe Perches <joe@perches.com> (maintainer:CHECKPATCH)
> > Cc: Dwaipayan Ray <dwaipayanray1@gmail.com> (reviewer:CHECKPATCH)
> > Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > Cc: John Fastabend <john.fastabend@gmail.com>
> > Cc: <bpf@vger.kernel.org>
> > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > ---
> >  scripts/checkpatch.pl | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -7457,6 +7457,24 @@ sub process {
> >  			}
> >  		}
> >  
> > +# Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> > +		if ($line =~ /\brcu_read_lock_trace\s*\(/ ||
> > +		    $line =~ /\brcu_read_lock_trace_held\s*\(/ ||
> > +		    $line =~ /\brcu_read_unlock_trace\s*\(/ ||
> > +		    $line =~ /\bcall_rcu_tasks_trace\s*\(/ ||
> > +		    $line =~ /\bsynchronize_rcu_tasks_trace\s*\(/ ||
> > +		    $line =~ /\brcu_barrier_tasks_trace\s*\(/ ||
> > +		    $line =~ /\brcu_request_urgent_qs_task\s*\(/) {
> > +			if ($realfile !~ m@^kernel/bpf@ &&
> > +			    $realfile !~ m@^include/linux/bpf@ &&
> > +			    $realfile !~ m@^net/bpf@ &&
> > +			    $realfile !~ m@^kernel/rcu@ &&
> > +			    $realfile !~ m@^include/linux/rcu@) {
> 
> Functions and paths like these tend to be accreted.
> 
> Please use a variable or 2 like:
> 
> our $rcu_trace_funcs = qr{(?x:
> 	rcu_read_lock_trace |
> 	rcu_read_lock_trace_held |
> 	rcu_read_unlock_trace |
> 	call_rcu_tasks_trace |
> 	synchronize_rcu_tasks_trace |
> 	rcu_barrier_tasks_trace |
> 	rcu_request_urgent_qs_task
> )};
> our $rcu_trace_paths = qr{(?x:
> 	kernel/bfp/ |
> 	include/linux/bpf |
> 	net/bpf/ |
> 	kernel/rcu/ |
> 	include/linux/rcu
> )};

Like this?

# Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
		our $rcu_trace_funcs = qr{(?x:
			rcu_read_lock_trace |
			rcu_read_lock_trace_held |
			rcu_read_unlock_trace |
			call_rcu_tasks_trace |
			synchronize_rcu_tasks_trace |
			rcu_barrier_tasks_trace |
			rcu_request_urgent_qs_task
		)};
		our $rcu_trace_paths = qr{(?x:
			kernel/bfp/ |
			include/linux/bpf |
			net/bpf/ |
			kernel/rcu/ |
			include/linux/rcu
		)};
		if ($line =~ /$rcu_trace_funcs/) {
			if ($realfile !~ m@^$rcu_trace_paths@) {
				WARN("RCU_TASKS_TRACE",
				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
			}
		}

No, that is definitely wrong.  It has lost track of the list of pathnames,
thus complaining about uses of those functions in files where their use
is permitted.

But this seems to work:

# Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
		our $rcu_trace_funcs = qr{(?x:
			rcu_read_lock_trace |
			rcu_read_lock_trace_held |
			rcu_read_unlock_trace |
			call_rcu_tasks_trace |
			synchronize_rcu_tasks_trace |
			rcu_barrier_tasks_trace |
			rcu_request_urgent_qs_task
		)};
		if ($line =~ /\b$rcu_trace_funcs\s*\(/) {
			if ($realfile !~ m@^kernel/bpf@ &&
			    $realfile !~ m@^include/linux/bpf@ &&
			    $realfile !~ m@^net/bpf@ &&
			    $realfile !~ m@^kernel/rcu@ &&
			    $realfile !~ m@^include/linux/rcu@) {
				WARN("RCU_TASKS_TRACE",
				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
			}
		}

Maybe the "^" needs to be distributed into $rcu_trace_paths?

# Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
		our $rcu_trace_funcs = qr{(?x:
			rcu_read_lock_trace |
			rcu_read_lock_trace_held |
			rcu_read_unlock_trace |
			call_rcu_tasks_trace |
			synchronize_rcu_tasks_trace |
			rcu_barrier_tasks_trace |
			rcu_request_urgent_qs_task
		)};
		our $rcu_trace_paths = qr{(?x:
			^kernel/bfp/ |
			^include/linux/bpf |
			^net/bpf/ |
			^kernel/rcu/ |
			^include/linux/rcu
		)};
		if ($line =~ /\b$rcu_trace_funcs\s*\(/) {
			if ($realfile !~ m@$rcu_trace_paths@) {
				WARN("RCU_TASKS_TRACE",
				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
			}
		}

But no joy here, either.  Which is no surprise, given that perl is
happy to distribute the "\b" and the "\s*\(" across the elements of
$rcu_trace_funcs.  I tried a number of other variations, including
inverting the "if" condition "(!(... =~ ...))", inverting the "if"
condition via an empty "then" clause, replacing the "m@" with "/",
replacing the "|" in the "qr{}" with "&", and a few others.

Again, listing the pathnames explicitly in the second "if" condition
works just fine.

Help?

							Thanx, Paul

Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace

[Joel Fernandes]

On 7/17/23 19:34, Paul E. McKenney wrote:
> On Mon, Jul 17, 2023 at 03:34:14PM -0700, Joe Perches wrote:
>> On Mon, 2023-07-17 at 11:04 -0700, Paul E. McKenney wrote:
>>> RCU Tasks Trace is quite specialized, having been created specifically
>>> for sleepable BPF programs.  Because it allows general blocking within
>>> readers, any new use of RCU Tasks Trace must take current use cases into
>>> account.  Therefore, update checkpatch.pl to complain about use of any of
>>> the RCU Tasks Trace API members outside of BPF and outside of RCU itself.
>>>
>>> Cc: Andy Whitcroft <apw@canonical.com> (maintainer:CHECKPATCH)
>>> Cc: Joe Perches <joe@perches.com> (maintainer:CHECKPATCH)
>>> Cc: Dwaipayan Ray <dwaipayanray1@gmail.com> (reviewer:CHECKPATCH)
>>> Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
>>> Cc: Alexei Starovoitov <ast@kernel.org>
>>> Cc: Daniel Borkmann <daniel@iogearbox.net>
>>> Cc: John Fastabend <john.fastabend@gmail.com>
>>> Cc: <bpf@vger.kernel.org>
>>> Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
>>> ---
>>>   scripts/checkpatch.pl | 18 ++++++++++++++++++
>>>   1 file changed, 18 insertions(+)
>>>
>>> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
>> []
>>> @@ -7457,6 +7457,24 @@ sub process {
>>>   			}
>>>   		}
>>>   
>>> +# Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
>>> +		if ($line =~ /\brcu_read_lock_trace\s*\(/ ||
>>> +		    $line =~ /\brcu_read_lock_trace_held\s*\(/ ||
>>> +		    $line =~ /\brcu_read_unlock_trace\s*\(/ ||
>>> +		    $line =~ /\bcall_rcu_tasks_trace\s*\(/ ||
>>> +		    $line =~ /\bsynchronize_rcu_tasks_trace\s*\(/ ||
>>> +		    $line =~ /\brcu_barrier_tasks_trace\s*\(/ ||
>>> +		    $line =~ /\brcu_request_urgent_qs_task\s*\(/) {
>>> +			if ($realfile !~ m@^kernel/bpf@ &&
>>> +			    $realfile !~ m@^include/linux/bpf@ &&
>>> +			    $realfile !~ m@^net/bpf@ &&
>>> +			    $realfile !~ m@^kernel/rcu@ &&
>>> +			    $realfile !~ m@^include/linux/rcu@) {
>>
>> Functions and paths like these tend to be accreted.
>>
>> Please use a variable or 2 like:
>>
>> our $rcu_trace_funcs = qr{(?x:
>> 	rcu_read_lock_trace |
>> 	rcu_read_lock_trace_held |
>> 	rcu_read_unlock_trace |
>> 	call_rcu_tasks_trace |
>> 	synchronize_rcu_tasks_trace |
>> 	rcu_barrier_tasks_trace |
>> 	rcu_request_urgent_qs_task
>> )};
>> our $rcu_trace_paths = qr{(?x:
>> 	kernel/bfp/ |
>> 	include/linux/bpf |
>> 	net/bpf/ |
>> 	kernel/rcu/ |
>> 	include/linux/rcu
>> )};
> 
> Like this?
> 
> # Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> 		our $rcu_trace_funcs = qr{(?x:
> 			rcu_read_lock_trace |
> 			rcu_read_lock_trace_held |
> 			rcu_read_unlock_trace |
> 			call_rcu_tasks_trace |
> 			synchronize_rcu_tasks_trace |
> 			rcu_barrier_tasks_trace |
> 			rcu_request_urgent_qs_task
> 		)};
> 		our $rcu_trace_paths = qr{(?x:
> 			kernel/bfp/ |
> 			include/linux/bpf |
> 			net/bpf/ |
> 			kernel/rcu/ |
> 			include/linux/rcu
> 		)};
> 		if ($line =~ /$rcu_trace_funcs/) {
> 			if ($realfile !~ m@^$rcu_trace_paths@) {
> 				WARN("RCU_TASKS_TRACE",
> 				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
> 			}
> 		}
> 
> No, that is definitely wrong.  It has lost track of the list of pathnames,
> thus complaining about uses of those functions in files where their use
> is permitted.
> 
> But this seems to work:
> 
> # Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> 		our $rcu_trace_funcs = qr{(?x:
> 			rcu_read_lock_trace |
> 			rcu_read_lock_trace_held |
> 			rcu_read_unlock_trace |
> 			call_rcu_tasks_trace |
> 			synchronize_rcu_tasks_trace |
> 			rcu_barrier_tasks_trace |
> 			rcu_request_urgent_qs_task
> 		)};
> 		if ($line =~ /\b$rcu_trace_funcs\s*\(/) {
> 			if ($realfile !~ m@^kernel/bpf@ &&
> 			    $realfile !~ m@^include/linux/bpf@ &&
> 			    $realfile !~ m@^net/bpf@ &&
> 			    $realfile !~ m@^kernel/rcu@ &&
> 			    $realfile !~ m@^include/linux/rcu@) {
> 				WARN("RCU_TASKS_TRACE",
> 				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
> 			}
> 		}
> 
> Maybe the "^" needs to be distributed into $rcu_trace_paths?
> 
> # Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> 		our $rcu_trace_funcs = qr{(?x:
> 			rcu_read_lock_trace |
> 			rcu_read_lock_trace_held |
> 			rcu_read_unlock_trace |
> 			call_rcu_tasks_trace |
> 			synchronize_rcu_tasks_trace |
> 			rcu_barrier_tasks_trace |
> 			rcu_request_urgent_qs_task
> 		)};
> 		our $rcu_trace_paths = qr{(?x:
> 			^kernel/bfp/ |
> 			^include/linux/bpf |
> 			^net/bpf/ |
> 			^kernel/rcu/ |
> 			^include/linux/rcu
> 		)};
> 		if ($line =~ /\b$rcu_trace_funcs\s*\(/) {
> 			if ($realfile !~ m@$rcu_trace_paths@) {
> 				WARN("RCU_TASKS_TRACE",
> 				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
> 			}
> 		}
> 
> But no joy here, either.  Which is no surprise, given that perl is
> happy to distribute the "\b" and the "\s*\(" across the elements of
> $rcu_trace_funcs.  I tried a number of other variations, including
> inverting the "if" condition "(!(... =~ ...))", inverting the "if"
> condition via an empty "then" clause, replacing the "m@" with "/",
> replacing the "|" in the "qr{}" with "&", and a few others.
> 
> Again, listing the pathnames explicitly in the second "if" condition
> works just fine.
> 

Not a perl expert but I wonder if the following are any options at all:

1. Instead of having a complex list of strings in a regex variable, it 
might be easier to hold the strings as a perl array, and then iterate 
over that array checking each element of the array on every iteration, 
against the line.

2. Roll the "\b" and/or "^" in into the regex variable itself than 
trying make them play with the variable later.

3. Use parentheses around the variable? Not sure if that will work but I 
wonder if it has something to do with operator precedence.

4. Instead of a list of paths, maybe it is better to look for "rcu" or 
"bpf" in the regex itself? That way new paths don't require script 
updates (at the expense though of false-positives (highly unlikely, IMHO)).

thanks,

- Joel

Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace

[Paul E. McKenney]

On Wed, Jul 19, 2023 at 07:51:58AM -0400, Joel Fernandes wrote:
> 
> 
> On 7/17/23 19:34, Paul E. McKenney wrote:
> > On Mon, Jul 17, 2023 at 03:34:14PM -0700, Joe Perches wrote:
> > > On Mon, 2023-07-17 at 11:04 -0700, Paul E. McKenney wrote:
> > > > RCU Tasks Trace is quite specialized, having been created specifically
> > > > for sleepable BPF programs.  Because it allows general blocking within
> > > > readers, any new use of RCU Tasks Trace must take current use cases into
> > > > account.  Therefore, update checkpatch.pl to complain about use of any of
> > > > the RCU Tasks Trace API members outside of BPF and outside of RCU itself.
> > > > 
> > > > Cc: Andy Whitcroft <apw@canonical.com> (maintainer:CHECKPATCH)
> > > > Cc: Joe Perches <joe@perches.com> (maintainer:CHECKPATCH)
> > > > Cc: Dwaipayan Ray <dwaipayanray1@gmail.com> (reviewer:CHECKPATCH)
> > > > Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > > > Cc: John Fastabend <john.fastabend@gmail.com>
> > > > Cc: <bpf@vger.kernel.org>
> > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > ---
> > > >   scripts/checkpatch.pl | 18 ++++++++++++++++++
> > > >   1 file changed, 18 insertions(+)
> > > > 
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > []
> > > > @@ -7457,6 +7457,24 @@ sub process {
> > > >   			}
> > > >   		}
> > > > +# Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> > > > +		if ($line =~ /\brcu_read_lock_trace\s*\(/ ||
> > > > +		    $line =~ /\brcu_read_lock_trace_held\s*\(/ ||
> > > > +		    $line =~ /\brcu_read_unlock_trace\s*\(/ ||
> > > > +		    $line =~ /\bcall_rcu_tasks_trace\s*\(/ ||
> > > > +		    $line =~ /\bsynchronize_rcu_tasks_trace\s*\(/ ||
> > > > +		    $line =~ /\brcu_barrier_tasks_trace\s*\(/ ||
> > > > +		    $line =~ /\brcu_request_urgent_qs_task\s*\(/) {
> > > > +			if ($realfile !~ m@^kernel/bpf@ &&
> > > > +			    $realfile !~ m@^include/linux/bpf@ &&
> > > > +			    $realfile !~ m@^net/bpf@ &&
> > > > +			    $realfile !~ m@^kernel/rcu@ &&
> > > > +			    $realfile !~ m@^include/linux/rcu@) {
> > > 
> > > Functions and paths like these tend to be accreted.
> > > 
> > > Please use a variable or 2 like:
> > > 
> > > our $rcu_trace_funcs = qr{(?x:
> > > 	rcu_read_lock_trace |
> > > 	rcu_read_lock_trace_held |
> > > 	rcu_read_unlock_trace |
> > > 	call_rcu_tasks_trace |
> > > 	synchronize_rcu_tasks_trace |
> > > 	rcu_barrier_tasks_trace |
> > > 	rcu_request_urgent_qs_task
> > > )};
> > > our $rcu_trace_paths = qr{(?x:
> > > 	kernel/bfp/ |
> > > 	include/linux/bpf |
> > > 	net/bpf/ |
> > > 	kernel/rcu/ |
> > > 	include/linux/rcu
> > > )};
> > 
> > Like this?
> > 
> > # Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> > 		our $rcu_trace_funcs = qr{(?x:
> > 			rcu_read_lock_trace |
> > 			rcu_read_lock_trace_held |
> > 			rcu_read_unlock_trace |
> > 			call_rcu_tasks_trace |
> > 			synchronize_rcu_tasks_trace |
> > 			rcu_barrier_tasks_trace |
> > 			rcu_request_urgent_qs_task
> > 		)};
> > 		our $rcu_trace_paths = qr{(?x:
> > 			kernel/bfp/ |
> > 			include/linux/bpf |
> > 			net/bpf/ |
> > 			kernel/rcu/ |
> > 			include/linux/rcu
> > 		)};
> > 		if ($line =~ /$rcu_trace_funcs/) {
> > 			if ($realfile !~ m@^$rcu_trace_paths@) {
> > 				WARN("RCU_TASKS_TRACE",
> > 				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
> > 			}
> > 		}
> > 
> > No, that is definitely wrong.  It has lost track of the list of pathnames,
> > thus complaining about uses of those functions in files where their use
> > is permitted.
> > 
> > But this seems to work:
> > 
> > # Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> > 		our $rcu_trace_funcs = qr{(?x:
> > 			rcu_read_lock_trace |
> > 			rcu_read_lock_trace_held |
> > 			rcu_read_unlock_trace |
> > 			call_rcu_tasks_trace |
> > 			synchronize_rcu_tasks_trace |
> > 			rcu_barrier_tasks_trace |
> > 			rcu_request_urgent_qs_task
> > 		)};
> > 		if ($line =~ /\b$rcu_trace_funcs\s*\(/) {
> > 			if ($realfile !~ m@^kernel/bpf@ &&
> > 			    $realfile !~ m@^include/linux/bpf@ &&
> > 			    $realfile !~ m@^net/bpf@ &&
> > 			    $realfile !~ m@^kernel/rcu@ &&
> > 			    $realfile !~ m@^include/linux/rcu@) {
> > 				WARN("RCU_TASKS_TRACE",
> > 				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
> > 			}
> > 		}
> > 
> > Maybe the "^" needs to be distributed into $rcu_trace_paths?
> > 
> > # Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> > 		our $rcu_trace_funcs = qr{(?x:
> > 			rcu_read_lock_trace |
> > 			rcu_read_lock_trace_held |
> > 			rcu_read_unlock_trace |
> > 			call_rcu_tasks_trace |
> > 			synchronize_rcu_tasks_trace |
> > 			rcu_barrier_tasks_trace |
> > 			rcu_request_urgent_qs_task
> > 		)};
> > 		our $rcu_trace_paths = qr{(?x:
> > 			^kernel/bfp/ |
> > 			^include/linux/bpf |
> > 			^net/bpf/ |
> > 			^kernel/rcu/ |
> > 			^include/linux/rcu
> > 		)};
> > 		if ($line =~ /\b$rcu_trace_funcs\s*\(/) {
> > 			if ($realfile !~ m@$rcu_trace_paths@) {
> > 				WARN("RCU_TASKS_TRACE",
> > 				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
> > 			}
> > 		}
> > 
> > But no joy here, either.  Which is no surprise, given that perl is
> > happy to distribute the "\b" and the "\s*\(" across the elements of
> > $rcu_trace_funcs.  I tried a number of other variations, including
> > inverting the "if" condition "(!(... =~ ...))", inverting the "if"
> > condition via an empty "then" clause, replacing the "m@" with "/",
> > replacing the "|" in the "qr{}" with "&", and a few others.
> > 
> > Again, listing the pathnames explicitly in the second "if" condition
> > works just fine.
> > 
> 
> Not a perl expert but I wonder if the following are any options at all:
> 
> 1. Instead of having a complex list of strings in a regex variable, it might
> be easier to hold the strings as a perl array, and then iterate over that
> array checking each element of the array on every iteration, against the
> line.
> 
> 2. Roll the "\b" and/or "^" in into the regex variable itself than trying
> make them play with the variable later.
> 
> 3. Use parentheses around the variable? Not sure if that will work but I
> wonder if it has something to do with operator precedence.
> 
> 4. Instead of a list of paths, maybe it is better to look for "rcu" or "bpf"
> in the regex itself? That way new paths don't require script updates (at the
> expense though of false-positives (highly unlikely, IMHO)).

Given perl's tendency to have corner cases in its corner cases, I
am guessing that the "^" character combined with the "/" character is
causing trouble here.  Especially given that I don't see any use of such
a pattern anywhere in checkpatch.pl except directly in a "~" expression,
and there are a lot of those.

So I will keep it as is unless I hear otherwise from Joe Perches.

							Thanx, Paul

Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace

[Joe Perches]

On Wed, 2023-07-19 at 11:27 -0700, Paul E. McKenney wrote:
[]
> > > > 
> Given perl's tendency to have corner cases in its corner cases, I
> am guessing that the "^" character combined with the "/" character is
> causing trouble here.  Especially given that I don't see any use of such
> a pattern anywhere in checkpatch.pl except directly in a "~" expression,
> and there are a lot of those.
> 
> So I will keep it as is unless I hear otherwise from Joe Perches.

I played with it a bit and can't think of anything better.

Code is always something that can be improved later so the
way Paul has it now is fine with me.

Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace

[Paul E. McKenney]

On Wed, Jul 19, 2023 at 11:44:17AM -0700, Joe Perches wrote:
> On Wed, 2023-07-19 at 11:27 -0700, Paul E. McKenney wrote:
> []
> > > > > 
> > Given perl's tendency to have corner cases in its corner cases, I
> > am guessing that the "^" character combined with the "/" character is
> > causing trouble here.  Especially given that I don't see any use of such
> > a pattern anywhere in checkpatch.pl except directly in a "~" expression,
> > and there are a lot of those.
> > 
> > So I will keep it as is unless I hear otherwise from Joe Perches.
> 
> I played with it a bit and can't think of anything better.
> 
> Code is always something that can be improved later so the
> way Paul has it now is fine with me.

Then I don't feel so bad not finding anything better myself.

Thank you, and if nothing else this exercise refreshed a few of my
perl neurons.  ;-)

							Thanx, Paul

Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace

[Joe Perches]

On Mon, 2023-07-17 at 16:34 -0700, Paul E. McKenney wrote:
> On Mon, Jul 17, 2023 at 03:34:14PM -0700, Joe Perches wrote:
> > On Mon, 2023-07-17 at 11:04 -0700, Paul E. McKenney wrote:
> > > RCU Tasks Trace is quite specialized, having been created specifically
> > > for sleepable BPF programs.  Because it allows general blocking within
> > > readers, any new use of RCU Tasks Trace must take current use cases into
> > > account.  Therefore, update checkpatch.pl to complain about use of any of
> > > the RCU Tasks Trace API members outside of BPF and outside of RCU itself.
> > > 
> > > Cc: Andy Whitcroft <apw@canonical.com> (maintainer:CHECKPATCH)
> > > Cc: Joe Perches <joe@perches.com> (maintainer:CHECKPATCH)
> > > Cc: Dwaipayan Ray <dwaipayanray1@gmail.com> (reviewer:CHECKPATCH)
> > > Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > > Cc: John Fastabend <john.fastabend@gmail.com>
> > > Cc: <bpf@vger.kernel.org>
> > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > ---
> > >  scripts/checkpatch.pl | 18 ++++++++++++++++++
> > >  1 file changed, 18 insertions(+)
> > > 
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > []
> > > @@ -7457,6 +7457,24 @@ sub process {
> > >  			}
> > >  		}
> > >  
> > > +# Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> > > +		if ($line =~ /\brcu_read_lock_trace\s*\(/ ||
> > > +		    $line =~ /\brcu_read_lock_trace_held\s*\(/ ||
> > > +		    $line =~ /\brcu_read_unlock_trace\s*\(/ ||
> > > +		    $line =~ /\bcall_rcu_tasks_trace\s*\(/ ||
> > > +		    $line =~ /\bsynchronize_rcu_tasks_trace\s*\(/ ||
> > > +		    $line =~ /\brcu_barrier_tasks_trace\s*\(/ ||
> > > +		    $line =~ /\brcu_request_urgent_qs_task\s*\(/) {
> > > +			if ($realfile !~ m@^kernel/bpf@ &&
> > > +			    $realfile !~ m@^include/linux/bpf@ &&
> > > +			    $realfile !~ m@^net/bpf@ &&
> > > +			    $realfile !~ m@^kernel/rcu@ &&
> > > +			    $realfile !~ m@^include/linux/rcu@) {
> > 
> > Functions and paths like these tend to be accreted.
> > 
> > Please use a variable or 2 like:
> > 
> > our $rcu_trace_funcs = qr{(?x:
> > 	rcu_read_lock_trace |
> > 	rcu_read_lock_trace_held |
> > 	rcu_read_unlock_trace |
> > 	call_rcu_tasks_trace |
> > 	synchronize_rcu_tasks_trace |
> > 	rcu_barrier_tasks_trace |
> > 	rcu_request_urgent_qs_task
> > )};
> > our $rcu_trace_paths = qr{(?x:
> > 	kernel/bfp/ |
		^^
	kernel/bfp/ |

(umm, oops...)
I think my original suggestion works better when I don't typo the path.

> > 	include/linux/bpf |
> > 	net/bpf/ |
> > 	kernel/rcu/ |
> > 	include/linux/rcu
> > )};
> 
> Like this?
> 
> # Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> 		our $rcu_trace_funcs = qr{(?x:
> 			rcu_read_lock_trace |
> 			rcu_read_lock_trace_held |
> 			rcu_read_unlock_trace |
> 			call_rcu_tasks_trace |
> 			synchronize_rcu_tasks_trace |
> 			rcu_barrier_tasks_trace |
> 			rcu_request_urgent_qs_task
> 		)};
> 		our $rcu_trace_paths = qr{(?x:
> 			kernel/bfp/ |
> 			include/linux/bpf |
> 			net/bpf/ |
> 			kernel/rcu/ |
> 			include/linux/rcu
> 		)};
> 		if ($line =~ /$rcu_trace_funcs/) {
> 			if ($realfile !~ m@^$rcu_trace_paths@) {
> 				WARN("RCU_TASKS_TRACE",
> 				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
> 			}
> 		}
> 
> No, that is definitely wrong.  It has lost track of the list of pathnames,
> thus complaining about uses of those functions in files where their use
> is permitted.
> 
> But this seems to work:
> 
> # Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> 		our $rcu_trace_funcs = qr{(?x:
> 			rcu_read_lock_trace |
> 			rcu_read_lock_trace_held |
> 			rcu_read_unlock_trace |
> 			call_rcu_tasks_trace |
> 			synchronize_rcu_tasks_trace |
> 			rcu_barrier_tasks_trace |
> 			rcu_request_urgent_qs_task
> 		)};
> 		if ($line =~ /\b$rcu_trace_funcs\s*\(/) {
> 			if ($realfile !~ m@^kernel/bpf@ &&
> 			    $realfile !~ m@^include/linux/bpf@ &&
> 			    $realfile !~ m@^net/bpf@ &&
> 			    $realfile !~ m@^kernel/rcu@ &&
> 			    $realfile !~ m@^include/linux/rcu@) {
> 				WARN("RCU_TASKS_TRACE",
> 				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
> 			}
> 		}
> 
> Maybe the "^" needs to be distributed into $rcu_trace_paths?
> 
> # Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> 		our $rcu_trace_funcs = qr{(?x:
> 			rcu_read_lock_trace |
> 			rcu_read_lock_trace_held |
> 			rcu_read_unlock_trace |
> 			call_rcu_tasks_trace |
> 			synchronize_rcu_tasks_trace |
> 			rcu_barrier_tasks_trace |
> 			rcu_request_urgent_qs_task
> 		)};
> 		our $rcu_trace_paths = qr{(?x:
> 			^kernel/bfp/ |
> 			^include/linux/bpf |
> 			^net/bpf/ |
> 			^kernel/rcu/ |
> 			^include/linux/rcu
> 		)};
> 		if ($line =~ /\b$rcu_trace_funcs\s*\(/) {
> 			if ($realfile !~ m@$rcu_trace_paths@) {
> 				WARN("RCU_TASKS_TRACE",
> 				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
> 			}
> 		}
> 
> But no joy here, either.  Which is no surprise, given that perl is
> happy to distribute the "\b" and the "\s*\(" across the elements of
> $rcu_trace_funcs.  I tried a number of other variations, including
> inverting the "if" condition "(!(... =~ ...))", inverting the "if"
> condition via an empty "then" clause, replacing the "m@" with "/",
> replacing the "|" in the "qr{}" with "&", and a few others.
> 
> Again, listing the pathnames explicitly in the second "if" condition
> works just fine.
> 
> Help?
> 
> 							Thanx, Paul

Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace

[Paul E. McKenney]

On Thu, Jul 20, 2023 at 12:29:42AM -0700, Joe Perches wrote:
> On Mon, 2023-07-17 at 16:34 -0700, Paul E. McKenney wrote:
> > On Mon, Jul 17, 2023 at 03:34:14PM -0700, Joe Perches wrote:
> > > On Mon, 2023-07-17 at 11:04 -0700, Paul E. McKenney wrote:
> > > > RCU Tasks Trace is quite specialized, having been created specifically
> > > > for sleepable BPF programs.  Because it allows general blocking within
> > > > readers, any new use of RCU Tasks Trace must take current use cases into
> > > > account.  Therefore, update checkpatch.pl to complain about use of any of
> > > > the RCU Tasks Trace API members outside of BPF and outside of RCU itself.
> > > > 
> > > > Cc: Andy Whitcroft <apw@canonical.com> (maintainer:CHECKPATCH)
> > > > Cc: Joe Perches <joe@perches.com> (maintainer:CHECKPATCH)
> > > > Cc: Dwaipayan Ray <dwaipayanray1@gmail.com> (reviewer:CHECKPATCH)
> > > > Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > > > Cc: John Fastabend <john.fastabend@gmail.com>
> > > > Cc: <bpf@vger.kernel.org>
> > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > ---
> > > >  scripts/checkpatch.pl | 18 ++++++++++++++++++
> > > >  1 file changed, 18 insertions(+)
> > > > 
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > []
> > > > @@ -7457,6 +7457,24 @@ sub process {
> > > >  			}
> > > >  		}
> > > >  
> > > > +# Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> > > > +		if ($line =~ /\brcu_read_lock_trace\s*\(/ ||
> > > > +		    $line =~ /\brcu_read_lock_trace_held\s*\(/ ||
> > > > +		    $line =~ /\brcu_read_unlock_trace\s*\(/ ||
> > > > +		    $line =~ /\bcall_rcu_tasks_trace\s*\(/ ||
> > > > +		    $line =~ /\bsynchronize_rcu_tasks_trace\s*\(/ ||
> > > > +		    $line =~ /\brcu_barrier_tasks_trace\s*\(/ ||
> > > > +		    $line =~ /\brcu_request_urgent_qs_task\s*\(/) {
> > > > +			if ($realfile !~ m@^kernel/bpf@ &&
> > > > +			    $realfile !~ m@^include/linux/bpf@ &&
> > > > +			    $realfile !~ m@^net/bpf@ &&
> > > > +			    $realfile !~ m@^kernel/rcu@ &&
> > > > +			    $realfile !~ m@^include/linux/rcu@) {
> > > 
> > > Functions and paths like these tend to be accreted.
> > > 
> > > Please use a variable or 2 like:
> > > 
> > > our $rcu_trace_funcs = qr{(?x:
> > > 	rcu_read_lock_trace |
> > > 	rcu_read_lock_trace_held |
> > > 	rcu_read_unlock_trace |
> > > 	call_rcu_tasks_trace |
> > > 	synchronize_rcu_tasks_trace |
> > > 	rcu_barrier_tasks_trace |
> > > 	rcu_request_urgent_qs_task
> > > )};
> > > our $rcu_trace_paths = qr{(?x:
> > > 	kernel/bfp/ |
> 		^^
> 	kernel/bfp/ |
> 
> (umm, oops...)
> I think my original suggestion works better when I don't typo the path.

Color me blind!  ;-)

That works much better, thank you!  I will update the patch on my
next rebase.

							Thanx, Paul

> > > 	include/linux/bpf |
> > > 	net/bpf/ |
> > > 	kernel/rcu/ |
> > > 	include/linux/rcu
> > > )};
> > 
> > Like this?
> > 
> > # Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> > 		our $rcu_trace_funcs = qr{(?x:
> > 			rcu_read_lock_trace |
> > 			rcu_read_lock_trace_held |
> > 			rcu_read_unlock_trace |
> > 			call_rcu_tasks_trace |
> > 			synchronize_rcu_tasks_trace |
> > 			rcu_barrier_tasks_trace |
> > 			rcu_request_urgent_qs_task
> > 		)};
> > 		our $rcu_trace_paths = qr{(?x:
> > 			kernel/bfp/ |
> > 			include/linux/bpf |
> > 			net/bpf/ |
> > 			kernel/rcu/ |
> > 			include/linux/rcu
> > 		)};
> > 		if ($line =~ /$rcu_trace_funcs/) {
> > 			if ($realfile !~ m@^$rcu_trace_paths@) {
> > 				WARN("RCU_TASKS_TRACE",
> > 				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
> > 			}
> > 		}
> > 
> > No, that is definitely wrong.  It has lost track of the list of pathnames,
> > thus complaining about uses of those functions in files where their use
> > is permitted.
> > 
> > But this seems to work:
> > 
> > # Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> > 		our $rcu_trace_funcs = qr{(?x:
> > 			rcu_read_lock_trace |
> > 			rcu_read_lock_trace_held |
> > 			rcu_read_unlock_trace |
> > 			call_rcu_tasks_trace |
> > 			synchronize_rcu_tasks_trace |
> > 			rcu_barrier_tasks_trace |
> > 			rcu_request_urgent_qs_task
> > 		)};
> > 		if ($line =~ /\b$rcu_trace_funcs\s*\(/) {
> > 			if ($realfile !~ m@^kernel/bpf@ &&
> > 			    $realfile !~ m@^include/linux/bpf@ &&
> > 			    $realfile !~ m@^net/bpf@ &&
> > 			    $realfile !~ m@^kernel/rcu@ &&
> > 			    $realfile !~ m@^include/linux/rcu@) {
> > 				WARN("RCU_TASKS_TRACE",
> > 				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
> > 			}
> > 		}
> > 
> > Maybe the "^" needs to be distributed into $rcu_trace_paths?
> > 
> > # Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> > 		our $rcu_trace_funcs = qr{(?x:
> > 			rcu_read_lock_trace |
> > 			rcu_read_lock_trace_held |
> > 			rcu_read_unlock_trace |
> > 			call_rcu_tasks_trace |
> > 			synchronize_rcu_tasks_trace |
> > 			rcu_barrier_tasks_trace |
> > 			rcu_request_urgent_qs_task
> > 		)};
> > 		our $rcu_trace_paths = qr{(?x:
> > 			^kernel/bfp/ |
> > 			^include/linux/bpf |
> > 			^net/bpf/ |
> > 			^kernel/rcu/ |
> > 			^include/linux/rcu
> > 		)};
> > 		if ($line =~ /\b$rcu_trace_funcs\s*\(/) {
> > 			if ($realfile !~ m@$rcu_trace_paths@) {
> > 				WARN("RCU_TASKS_TRACE",
> > 				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
> > 			}
> > 		}
> > 
> > But no joy here, either.  Which is no surprise, given that perl is
> > happy to distribute the "\b" and the "\s*\(" across the elements of
> > $rcu_trace_funcs.  I tried a number of other variations, including
> > inverting the "if" condition "(!(... =~ ...))", inverting the "if"
> > condition via an empty "then" clause, replacing the "m@" with "/",
> > replacing the "|" in the "qr{}" with "&", and a few others.
> > 
> > Again, listing the pathnames explicitly in the second "if" condition
> > works just fine.
> > 
> > Help?
> > 
> > 							Thanx, Paul
> 

Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace

[Paul E. McKenney]

On Thu, Jul 20, 2023 at 12:43:55PM -0700, Paul E. McKenney wrote:
> On Thu, Jul 20, 2023 at 12:29:42AM -0700, Joe Perches wrote:
> > On Mon, 2023-07-17 at 16:34 -0700, Paul E. McKenney wrote:
> > > On Mon, Jul 17, 2023 at 03:34:14PM -0700, Joe Perches wrote:
> > > > On Mon, 2023-07-17 at 11:04 -0700, Paul E. McKenney wrote:
> > > > > RCU Tasks Trace is quite specialized, having been created specifically
> > > > > for sleepable BPF programs.  Because it allows general blocking within
> > > > > readers, any new use of RCU Tasks Trace must take current use cases into
> > > > > account.  Therefore, update checkpatch.pl to complain about use of any of
> > > > > the RCU Tasks Trace API members outside of BPF and outside of RCU itself.
> > > > > 
> > > > > Cc: Andy Whitcroft <apw@canonical.com> (maintainer:CHECKPATCH)
> > > > > Cc: Joe Perches <joe@perches.com> (maintainer:CHECKPATCH)
> > > > > Cc: Dwaipayan Ray <dwaipayanray1@gmail.com> (reviewer:CHECKPATCH)
> > > > > Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > > > > Cc: Alexei Starovoitov <ast@kernel.org>
> > > > > Cc: Daniel Borkmann <daniel@iogearbox.net>
> > > > > Cc: John Fastabend <john.fastabend@gmail.com>
> > > > > Cc: <bpf@vger.kernel.org>
> > > > > Signed-off-by: Paul E. McKenney <paulmck@kernel.org>
> > > > > ---
> > > > >  scripts/checkpatch.pl | 18 ++++++++++++++++++
> > > > >  1 file changed, 18 insertions(+)
> > > > > 
> > > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > > []
> > > > > @@ -7457,6 +7457,24 @@ sub process {
> > > > >  			}
> > > > >  		}
> > > > >  
> > > > > +# Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> > > > > +		if ($line =~ /\brcu_read_lock_trace\s*\(/ ||
> > > > > +		    $line =~ /\brcu_read_lock_trace_held\s*\(/ ||
> > > > > +		    $line =~ /\brcu_read_unlock_trace\s*\(/ ||
> > > > > +		    $line =~ /\bcall_rcu_tasks_trace\s*\(/ ||
> > > > > +		    $line =~ /\bsynchronize_rcu_tasks_trace\s*\(/ ||
> > > > > +		    $line =~ /\brcu_barrier_tasks_trace\s*\(/ ||
> > > > > +		    $line =~ /\brcu_request_urgent_qs_task\s*\(/) {
> > > > > +			if ($realfile !~ m@^kernel/bpf@ &&
> > > > > +			    $realfile !~ m@^include/linux/bpf@ &&
> > > > > +			    $realfile !~ m@^net/bpf@ &&
> > > > > +			    $realfile !~ m@^kernel/rcu@ &&
> > > > > +			    $realfile !~ m@^include/linux/rcu@) {
> > > > 
> > > > Functions and paths like these tend to be accreted.
> > > > 
> > > > Please use a variable or 2 like:
> > > > 
> > > > our $rcu_trace_funcs = qr{(?x:
> > > > 	rcu_read_lock_trace |
> > > > 	rcu_read_lock_trace_held |
> > > > 	rcu_read_unlock_trace |
> > > > 	call_rcu_tasks_trace |
> > > > 	synchronize_rcu_tasks_trace |
> > > > 	rcu_barrier_tasks_trace |
> > > > 	rcu_request_urgent_qs_task
> > > > )};
> > > > our $rcu_trace_paths = qr{(?x:
> > > > 	kernel/bfp/ |
> > 		^^
> > 	kernel/bfp/ |
> > 
> > (umm, oops...)
> > I think my original suggestion works better when I don't typo the path.
> 
> Color me blind!  ;-)
> 
> That works much better, thank you!  I will update the patch on my
> next rebase.

As shown below.  Is this what you had in mind?

							Thanx, Paul

------------------------------------------------------------------------

commit 496aa3821b40459b107f4bbc14ca867daad21fb6
Author: Paul E. McKenney <paulmck@kernel.org>
Date:   Thu Jul 6 11:48:07 2023 -0700

    checkpatch: Complain about unexpected uses of RCU Tasks Trace
    
    RCU Tasks Trace is quite specialized, having been created specifically
    for sleepable BPF programs.  Because it allows general blocking within
    readers, any new use of RCU Tasks Trace must take current use cases into
    account.  Therefore, update checkpatch.pl to complain about use of any of
    the RCU Tasks Trace API members outside of BPF and outside of RCU itself.
    
    [ paulmck: Apply Joe Perches feedback. ]
    
    Cc: Andy Whitcroft <apw@canonical.com> (maintainer:CHECKPATCH)
    Cc: Joe Perches <joe@perches.com> (maintainer:CHECKPATCH)
    Cc: Dwaipayan Ray <dwaipayanray1@gmail.com> (reviewer:CHECKPATCH)
    Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
    Cc: Alexei Starovoitov <ast@kernel.org>
    Cc: Daniel Borkmann <daniel@iogearbox.net>
    Cc: John Fastabend <john.fastabend@gmail.com>
    Cc: <bpf@vger.kernel.org>
    Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 880fde13d9b8..a67e682e896c 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -7457,6 +7457,30 @@ sub process {
 			}
 		}
 
+# Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
+		our $rcu_trace_funcs = qr{(?x:
+			rcu_read_lock_trace |
+			rcu_read_lock_trace_held |
+			rcu_read_unlock_trace |
+			call_rcu_tasks_trace |
+			synchronize_rcu_tasks_trace |
+			rcu_barrier_tasks_trace |
+			rcu_request_urgent_qs_task
+		)};
+		our $rcu_trace_paths = qr{(?x:
+			kernel/bpf/ |
+			include/linux/bpf |
+			net/bpf/ |
+			kernel/rcu/ |
+			include/linux/rcu
+		)};
+		if ($line =~ /\b$rcu_trace_funcs\s*\(/) {
+			if ($realfile !~ m@^$rcu_trace_paths@) {
+				WARN("RCU_TASKS_TRACE",
+				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
+			}
+		}
+
 # check for lockdep_set_novalidate_class
 		if ($line =~ /^.\s*lockdep_set_novalidate_class\s*\(/ ||
 		    $line =~ /__lockdep_no_validate__\s*\)/ ) {

Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace

[Joe Perches]

On Thu, 2023-07-20 at 20:56 -0700, Paul E. McKenney wrote:

> 
> > That works much better, thank you!  I will update the patch on my
> > next rebase.
> 
> As shown below.  Is this what you had in mind?
[]
> commit 496aa3821b40459b107f4bbc14ca867daad21fb6
> Author: Paul E. McKenney <paulmck@kernel.org>
> Date:   Thu Jul 6 11:48:07 2023 -0700
> 
>     checkpatch: Complain about unexpected uses of RCU Tasks Trace
>     
>     RCU Tasks Trace is quite specialized, having been created specifically
>     for sleepable BPF programs.  Because it allows general blocking within
>     readers, any new use of RCU Tasks Trace must take current use cases into
>     account.  Therefore, update checkpatch.pl to complain about use of any of
>     the RCU Tasks Trace API members outside of BPF and outside of RCU itself.
[]
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
[]
> @@ -7457,6 +7457,30 @@ sub process {
>  			}
>  		}
>  
> +# Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> +		our $rcu_trace_funcs = qr{(?x:
> +			rcu_read_lock_trace |
> +			rcu_read_lock_trace_held |
> +			rcu_read_unlock_trace |
> +			call_rcu_tasks_trace |
> +			synchronize_rcu_tasks_trace |
> +			rcu_barrier_tasks_trace |
> +			rcu_request_urgent_qs_task
> +		)};
> +		our $rcu_trace_paths = qr{(?x:
> +			kernel/bpf/ |
> +			include/linux/bpf |
> +			net/bpf/ |
> +			kernel/rcu/ |
> +			include/linux/rcu
> +		)};
> +		if ($line =~ /\b$rcu_trace_funcs\s*\(/) {
> +			if ($realfile !~ m@^$rcu_trace_paths@) {
> +				WARN("RCU_TASKS_TRACE",
> +				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);

Exactly yes.

(though I still suggest a capture group to show the function like below)

		if ($line =~ /\b($rcu_trace_funcs)\s*\(/ &&
		    $realfile !~ m{^$rcu_trace_paths}) {
			WARN("RCU_TASKS_TRACE",
			     "use of RCU task trace '$1' is incorrect outside BPF or core RCU code\n" . $herecurr);
		}

Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace

[Paul E. McKenney]

On Thu, Jul 20, 2023 at 09:38:51PM -0700, Joe Perches wrote:
> On Thu, 2023-07-20 at 20:56 -0700, Paul E. McKenney wrote:
> 
> > 
> > > That works much better, thank you!  I will update the patch on my
> > > next rebase.
> > 
> > As shown below.  Is this what you had in mind?
> []
> > commit 496aa3821b40459b107f4bbc14ca867daad21fb6
> > Author: Paul E. McKenney <paulmck@kernel.org>
> > Date:   Thu Jul 6 11:48:07 2023 -0700
> > 
> >     checkpatch: Complain about unexpected uses of RCU Tasks Trace
> >     
> >     RCU Tasks Trace is quite specialized, having been created specifically
> >     for sleepable BPF programs.  Because it allows general blocking within
> >     readers, any new use of RCU Tasks Trace must take current use cases into
> >     account.  Therefore, update checkpatch.pl to complain about use of any of
> >     the RCU Tasks Trace API members outside of BPF and outside of RCU itself.
> []
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> []
> > @@ -7457,6 +7457,30 @@ sub process {
> >  			}
> >  		}
> >  
> > +# Complain about RCU Tasks Trace used outside of BPF (and of course, RCU).
> > +		our $rcu_trace_funcs = qr{(?x:
> > +			rcu_read_lock_trace |
> > +			rcu_read_lock_trace_held |
> > +			rcu_read_unlock_trace |
> > +			call_rcu_tasks_trace |
> > +			synchronize_rcu_tasks_trace |
> > +			rcu_barrier_tasks_trace |
> > +			rcu_request_urgent_qs_task
> > +		)};
> > +		our $rcu_trace_paths = qr{(?x:
> > +			kernel/bpf/ |
> > +			include/linux/bpf |
> > +			net/bpf/ |
> > +			kernel/rcu/ |
> > +			include/linux/rcu
> > +		)};
> > +		if ($line =~ /\b$rcu_trace_funcs\s*\(/) {
> > +			if ($realfile !~ m@^$rcu_trace_paths@) {
> > +				WARN("RCU_TASKS_TRACE",
> > +				     "use of RCU tasks trace is incorrect outside BPF or core RCU code\n" . $herecurr);
> 
> Exactly yes.
> 
> (though I still suggest a capture group to show the function like below)
> 
> 		if ($line =~ /\b($rcu_trace_funcs)\s*\(/ &&
> 		    $realfile !~ m{^$rcu_trace_paths}) {
> 			WARN("RCU_TASKS_TRACE",
> 			     "use of RCU task trace '$1' is incorrect outside BPF or core RCU code\n" . $herecurr);
> 		}

That does seem to work!

I will fold this change in on my next rebase.

							Thanx, Paul

[PATCH rcu 6/5] rcu-tasks: Permit use of debug-objects with RCU Tasks flavors

[Paul E. McKenney]

rcu-tasks: Permit use of debug-objects with RCU Tasks flavors

Currently, cblist_init_generic() holds a raw spinlock when invoking
INIT_WORK().  This fails in kernels built with CONFIG_DEBUG_OBJECTS=y
due to memory allocation being forbidden while holding a raw spinlock.
But the only reason for holding the raw spinlock is to synchronize
with early boot calls to call_rcu_tasks(), call_rcu_tasks_rude, and,
last but not least, call_rcu_tasks_trace().  These calls also invoke
cblist_init_generic() in order to support early boot queueing of
callbacks.

Except that there are no early boot calls to either of these three
functions, and the BPF guys confirm that they have no plans to add any
such calls.

This commit therefore removes the synchronization and adds a
WARN_ON_ONCE() to catch the case of now-prohibited early boot RCU Tasks
callback queueing.

If early boot queueing is needed, an "initialized" flag may be added to
the rcu_tasks structure.  Then queueing a callback before this flag is set
would initialize the callback list (if needed) and queue the callback.
The decision as to where to queue the callback given the possibility of
non-zero boot CPUs is left as an exercise for the reader.

Reported-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Paul E. McKenney <paulmck@kernel.org>

diff --git a/kernel/rcu/tasks.h b/kernel/rcu/tasks.h
index 291d536b1789..a4cd4ea2402c 100644
--- a/kernel/rcu/tasks.h
+++ b/kernel/rcu/tasks.h
@@ -236,7 +236,7 @@ static const char *tasks_gp_state_getname(struct rcu_tasks *rtp)
 #endif /* #ifndef CONFIG_TINY_RCU */
 
 // Initialize per-CPU callback lists for the specified flavor of
-// Tasks RCU.
+// Tasks RCU.  Do not enqueue callbacks before this function is invoked.
 static void cblist_init_generic(struct rcu_tasks *rtp)
 {
 	int cpu;
@@ -244,7 +244,6 @@ static void cblist_init_generic(struct rcu_tasks *rtp)
 	int lim;
 	int shift;
 
-	raw_spin_lock_irqsave(&rtp->cbs_gbl_lock, flags);
 	if (rcu_task_enqueue_lim < 0) {
 		rcu_task_enqueue_lim = 1;
 		rcu_task_cb_adjust = true;
@@ -267,17 +266,16 @@ static void cblist_init_generic(struct rcu_tasks *rtp)
 		WARN_ON_ONCE(!rtpcp);
 		if (cpu)
 			raw_spin_lock_init(&ACCESS_PRIVATE(rtpcp, lock));
-		raw_spin_lock_rcu_node(rtpcp); // irqs already disabled.
+		local_irq_save(flags);  // serialize initialization
 		if (rcu_segcblist_empty(&rtpcp->cblist))
 			rcu_segcblist_init(&rtpcp->cblist);
+		local_irq_restore(flags);
 		INIT_WORK(&rtpcp->rtp_work, rcu_tasks_invoke_cbs_wq);
 		rtpcp->cpu = cpu;
 		rtpcp->rtpp = rtp;
 		if (!rtpcp->rtp_blkd_tasks.next)
 			INIT_LIST_HEAD(&rtpcp->rtp_blkd_tasks);
-		raw_spin_unlock_rcu_node(rtpcp); // irqs remain disabled.
 	}
-	raw_spin_unlock_irqrestore(&rtp->cbs_gbl_lock, flags);
 
 	pr_info("%s: Setting shift to %d and lim to %d rcu_task_cb_adjust=%d.\n", rtp->name,
 			data_race(rtp->percpu_enqueue_shift), data_race(rtp->percpu_enqueue_lim), rcu_task_cb_adjust);
@@ -351,11 +349,9 @@ static void call_rcu_tasks_generic(struct rcu_head *rhp, rcu_callback_t func,
 		    READ_ONCE(rtp->percpu_enqueue_lim) != nr_cpu_ids)
 			needadjust = true;  // Defer adjustment to avoid deadlock.
 	}
-	if (!rcu_segcblist_is_enabled(&rtpcp->cblist)) {
-		raw_spin_unlock_rcu_node(rtpcp); // irqs remain disabled.
-		cblist_init_generic(rtp);
-		raw_spin_lock_rcu_node(rtpcp); // irqs already disabled.
-	}
+	// Queuing callbacks before initialization not yet supported.
+	if (WARN_ON_ONCE(!rcu_segcblist_is_enabled(&rtpcp->cblist)))
+		rcu_segcblist_init(&rtpcp->cblist);
 	needwake = (func == wakeme_after_rcu) ||
 		   (rcu_segcblist_n_cbs(&rtpcp->cblist) == rcu_task_lazy_lim);
 	if (havekthread && !needwake && !timer_pending(&rtpcp->lazy_timer)) {