* [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace [not found] <a6fff63c-5930-4918-82a3-a9301309d88d@paulmck-laptop> @ 2023-07-17 18:04 ` Paul E. McKenney 2023-07-17 22:34 ` Joe Perches 0 siblings, 1 reply; 12+ messages in thread From: Paul E. McKenney @ 2023-07-17 18:04 UTC (permalink / raw) To: rcu Cc: linux-kernel, kernel-team, rostedt, Paul E. McKenney, Andy Whitcroft, Joe Perches, Dwaipayan Ray, Lukas Bulwahn, Alexei Starovoitov, Daniel Borkmann, John Fastabend, bpf 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace 2023-07-17 18:04 ` [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace Paul E. McKenney @ 2023-07-17 22:34 ` Joe Perches 2023-07-17 23:34 ` Paul E. McKenney 0 siblings, 1 reply; 12+ messages in thread From: Joe Perches @ 2023-07-17 22:34 UTC (permalink / raw) To: Paul E. McKenney, rcu Cc: linux-kernel, kernel-team, rostedt, Andy Whitcroft, Dwaipayan Ray, Lukas Bulwahn, Alexei Starovoitov, Daniel Borkmann, John Fastabend, bpf 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); } } ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace 2023-07-17 22:34 ` Joe Perches @ 2023-07-17 23:34 ` Paul E. McKenney 2023-07-19 11:51 ` Joel Fernandes 2023-07-20 7:29 ` Joe Perches 0 siblings, 2 replies; 12+ messages in thread From: Paul E. McKenney @ 2023-07-17 23:34 UTC (permalink / raw) To: Joe Perches Cc: rcu, linux-kernel, kernel-team, rostedt, Andy Whitcroft, Dwaipayan Ray, Lukas Bulwahn, Alexei Starovoitov, Daniel Borkmann, John Fastabend, bpf 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace 2023-07-17 23:34 ` Paul E. McKenney @ 2023-07-19 11:51 ` Joel Fernandes 2023-07-19 18:27 ` Paul E. McKenney 2023-07-20 7:29 ` Joe Perches 1 sibling, 1 reply; 12+ messages in thread From: Joel Fernandes @ 2023-07-19 11:51 UTC (permalink / raw) To: paulmck, Joe Perches Cc: rcu, linux-kernel, kernel-team, rostedt, Andy Whitcroft, Dwaipayan Ray, Lukas Bulwahn, Alexei Starovoitov, Daniel Borkmann, John Fastabend, bpf 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace 2023-07-19 11:51 ` Joel Fernandes @ 2023-07-19 18:27 ` Paul E. McKenney 2023-07-19 18:44 ` Joe Perches 0 siblings, 1 reply; 12+ messages in thread From: Paul E. McKenney @ 2023-07-19 18:27 UTC (permalink / raw) To: Joel Fernandes Cc: Joe Perches, rcu, linux-kernel, kernel-team, rostedt, Andy Whitcroft, Dwaipayan Ray, Lukas Bulwahn, Alexei Starovoitov, Daniel Borkmann, John Fastabend, bpf 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace 2023-07-19 18:27 ` Paul E. McKenney @ 2023-07-19 18:44 ` Joe Perches 2023-07-19 19:02 ` Paul E. McKenney 0 siblings, 1 reply; 12+ messages in thread From: Joe Perches @ 2023-07-19 18:44 UTC (permalink / raw) To: paulmck, Joel Fernandes Cc: rcu, linux-kernel, kernel-team, rostedt, Andy Whitcroft, Dwaipayan Ray, Lukas Bulwahn, Alexei Starovoitov, Daniel Borkmann, John Fastabend, bpf 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace 2023-07-19 18:44 ` Joe Perches @ 2023-07-19 19:02 ` Paul E. McKenney 0 siblings, 0 replies; 12+ messages in thread From: Paul E. McKenney @ 2023-07-19 19:02 UTC (permalink / raw) To: Joe Perches Cc: Joel Fernandes, rcu, linux-kernel, kernel-team, rostedt, Andy Whitcroft, Dwaipayan Ray, Lukas Bulwahn, Alexei Starovoitov, Daniel Borkmann, John Fastabend, bpf 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace 2023-07-17 23:34 ` Paul E. McKenney 2023-07-19 11:51 ` Joel Fernandes @ 2023-07-20 7:29 ` Joe Perches 2023-07-20 19:43 ` Paul E. McKenney 1 sibling, 1 reply; 12+ messages in thread From: Joe Perches @ 2023-07-20 7:29 UTC (permalink / raw) To: paulmck Cc: rcu, linux-kernel, kernel-team, rostedt, Andy Whitcroft, Dwaipayan Ray, Lukas Bulwahn, Alexei Starovoitov, Daniel Borkmann, John Fastabend, bpf 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace 2023-07-20 7:29 ` Joe Perches @ 2023-07-20 19:43 ` Paul E. McKenney 2023-07-21 3:56 ` Paul E. McKenney 0 siblings, 1 reply; 12+ messages in thread From: Paul E. McKenney @ 2023-07-20 19:43 UTC (permalink / raw) To: Joe Perches Cc: rcu, linux-kernel, kernel-team, rostedt, Andy Whitcroft, Dwaipayan Ray, Lukas Bulwahn, Alexei Starovoitov, Daniel Borkmann, John Fastabend, bpf 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace 2023-07-20 19:43 ` Paul E. McKenney @ 2023-07-21 3:56 ` Paul E. McKenney 2023-07-21 4:38 ` Joe Perches 0 siblings, 1 reply; 12+ messages in thread From: Paul E. McKenney @ 2023-07-21 3:56 UTC (permalink / raw) To: Joe Perches Cc: rcu, linux-kernel, kernel-team, rostedt, Andy Whitcroft, Dwaipayan Ray, Lukas Bulwahn, Alexei Starovoitov, Daniel Borkmann, John Fastabend, bpf 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*\)/ ) { ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace 2023-07-21 3:56 ` Paul E. McKenney @ 2023-07-21 4:38 ` Joe Perches 2023-07-21 22:34 ` Paul E. McKenney 0 siblings, 1 reply; 12+ messages in thread From: Joe Perches @ 2023-07-21 4:38 UTC (permalink / raw) To: paulmck Cc: rcu, linux-kernel, kernel-team, rostedt, Andy Whitcroft, Dwaipayan Ray, Lukas Bulwahn, Alexei Starovoitov, Daniel Borkmann, John Fastabend, bpf 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); } ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace 2023-07-21 4:38 ` Joe Perches @ 2023-07-21 22:34 ` Paul E. McKenney 0 siblings, 0 replies; 12+ messages in thread From: Paul E. McKenney @ 2023-07-21 22:34 UTC (permalink / raw) To: Joe Perches Cc: rcu, linux-kernel, kernel-team, rostedt, Andy Whitcroft, Dwaipayan Ray, Lukas Bulwahn, Alexei Starovoitov, Daniel Borkmann, John Fastabend, bpf 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2023-07-21 22:34 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <a6fff63c-5930-4918-82a3-a9301309d88d@paulmck-laptop>
2023-07-17 18:04 ` [PATCH rcu 5/5] checkpatch: Complain about unexpected uses of RCU Tasks Trace Paul E. McKenney
2023-07-17 22:34 ` Joe Perches
2023-07-17 23:34 ` Paul E. McKenney
2023-07-19 11:51 ` Joel Fernandes
2023-07-19 18:27 ` Paul E. McKenney
2023-07-19 18:44 ` Joe Perches
2023-07-19 19:02 ` Paul E. McKenney
2023-07-20 7:29 ` Joe Perches
2023-07-20 19:43 ` Paul E. McKenney
2023-07-21 3:56 ` Paul E. McKenney
2023-07-21 4:38 ` Joe Perches
2023-07-21 22:34 ` Paul E. McKenney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox