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 <[email protected]> (maintainer:CHECKPATCH)
Cc: Joe Perches <[email protected]> (maintainer:CHECKPATCH)
Cc: Dwaipayan Ray <[email protected]> (reviewer:CHECKPATCH)
Cc: Lukas Bulwahn <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: John Fastabend <[email protected]>
Cc: <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
---
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
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 <[email protected]> (maintainer:CHECKPATCH)
> Cc: Joe Perches <[email protected]> (maintainer:CHECKPATCH)
> Cc: Dwaipayan Ray <[email protected]> (reviewer:CHECKPATCH)
> Cc: Lukas Bulwahn <[email protected]>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Daniel Borkmann <[email protected]>
> Cc: John Fastabend <[email protected]>
> Cc: <[email protected]>
> Signed-off-by: Paul E. McKenney <[email protected]>
> ---
> 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); }
}
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 <[email protected]> (maintainer:CHECKPATCH)
> > Cc: Joe Perches <[email protected]> (maintainer:CHECKPATCH)
> > Cc: Dwaipayan Ray <[email protected]> (reviewer:CHECKPATCH)
> > Cc: Lukas Bulwahn <[email protected]>
> > Cc: Alexei Starovoitov <[email protected]>
> > Cc: Daniel Borkmann <[email protected]>
> > Cc: John Fastabend <[email protected]>
> > Cc: <[email protected]>
> > Signed-off-by: Paul E. McKenney <[email protected]>
> > ---
> > 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
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 <[email protected]> (maintainer:CHECKPATCH)
>>> Cc: Joe Perches <[email protected]> (maintainer:CHECKPATCH)
>>> Cc: Dwaipayan Ray <[email protected]> (reviewer:CHECKPATCH)
>>> Cc: Lukas Bulwahn <[email protected]>
>>> Cc: Alexei Starovoitov <[email protected]>
>>> Cc: Daniel Borkmann <[email protected]>
>>> Cc: John Fastabend <[email protected]>
>>> Cc: <[email protected]>
>>> Signed-off-by: Paul E. McKenney <[email protected]>
>>> ---
>>> 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
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 <[email protected]> (maintainer:CHECKPATCH)
> > > > Cc: Joe Perches <[email protected]> (maintainer:CHECKPATCH)
> > > > Cc: Dwaipayan Ray <[email protected]> (reviewer:CHECKPATCH)
> > > > Cc: Lukas Bulwahn <[email protected]>
> > > > Cc: Alexei Starovoitov <[email protected]>
> > > > Cc: Daniel Borkmann <[email protected]>
> > > > Cc: John Fastabend <[email protected]>
> > > > Cc: <[email protected]>
> > > > Signed-off-by: Paul E. McKenney <[email protected]>
> > > > ---
> > > > 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
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
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.
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 <[email protected]> (maintainer:CHECKPATCH)
> > > Cc: Joe Perches <[email protected]> (maintainer:CHECKPATCH)
> > > Cc: Dwaipayan Ray <[email protected]> (reviewer:CHECKPATCH)
> > > Cc: Lukas Bulwahn <[email protected]>
> > > Cc: Alexei Starovoitov <[email protected]>
> > > Cc: Daniel Borkmann <[email protected]>
> > > Cc: John Fastabend <[email protected]>
> > > Cc: <[email protected]>
> > > Signed-off-by: Paul E. McKenney <[email protected]>
> > > ---
> > > 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
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 <[email protected]> (maintainer:CHECKPATCH)
> > > > Cc: Joe Perches <[email protected]> (maintainer:CHECKPATCH)
> > > > Cc: Dwaipayan Ray <[email protected]> (reviewer:CHECKPATCH)
> > > > Cc: Lukas Bulwahn <[email protected]>
> > > > Cc: Alexei Starovoitov <[email protected]>
> > > > Cc: Daniel Borkmann <[email protected]>
> > > > Cc: John Fastabend <[email protected]>
> > > > Cc: <[email protected]>
> > > > Signed-off-by: Paul E. McKenney <[email protected]>
> > > > ---
> > > > 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
>
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 <[email protected]> (maintainer:CHECKPATCH)
> > > > > Cc: Joe Perches <[email protected]> (maintainer:CHECKPATCH)
> > > > > Cc: Dwaipayan Ray <[email protected]> (reviewer:CHECKPATCH)
> > > > > Cc: Lukas Bulwahn <[email protected]>
> > > > > Cc: Alexei Starovoitov <[email protected]>
> > > > > Cc: Daniel Borkmann <[email protected]>
> > > > > Cc: John Fastabend <[email protected]>
> > > > > Cc: <[email protected]>
> > > > > Signed-off-by: Paul E. McKenney <[email protected]>
> > > > > ---
> > > > > 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 <[email protected]>
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 <[email protected]> (maintainer:CHECKPATCH)
Cc: Joe Perches <[email protected]> (maintainer:CHECKPATCH)
Cc: Dwaipayan Ray <[email protected]> (reviewer:CHECKPATCH)
Cc: Lukas Bulwahn <[email protected]>
Cc: Alexei Starovoitov <[email protected]>
Cc: Daniel Borkmann <[email protected]>
Cc: John Fastabend <[email protected]>
Cc: <[email protected]>
Signed-off-by: Paul E. McKenney <[email protected]>
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*\)/ ) {
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 <[email protected]>
> 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);
}
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 <[email protected]>
> > 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