[cocci] possible regressions in 1.1.1 -> 1.2

[cocci] possible regressions in 1.1.1 -> 1.2

[Johannes Berg]

Hi,

We have an spatch (fragment) like this:

@@
identifier wiphy, dev, peer, link_id, action_code, dialog_token, status_code, peer_capability, initiator, extra_ies, extra_ies_len;
type u8;
@@
int ieee80211_tdls_mgmt(struct wiphy *wiphy, struct net_device *dev, const u8 *peer,
-int link_id,
u8 action_code, u8 dialog_token, u16 status_code,
u32 peer_capability, bool initiator,
const u8 *extra_ies, size_t extra_ies_len);


that should remove the link_id argument from the function declaration in
the Linux kernel in net/mac80211/ieee80211_i.h (for some backport
purposes, [1]).

With "spatch version 1.1.1 compiled with OCaml version 4.14.2" this
works just fine; however, we're trying to upgrade our dependencies and
now with "spatch version 1.2 compiled with OCaml version 5.2.0" it
doesn't match anything any more? I can't seem to figure out why, I
thought maybe the types didn't match - not sure why we had added type u8
to that, but pretty sure it was needed - but adding more types didn't
really help either.



There's a second issue: we have this
spatch: https://p.sipsolutions.net/765e55f2b951326c.txt
It's not pretty, but it's intended to (again for backport reasons)
manipulate some code in net/mac80211/cfg.c (see also [1]). We run all
our spatches with --include-headers, and this one when running on
net/mac80211/ieee80211_i.h, goes into some kind of infinite loop? I.e.
just

spatch --include-headers --in-place --sp-file /tmp/765e55f2b951326c.txt net/mac80211/ieee80211_i.h

never finishes?


Any thoughts?

Thanks,
johannes


[1] there's another fragment that removes the variable from the function
but I think net/mac80211/cfg.c really only gets parsed properly when we
first patch out the CFG80211_TESTMODE_CMD/CFG80211_TESTMODE_DUMP lines
from the struct cfg80211_ops mac80211_config_ops declaration. Anyway,
the above is already an issue.

Re: [cocci] possible regressions in 1.1.1 -> 1.2

[Johannes Berg]

On Thu, 2025-02-06 at 10:31 +0100, Johannes Berg wrote:
> 
> There's a second issue: we have this
> spatch: https://p.sipsolutions.net/765e55f2b951326c.txt
> It's not pretty, but it's intended to (again for backport reasons)
> manipulate some code in net/mac80211/cfg.c (see also [1]). We run all
> our spatches with --include-headers, and this one when running on
> net/mac80211/ieee80211_i.h, goes into some kind of infinite loop? I.e.
> just
> 
> spatch --include-headers --in-place --sp-file /tmp/765e55f2b951326c.txt net/mac80211/ieee80211_i.h
> 
> never finishes?

I stand corrected, I wasn't patient enough. On a machine _much_ faster
than my laptop (server CPU), it finished (with warnings about
inconsistent modifications) after a bit more than 10 minutes.

johannes

Re: [cocci] possible regressions in 1.1.1 -> 1.2

[Victor Gambier]

Hi,

Thanks for the report!

> With "spatch version 1.1.1 compiled with OCaml version 4.14.2" this
> works just fine; however, we're trying to upgrade our dependencies and
> now with "spatch version 1.2 compiled with OCaml version 5.2.0" it
> doesn't match anything any more? I can't seem to figure out why, I
> thought maybe the types didn't match - not sure why we had added type u8
> to that, but pretty sure it was needed - but adding more types didn't
> really help either.
We don't officially support OCaml 5.2.0. Could you let me know how you 
installed/built Coccinelle?

> I stand corrected, I wasn't patient enough. On a machine _much_ faster
> than my laptop (server CPU), it finished (with warnings about
> inconsistent modifications) after a bit more than 10 minutes.

Glad to hear! We know there's been a performance regression in 1.2 and 
we are looking into it. It's a little faster in 1.3.0, but still slower 
than 1.1.1. It's unclear at the moment to what extent the performance 
has been degraded because of necessary fixes that perform unavoidable 
additional work.

Victor

Re: [cocci] possible regressions in 1.1.1 -> 1.2

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]



On Thu, 6 Feb 2025, Johannes Berg wrote:

> On Thu, 2025-02-06 at 10:31 +0100, Johannes Berg wrote:
> >
> > There's a second issue: we have this
> > spatch: https://p.sipsolutions.net/765e55f2b951326c.txt
> > It's not pretty, but it's intended to (again for backport reasons)
> > manipulate some code in net/mac80211/cfg.c (see also [1]). We run all
> > our spatches with --include-headers, and this one when running on
> > net/mac80211/ieee80211_i.h, goes into some kind of infinite loop? I.e.
> > just
> >
> > spatch --include-headers --in-place --sp-file /tmp/765e55f2b951326c.txt net/mac80211/ieee80211_i.h
> >
> > never finishes?
>
> I stand corrected, I wasn't patient enough. On a machine _much_ faster
> than my laptop (server CPU), it finished (with warnings about
> inconsistent modifications) after a bit more than 10 minutes.

You can try --profile to find out what it is spending the most time on.

--show-trying shows if there is some function in the source code that is
particularly problematic.

These options might require also using --debug; I don't remember.

julia

Re: [cocci] possible regressions in 1.1.1 -> 1.2

[Johannes Berg]

On Thu, 2025-02-06 at 10:53 +0100, Victor Gambier wrote:
> > With "spatch version 1.1.1 compiled with OCaml version 4.14.2" this
> > works just fine; however, we're trying to upgrade our dependencies and
> > now with "spatch version 1.2 compiled with OCaml version 5.2.0" it
> > doesn't match anything any more? I can't seem to figure out why, I
> > thought maybe the types didn't match - not sure why we had added type u8
> > to that, but pretty sure it was needed - but adding more types didn't
> > really help either.
> We don't officially support OCaml 5.2.0. Could you let me know how you 
> installed/built Coccinelle?

Heh, funny. I didn't even really think about it; it's the nix package
from their 2024.11 release:
https://github.com/NixOS/nixpkgs/blob/nixos-24.11/pkgs/development/tools/misc/coccinelle/default.nix

> > I stand corrected, I wasn't patient enough. On a machine _much_ faster
> > than my laptop (server CPU), it finished (with warnings about
> > inconsistent modifications) after a bit more than 10 minutes.
> 
> Glad to hear! We know there's been a performance regression in 1.2 and 
> we are looking into it. It's a little faster in 1.3.0, but still slower 
> than 1.1.1. It's unclear at the moment to what extent the performance 
> has been degraded because of necessary fixes that perform unavoidable 
> additional work.

OK. I guess we'll have to stick to 1.1.1 for the time being, it does
what we need, we check the output, and this probably more than triples
the overall runtime :)

johannes

Re: [cocci] possible regressions in 1.1.1 -> 1.2

[Johannes Berg]

On Thu, 2025-02-06 at 10:59 +0100, Julia Lawall wrote:
> 
> You can try --profile to find out what it is spending the most time on.
> 
> --show-trying shows if there is some function in the source code that is
> particularly problematic.
> 
> These options might require also using --debug; I don't remember.

Interesting. It says:

> [...]
> rule starting on line 86 = 
> -----------------------------------------------------------------------
> dependencies for rule rule starting on line 86 satisfied:

which is indeed the rule that modifies a function in the header file, an
inline:

@@
@@
static inline int
ieee80211_get_mbssid_beacon_len(...)
{
...
-if (rnr_elems) {...}
...
}

That seems ... simple enough?

However, the debug log continues:

> [...]
>      trying  something else
>      trying function: vif_to_sdata: ieee80211_i.h:1215
>      trying  something else
>      trying function: ieee80211_get_mbssid_beacon_len: ieee80211_i.h:1237

which makes sense, that's where the function is, but now it's just
"stuck". I guess it'll finish eventually, but that seems ... strange?
That spatch fragment doesn't even seem so bad, least compared to other
things?

Isolating just that spatch above results in the same. Looks like the
profile isn't very good when I abort it, so I'll let it run and report
back what it says later.

johannes

Re: [cocci] possible regressions in 1.1.1 -> 1.2

[Johannes Berg]

On Thu, 2025-02-06 at 11:07 +0100, Johannes Berg wrote:
> @@
> @@
> static inline int
> ieee80211_get_mbssid_beacon_len(...)
> {
> ...
> -if (rnr_elems) {...}
> ...
> }
> 
> That seems ... simple enough?
> 

Oh. I notice that the function actually contains *two* blocks that could
match "if (rnr_elems) {...}", one is nested in a loop.

The older version of spatch _also_ takes a very long time on this, just
not quite _as_ long (about 1 minute vs. 10)

They do seem to remove *both* instances, however, modifying the spatch
to be

@@
@@
static inline int
ieee80211_get_mbssid_beacon_len(...)
{
<+...
-if (rnr_elems) {...}
...+>
}


makes both versions process it pretty much instantaneously. :)

johannes

Re: [cocci] possible regressions in 1.1.1 -> 1.2

[Julia Lawall]

On Thu, 6 Feb 2025, Johannes Berg wrote:

> On Thu, 2025-02-06 at 10:59 +0100, Julia Lawall wrote:
> >
> > You can try --profile to find out what it is spending the most time on.
> >
> > --show-trying shows if there is some function in the source code that is
> > particularly problematic.
> >
> > These options might require also using --debug; I don't remember.
>
> Interesting. It says:
>
> > [...]
> > rule starting on line 86 =
> > -----------------------------------------------------------------------
> > dependencies for rule rule starting on line 86 satisfied:
>
> which is indeed the rule that modifies a function in the header file, an
> inline:
>
> @@
> @@
> static inline int
> ieee80211_get_mbssid_beacon_len(...)
> {
> ...
> -if (rnr_elems) {...}
> ...
> }
>
> That seems ... simple enough?

This looks like it could be related to the bug that was corrected and that
leads to the performance problem (ie the old implementation was faster
because it was incorrect).

Is it essential to verify that there is exactly one such if on every
control-flow path?

If You just want to remove all tests on rnr_elems, it will probably be
much faster to do:

 @@
 @@
 static inline int
 ieee80211_get_mbssid_beacon_len(...)
 {
 <...
 -if (rnr_elems) {...}
 ...>
 }

julia

>
> However, the debug log continues:
>
> > [...]
> >      trying  something else
> >      trying function: vif_to_sdata: ieee80211_i.h:1215
> >      trying  something else
> >      trying function: ieee80211_get_mbssid_beacon_len: ieee80211_i.h:1237
>
> which makes sense, that's where the function is, but now it's just
> "stuck". I guess it'll finish eventually, but that seems ... strange?
> That spatch fragment doesn't even seem so bad, least compared to other
> things?
>
> Isolating just that spatch above results in the same. Looks like the
> profile isn't very good when I abort it, so I'll let it run and report
> back what it says later.
>
> johannes
>

Re: [cocci] possible regressions in 1.1.1 -> 1.2

[Johannes Berg]

On Thu, 2025-02-06 at 11:13 +0100, Julia Lawall wrote:
> 
> If You just want to remove all tests on rnr_elems, it will probably be
> much faster to do:
> 
>  @@
>  @@
>  static inline int
>  ieee80211_get_mbssid_beacon_len(...)
>  {
>  <...
>  -if (rnr_elems) {...}
>  ...>
>  }
> 

Right. I don't even care about <+ or < because we check the resulting
output anyway :)

Thanks!

Now maybe I can also figure out something why the TDLS thing doesn't
match, but so far hitting a wall on it :)

johannes

Re: [cocci] possible regressions in 1.1.1 -> 1.2

[Markus Elfring]

>> I stand corrected, I wasn't patient enough. On a machine _much_ faster
>> than my laptop (server CPU), it finished (with warnings about
>> inconsistent modifications) after a bit more than 10 minutes.
>
> Glad to hear! We know there's been a performance regression in 1.2 and we are looking into it. It's a little faster in 1.3.0, but still slower than 1.1.1. It's unclear at the moment to what extent the performance has been degraded because of necessary fixes that perform unavoidable additional work.
Thanks for such background information.

How do you think about to improve measurement approaches
for questionable software run time characteristics?

Regards,
Markus

Re: [cocci] possible regressions in 1.1.1 -> 1.2

[Markus Elfring]

> This looks like it could be related to the bug that was corrected and that
> leads to the performance problem (ie the old implementation was faster
> because it was incorrect).

Would you like to become more precise about technical details
which influenced software run time characteristics in special ways?

Regards,
Markus

Re: [cocci] possible regressions in 1.1.1 -> 1.2

[Markus Elfring]

> They do seem to remove *both* instances, however, modifying the spatch
> to be
>
> @@
> @@
> static inline int
> ieee80211_get_mbssid_beacon_len(...)
> {
> <+...
> -if (rnr_elems) {...}
> ...+>
> }
>
>
> makes both versions process it pretty much instantaneously. :)
Would you like to reconsider source code transformation approaches
any more also with your SmPL script variations?

See also:
Get the non-optional nest construct completely working for SmPL
2017-08-18
https://github.com/coccinelle/coccinelle/issues/114

Regards,
Markus

Re: [cocci] possible regressions in 1.1.1 -> 1.2

[Markus Elfring]

> I don't even care about <+ or < because we check the resulting output anyway :)

I would appreciate further clarification for such information.

I tend to care more for the proper selection and support
of dot variants according to efficient applications of scripts
for the semantic patch language.

Regards,
Markus

Re: [cocci] possible regressions in 1.1.1 -> 1.2

[Markus Elfring]

> @@
> @@
> static inline int
> ieee80211_get_mbssid_beacon_len(...)
> {
> ...
> -if (rnr_elems) {...}
> ...
> }
>
> That seems ... simple enough?
The mentioned function implementation can be extracted into another test file.
https://elixir.bootlin.com/linux/v6.13.1/source/net/mac80211/ieee80211_i.h#L1210-L1240

I observed with the software combination “Coccinelle 1.3” (OCaml 5.1.1)
that the program parameter “--no-loops” can also trigger a very significant run time difference
for such a test case.

It seems that the application of the SmPL construct “<+... … ...+>” can eventually be more desirable
for this use case.

Regards,
Markus

Re: [cocci] Checking software run time characteristics

[Markus Elfring]

> You can try --profile to find out what it is spending the most time on.
Would you like to clarify measurement data (like the following) any further?

Markus_Elfring@Sonne:…/Projekte/Coccinelle/Probe> time spatch --profile delete_statement1-Johannes_Berg.cocci extract-ieee80211_get_mbssid_beacon_len-20250206.h
…
diff =
warning: rule starting on line 4, node 52: [fortrue][1,2,17,18,19] in ieee80211_get_mbssid_beacon_len may be inconsistently modified
warning: rule starting on line 4, node 55: statement[1,2,17,18,19,20] in ieee80211_get_mbssid_beacon_len may be inconsistently modified
warning: rule starting on line 4, node 30: [fortrue][1,2,6,7,9,10,12] in ieee80211_get_mbssid_beacon_len may be inconsistently modified
warning: rule starting on line 4, node 33: statement[1,2,6,7,9,10,12,13] in ieee80211_get_mbssid_beacon_len may be inconsistently modified
warning: rule starting on line 4, node 51: for[1,2,17,18,19]* in ieee80211_get_mbssid_beacon_len may be inconsistently modified
warning: rule starting on line 4, node 29: for[1,2,6,7,9,10,12]* in ieee80211_get_mbssid_beacon_len may be inconsistently modified
…
Note: processing took  1083.7s: extract-ieee80211_get_mbssid_beacon_len-20250206.h
---------------------
profiling result
---------------------
Main total                               : 1083.707616 sec          1 count
Main.outfiles computation                : 1083.691804 sec          1 count
full_engine                              : 1083.691654 sec          1 count
bigloop                                  : 1083.679190 sec          1 count
Rule rule starting on line 4             : 1083.679188 sec          1 count
process_a_ctl_a_env_a_toplevel           : 1083.676273 sec          1 count
mysat                                    : 1083.676131 sec          1 count
ctl                                      : 1083.674814 sec         35 count
pre_engine                               : 0.015786 sec          1 count
parse cocci                              : 0.015455 sec          1 count
show_xxx                                 : 0.010977 sec         11 count
HACK                                     : 0.010574 sec          5 count
Common.full_charpos_to_pos               : 0.002770 sec          2 count
C unparsing                              : 0.001845 sec          2 count
Parsing: 1st pass                        : 0.000956 sec          8 count
YACC                                     : 0.000933 sec          8 count
C parsing.fix_define                     : 0.000724 sec          5 count
C parsing.lex_ident                      : 0.000611 sec       1418 count
LEXING                                   : 0.000462 sec          4 count
flow                                     : 0.000359 sec          4 count
C parsing.fix_cpp                        : 0.000349 sec          4 count
MACRO mgmt prep 1                        : 0.000301 sec          4 count
asttoctl2                                : 0.000281 sec          1 count
TAC.annotate_program                     : 0.000257 sec          2 count
C parsing.lookahead                      : 0.000182 sec        546 count
C consistencycheck                       : 0.000140 sec          4 count
Transformation3.transform                : 0.000127 sec          1 count
Type_c.type_of_s                         : 0.000085 sec         23 count
Common.info_from_charpos                 : 0.000070 sec          2 count
worth_trying                             : 0.000058 sec          1 count
fix_flow                                 : 0.000051 sec          2 count
C parsing.mk_info_item                   : 0.000050 sec          8 count
Common.=~                                : 0.000043 sec          6 count
MACRO mgmt prep 2                        : 0.000017 sec          4 count
Main.result analysis                     : 0.000010 sec          1 count
TAC.typedef_fix                          : 0.000009 sec         58 count
TAC.add_binding                          : 0.000008 sec         10 count
C parsing                                : 0.000006 sec          4 count
check_duplicate                          : 0.000006 sec          1 count
post_engine                              : 0.000006 sec          1 count
Main.infiles computation                 : 0.000001 sec          1 count
TOTAL                                    : 0.000000 sec          4 count


real    18m3,734s
user    18m1,192s
sys     0m1,021s


Regards,
Markus

Re: [cocci] Checking software run time characteristics

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 4774 bytes --]



On Fri, 7 Feb 2025, Markus Elfring wrote:

> > You can try --profile to find out what it is spending the most time on.
> Would you like to clarify measurement data (like the following) any further?

It seems that the rule starting on line 4 is very slow?

Maybe that is the only rule in the semantic patch and the information is
not very helpful.

julia

>
> Markus_Elfring@Sonne:…/Projekte/Coccinelle/Probe> time spatch --profile delete_statement1-Johannes_Berg.cocci extract-ieee80211_get_mbssid_beacon_len-20250206.h
> …
> diff =
> warning: rule starting on line 4, node 52: [fortrue][1,2,17,18,19] in ieee80211_get_mbssid_beacon_len may be inconsistently modified
> warning: rule starting on line 4, node 55: statement[1,2,17,18,19,20] in ieee80211_get_mbssid_beacon_len may be inconsistently modified
> warning: rule starting on line 4, node 30: [fortrue][1,2,6,7,9,10,12] in ieee80211_get_mbssid_beacon_len may be inconsistently modified
> warning: rule starting on line 4, node 33: statement[1,2,6,7,9,10,12,13] in ieee80211_get_mbssid_beacon_len may be inconsistently modified
> warning: rule starting on line 4, node 51: for[1,2,17,18,19]* in ieee80211_get_mbssid_beacon_len may be inconsistently modified
> warning: rule starting on line 4, node 29: for[1,2,6,7,9,10,12]* in ieee80211_get_mbssid_beacon_len may be inconsistently modified
> …
> Note: processing took  1083.7s: extract-ieee80211_get_mbssid_beacon_len-20250206.h
> ---------------------
> profiling result
> ---------------------
> Main total                               : 1083.707616 sec          1 count
> Main.outfiles computation                : 1083.691804 sec          1 count
> full_engine                              : 1083.691654 sec          1 count
> bigloop                                  : 1083.679190 sec          1 count
> Rule rule starting on line 4             : 1083.679188 sec          1 count
> process_a_ctl_a_env_a_toplevel           : 1083.676273 sec          1 count
> mysat                                    : 1083.676131 sec          1 count
> ctl                                      : 1083.674814 sec         35 count
> pre_engine                               : 0.015786 sec          1 count
> parse cocci                              : 0.015455 sec          1 count
> show_xxx                                 : 0.010977 sec         11 count
> HACK                                     : 0.010574 sec          5 count
> Common.full_charpos_to_pos               : 0.002770 sec          2 count
> C unparsing                              : 0.001845 sec          2 count
> Parsing: 1st pass                        : 0.000956 sec          8 count
> YACC                                     : 0.000933 sec          8 count
> C parsing.fix_define                     : 0.000724 sec          5 count
> C parsing.lex_ident                      : 0.000611 sec       1418 count
> LEXING                                   : 0.000462 sec          4 count
> flow                                     : 0.000359 sec          4 count
> C parsing.fix_cpp                        : 0.000349 sec          4 count
> MACRO mgmt prep 1                        : 0.000301 sec          4 count
> asttoctl2                                : 0.000281 sec          1 count
> TAC.annotate_program                     : 0.000257 sec          2 count
> C parsing.lookahead                      : 0.000182 sec        546 count
> C consistencycheck                       : 0.000140 sec          4 count
> Transformation3.transform                : 0.000127 sec          1 count
> Type_c.type_of_s                         : 0.000085 sec         23 count
> Common.info_from_charpos                 : 0.000070 sec          2 count
> worth_trying                             : 0.000058 sec          1 count
> fix_flow                                 : 0.000051 sec          2 count
> C parsing.mk_info_item                   : 0.000050 sec          8 count
> Common.=~                                : 0.000043 sec          6 count
> MACRO mgmt prep 2                        : 0.000017 sec          4 count
> Main.result analysis                     : 0.000010 sec          1 count
> TAC.typedef_fix                          : 0.000009 sec         58 count
> TAC.add_binding                          : 0.000008 sec         10 count
> C parsing                                : 0.000006 sec          4 count
> check_duplicate                          : 0.000006 sec          1 count
> post_engine                              : 0.000006 sec          1 count
> Main.infiles computation                 : 0.000001 sec          1 count
> TOTAL                                    : 0.000000 sec          4 count
>
>
> real    18m3,734s
> user    18m1,192s
> sys     0m1,021s
>
>
> Regards,
> Markus
>

Re: [cocci] Checking software run time characteristics

[Markus Elfring]

>> Would you like to clarify measurement data (like the following) any further?
>
> It seems that the rule starting on line 4 is very slow?

The run time characteristics are obviously questionable.


> Maybe that is the only rule in the semantic patch

Yes.

Johannes Berg showed this test case yesterday.
https://lore.kernel.org/cocci/9063a6e6c83f9169456bc009cc35426889cc46e1.camel@sipsolutions.net/
https://sympa.inria.fr/sympa/arc/cocci/2025-02/msg00029.html


> and the information is not very helpful.
Will the measurement data granularity be increased anyhow for the affected function call hierarchy?

Why does the software run time “explode” (according to the handling of two simple for loops
within if branches) so far?
https://elixir.bootlin.com/linux/v6.13.1/source/net/mac80211/ieee80211_i.h#L1210-L1240

Can our discussion trigger any further software improvements?

Regards,
Markus

Re: [cocci] Checking software run time characteristics

[Johannes Berg]

On Fri, 2025-02-07 at 09:15 +0100, Markus Elfring wrote:
> > 
> 
> Can our discussion trigger any further software improvements?

You're kidding, right? Discussions with you specifically only ever waste
everyone's time.

johannes

Re: [cocci] Checking software run time characteristics

[Markus Elfring]

>> Can our discussion trigger any further software improvements?
>
> You're kidding, right?

No.


> Discussions with you specifically only ever waste everyone's time.
I wonder about this view.

Various open issues are waiting for solutions and desirable collateral evolution.

Regards,
Markus

Re: [cocci] Checking SmPL test cases?

[Markus Elfring]

> That seems ... simple enough?
Can you get further development ideas from test data like the following?

SmPL script example:
@deletion@
identifier i;
@@
 static inline int
 i(...)
 {
 ...
-if (rnr_elems) {...}
 ...
 }


Test result:
Markus_Elfring@Sonne:…/Projekte/Coccinelle/Probe> time spatch delete_statement3-Johannes_Berg.cocci test-beacon_len-20250206.c
…
diff =
warning: deletion, node 19: statement[1,2,4,5,7,8] in my_beacon_len may be inconsistently modified
warning: deletion, node 16: [fortrue][1,2,4,5,7] in my_beacon_len may be inconsistently modified
warning: deletion, node 15: for[1,2,4,5,7]* in my_beacon_len may be inconsistently modified
…
@@ -7,12 +7,5 @@ my_beacon_len(struct cfg80211_mbssid_ele
 {
        int len = 0;

-       if (rnr_elems) {
-               u8 i;
-
-               for (i = 0; i < rnr_elems->cnt; i++)
-                       len += rnr_elems->elem[i].len;
-       }
-
        return len;
 }

real    0m0,058s
user    0m0,041s
sys     0m0,016s


Another source file example:
static inline int
my_beacon_len(struct cfg80211_mbssid_elems *elems,
	      struct cfg80211_rnr_elems *rnr_elems,
	      u8 i)
{
	int len = 0;

	if (i < elems->cnt) {
		len = elems->elem[i].len;
		if (rnr_elems) {
			len += rnr_elems->elem[i].len;
			for (i = elems->cnt; i < rnr_elems->cnt; i++)
				len += rnr_elems->elem[i].len;
		}
		return len;
	}

	return len;
}


Questionable test result:
Markus_Elfring@Sonne:…/Projekte/Coccinelle/Probe> time spatch delete_statement3-Johannes_Berg.cocci test2-beacon_len-20250206.c
…
real    0m1,653s
user    0m1,635s
sys     0m0,017s


May I expect the generation of corresponding difference output here?

Regards,
Markus

Re: [cocci] Checking SmPL test cases?

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1927 bytes --]



On Sat, 8 Feb 2025, Markus Elfring wrote:

> > That seems ... simple enough?
> Can you get further development ideas from test data like the following?
>
> SmPL script example:
> @deletion@
> identifier i;
> @@
>  static inline int
>  i(...)
>  {
>  ...
> -if (rnr_elems) {...}
>  ...
>  }
>
>
> Test result:
> Markus_Elfring@Sonne:…/Projekte/Coccinelle/Probe> time spatch delete_statement3-Johannes_Berg.cocci test-beacon_len-20250206.c
> …
> diff =
> warning: deletion, node 19: statement[1,2,4,5,7,8] in my_beacon_len may be inconsistently modified
> warning: deletion, node 16: [fortrue][1,2,4,5,7] in my_beacon_len may be inconsistently modified
> warning: deletion, node 15: for[1,2,4,5,7]* in my_beacon_len may be inconsistently modified
> …
> @@ -7,12 +7,5 @@ my_beacon_len(struct cfg80211_mbssid_ele
>  {
>         int len = 0;
>
> -       if (rnr_elems) {
> -               u8 i;
> -
> -               for (i = 0; i < rnr_elems->cnt; i++)
> -                       len += rnr_elems->elem[i].len;
> -       }
> -
>         return len;
>  }
>
> real    0m0,058s
> user    0m0,041s
> sys     0m0,016s
>
>
> Another source file example:
> static inline int
> my_beacon_len(struct cfg80211_mbssid_elems *elems,
> 	      struct cfg80211_rnr_elems *rnr_elems,
> 	      u8 i)
> {
> 	int len = 0;
>
> 	if (i < elems->cnt) {
> 		len = elems->elem[i].len;
> 		if (rnr_elems) {
> 			len += rnr_elems->elem[i].len;
> 			for (i = elems->cnt; i < rnr_elems->cnt; i++)
> 				len += rnr_elems->elem[i].len;
> 		}
> 		return len;
> 	}
>
> 	return len;
> }
>
>
> Questionable test result:
> Markus_Elfring@Sonne:…/Projekte/Coccinelle/Probe> time spatch delete_statement3-Johannes_Berg.cocci test2-beacon_len-20250206.c
> …
> real    0m1,653s
> user    0m1,635s
> sys     0m0,017s
>
>
> May I expect the generation of corresponding difference output here?

No.  The matched if doesn't occur on every control-flow path.

julia

Re: [cocci] Checking SmPL test cases?

[Markus Elfring]

> The matched if doesn't occur on every control-flow path.
Is this also the case according to the source file/header example “extract-ieee80211_get_mbssid_beacon_len-20250206.h”
(despite of the discussed difference output)?
https://elixir.bootlin.com/linux/v6.13.1/source/net/mac80211/ieee80211_i.h#L1210-L1240

Regards,
Markus

Re: [cocci] Checking SmPL test cases?

[Markus Elfring]

> The matched if doesn't occur on every control-flow path.
Will further collateral evolution happen according to a test result like the following?

Markus_Elfring@Sonne:…/Projekte/Coccinelle/Probe> time spatch delete_statement3-Johannes_Berg.cocci test3-beacon_len-20250206.c
…
diff =
warning: deletion, node 22: for[1,2,4,5,7,8,10]* in my_beacon_len may be inconsistently modified
warning: deletion, node 23: [fortrue][1,2,4,5,7,8,10] in my_beacon_len may be inconsistently modified
warning: deletion, node 40: [fortrue][1,2,13,14,15] in my_beacon_len may be inconsistently modified
warning: deletion, node 26: statement[1,2,4,5,7,8,10,11] in my_beacon_len may be inconsistently modified
warning: deletion, node 39: for[1,2,13,14,15]* in my_beacon_len may be inconsistently modified
warning: deletion, node 43: statement[1,2,13,14,15,16] in my_beacon_len may be inconsistently modified
…
@@ -11,18 +11,8 @@ my_beacon_len(struct cfg80211_mbssid_ele

        if (i < elems->cnt) {
                len = elems->elem[i].len;
-               if (rnr_elems) {
-                       len += rnr_elems->elem[i].len;
-                       for (i = elems->cnt; i < rnr_elems->cnt; i++)
-                               len += rnr_elems->elem[i].len;
-               }
                return len;
        }

-       if (rnr_elems) {
-               for (i = 0; i < rnr_elems->cnt; i++)
-                       len += rnr_elems->elem[i].len;
-       }
-
        return len;
 }

real    0m2,146s
user    0m2,105s
sys     0m0,023s


Regards,
Markus

Re: [cocci] Checking SmPL test cases?

[Julia Lawall]

[-- Attachment #1: Type: text/plain, Size: 1724 bytes --]



On Sat, 8 Feb 2025, Markus Elfring wrote:

> > The matched if doesn't occur on every control-flow path.
> Will further collateral evolution happen according to a test result like the following?

Now the pattern occurs on every control-flow path.

julia

>
> Markus_Elfring@Sonne:…/Projekte/Coccinelle/Probe> time spatch delete_statement3-Johannes_Berg.cocci test3-beacon_len-20250206.c
> …
> diff =
> warning: deletion, node 22: for[1,2,4,5,7,8,10]* in my_beacon_len may be inconsistently modified
> warning: deletion, node 23: [fortrue][1,2,4,5,7,8,10] in my_beacon_len may be inconsistently modified
> warning: deletion, node 40: [fortrue][1,2,13,14,15] in my_beacon_len may be inconsistently modified
> warning: deletion, node 26: statement[1,2,4,5,7,8,10,11] in my_beacon_len may be inconsistently modified
> warning: deletion, node 39: for[1,2,13,14,15]* in my_beacon_len may be inconsistently modified
> warning: deletion, node 43: statement[1,2,13,14,15,16] in my_beacon_len may be inconsistently modified
> …
> @@ -11,18 +11,8 @@ my_beacon_len(struct cfg80211_mbssid_ele
>
>         if (i < elems->cnt) {
>                 len = elems->elem[i].len;
> -               if (rnr_elems) {
> -                       len += rnr_elems->elem[i].len;
> -                       for (i = elems->cnt; i < rnr_elems->cnt; i++)
> -                               len += rnr_elems->elem[i].len;
> -               }
>                 return len;
>         }
>
> -       if (rnr_elems) {
> -               for (i = 0; i < rnr_elems->cnt; i++)
> -                       len += rnr_elems->elem[i].len;
> -       }
> -
>         return len;
>  }
>
> real    0m2,146s
> user    0m2,105s
> sys     0m0,023s
>
>
> Regards,
> Markus
>

Re: [cocci] Checking SmPL test cases?

[Markus Elfring]

>> Will further collateral evolution happen according to a test result like the following?
>
> Now the pattern occurs on every control-flow path.

I wonder still about involved implementation details because mentioned test cases
demonstrated different data processing results.

Regards,
Markus

Re: [cocci] possible regressions in 1.1.1 -> 1.2

[Victor Gambier]

On 06/02/2025 10:31, Johannes Berg wrote:
> With "spatch version 1.1.1 compiled with OCaml version 4.14.2" this
> works just fine; however, we're trying to upgrade our dependencies and
> now with "spatch version 1.2 compiled with OCaml version 5.2.0" it
> doesn't match anything any more? I can't seem to figure out why, I
> thought maybe the types didn't match - not sure why we had added type u8
> to that, but pretty sure it was needed - but adding more types didn't
> really help either.

I ran a git bisect. As far as this match is concerned, the last known 
working commit is 4aa93b35; the first known non-working commit is 
74d791d77 (the intermediary commits do not build). That is not enough to 
narrow it down, I'll see if I can figure out something else.

Re: [cocci] Comparing software run time characteristics

[Markus Elfring]

> That is not enough to narrow it down, I'll see if I can figure out something else.
* Which test criteria did you try out so far?

* Will development interests grow anyhow for more precise analyses
  of questionable software behaviour?


Regards,
Markus

Re: [cocci] Profiling of SmPL code?

[Markus Elfring]

> That is not enough to narrow it down, I'll see if I can figure out something else.

Would you become interested to profile affected OCaml source code
in more detail?

How helpful would data be which could be determined by tools
like the following?
* ocamloptp
* ocamlprof
* gprof
* perf


Regards,
Markus

Re: [cocci] possible regressions in 1.1.1 -> 1.2

[Victor Gambier]

I'm happy to report this performance issue was identified and will be 
fixed in the next release.

Victor

On 2/11/25 10:58, Victor Gambier wrote:
>
> On 06/02/2025 10:31, Johannes Berg wrote:
>> With "spatch version 1.1.1 compiled with OCaml version 4.14.2" this
>> works just fine; however, we're trying to upgrade our dependencies and
>> now with "spatch version 1.2 compiled with OCaml version 5.2.0" it
>> doesn't match anything any more? I can't seem to figure out why, I
>> thought maybe the types didn't match - not sure why we had added type u8
>> to that, but pretty sure it was needed - but adding more types didn't
>> really help either.
>
> I ran a git bisect. As far as this match is concerned, the last known 
> working commit is 4aa93b35; the first known non-working commit is 
> 74d791d77 (the intermediary commits do not build). That is not enough 
> to narrow it down, I'll see if I can figure out something else.
>

Re: [cocci] possible regressions in 1.1.1 -> 1.2

[Johannes Berg]

On Fri, 2026-03-27 at 11:43 +0100, Victor Gambier wrote:
> I'm happy to report this performance issue was identified and will be 
> fixed in the next release.

Nice, thanks! I went with the <+... ...+> version at the time, I think,
so I didn't really have this issue any more :)

But it's great to see performance work in general, so thanks for that!

johannes