Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lib: remove pure attribute from functions that modify memory #8876

Merged
merged 1 commit into from
Jun 19, 2021

Conversation

idryzhov
Copy link
Contributor

@idryzhov idryzhov commented Jun 18, 2021

All these functions acquire a route_node lock. Marking them as pure
means that the compiler can optimize the code to not call them multiple
times when it already knows the return value. This is insane.

Fixes #8866
Fixes #8809
Fixes #8595
Fixes #6992

Signed-off-by: Igor Ryzhov iryzhov@nfware.com

@LabN-CI
Copy link
Collaborator

LabN-CI commented Jun 18, 2021

Outdated results 💚

Basic BGPD CI results: SUCCESS, 0 tests failed

_ _
Result SUCCESS git merge/8876 c5e7038
Date 06/18/2021
Start 06:10:47
Finish 06:36:19
Run-Time 25:32
Total 1815
Pass 1815
Fail 0
Valgrind-Errors
Valgrind-Loss
Details vncregress-2021-06-18-06:10:47.txt
Log autoscript-2021-06-18-06:12:00.log.bz2
Memory 511 515 429

For details, please contact louberger

Copy link
Contributor

@mjstapp mjstapp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the "ext_pure" macro used anywhere else? maybe it should be removed entirely so that it doesn't tempt anyone else?

@donaldsharp
Copy link
Member

in theory having the ability to specify that a function doesn't modify memory is good right? I can see places where it would make sense. We just didn't apply it properly in this case?

@mjstapp
Copy link
Contributor

mjstapp commented Jun 18, 2021

in theory having the ability to specify that a function doesn't modify memory is good right? I can see places where it would make sense. We just didn't apply it properly in this case?

I guess that's the question: I think it's an attractive nuisance, and the micro-optimization isn't worth the time to figure out how to use it safely, or the time to figure out how to review its use.

@idryzhov
Copy link
Contributor Author

I agree with Mark here. This is such a micro-optimization that can be used improperly and lead to a disastrous bugs like this one. There are only two functions now that use the macro - route_table_count and route_table_prefix_iter_cmp. They are never called multiple times from the same function, so I don't think this optimization ever takes place.
I'll remove the macro completely if @donaldsharp agrees.

@qlyoung
Copy link
Member

qlyoung commented Jun 18, 2021

Wow, great find.

@NetDEF-CI
Copy link
Collaborator

NetDEF-CI commented Jun 18, 2021

Continuous Integration Result: SUCCESSFUL

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19686/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

@idryzhov
Copy link
Contributor Author

@donaldsharp, please agree with removing the macro completely, or merge this if you think we should keep the macro.

@donaldsharp
Copy link
Member

go for it

Almost all functions currently marked with pure attribute acquire a
route_node lock. By marking them pure we allow compiler to optimize the
code and not call them when it already knows the return value. This is
completely incorrect.

Only two of eleven functions can be marked as pure. And they still won't
be optimized because they are never called from the same function twice.
Let's remove the ext_pure macro completely to reduce the chance of
repeating this mistake in the future.

Fixes FRRouting#8866, FRRouting#8809, FRRouting#8595, FRRouting#6992.

Signed-off-by: Igor Ryzhov <iryzhov@nfware.com>
@LabN-CI
Copy link
Collaborator

LabN-CI commented Jun 18, 2021

💚 Basic BGPD CI results: SUCCESS, 0 tests failed

Results table
_ _
Result SUCCESS git merge/8876 4f08c71
Date 06/18/2021
Start 13:00:46
Finish 13:26:16
Run-Time 25:30
Total 1815
Pass 1815
Fail 0
Valgrind-Errors
Valgrind-Loss
Details vncregress-2021-06-18-13:00:46.txt
Log autoscript-2021-06-18-13:02:02.log.bz2
Memory 486 492 430

For details, please contact louberger

@eqvinox
Copy link
Contributor

eqvinox commented Jun 18, 2021

Great find. Ouch. As the person who added ext_pure and introduced these bugs — sigh — I agree it should be removed, being too hard to use correctly. It's been sitting there b0rked for 2 years :(

@eqvinox
Copy link
Contributor

eqvinox commented Jun 18, 2021

FWIW the typesafe lists also use __attribute__((pure)), but that's only for _count, _first, _next and _next_safe - which indeed don't modify any memory. Double checked just to be safe. (And for these the attribute/optimization actually does make some sense.)

@idryzhov
Copy link
Contributor Author

ci:rerun

@NetDEF-CI
Copy link
Collaborator

NetDEF-CI commented Jun 19, 2021

Continuous Integration Result: FAILED

Continuous Integration Result: FAILED

See below for issues.
CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19694/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

Get source / Pull Request: Successful

Building Stage: Successful

Basic Tests: Failed

Topotests Ubuntu 18.04 i386 part 4: Failed (click for details)

Topology Test Results are at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-TOPO4U18I386-19694/test

Topology Tests failed for Topotests Ubuntu 18.04 i386 part 4:

2021-06-18 21:30:01,143 ERROR: 'router_json_cmp' failed after 131.40 seconds
2021-06-18 21:30:01,144 ERROR: assert failed at "bgp_ipv6_rtadv.test_bgp_ipv6_rtadv/test_protocols_convergence": "r2" JSON output mismatches
assert Generated JSON diff error report:
  
  > $: d2 has key '10.254.254.1/32' which is not present in d1
2021-06-18 21:33:31,009 ERROR: Traceback (most recent call last):
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 2530, in create_bgp_community_lists
    tgen, router, config_data, "bgp_community_list", build=build
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 355, in create_common_configuration
    load_config_to_router(tgen, router)
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 619, in load_config_to_router
    raise InvalidCLIError("%s" % output)
InvalidCLIError: % Malformed community-list value
line 2: Failure to communicate[13] to bgpd, line: bgp community-list standard ANY permit 0:-1 



2021-06-18 21:33:31,317 ERROR: Traceback (most recent call last):
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 2530, in create_bgp_community_lists
    tgen, router, config_data, "bgp_community_list", build=build
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 355, in create_common_configuration
    load_config_to_router(tgen, router)
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 619, in load_config_to_router
    raise InvalidCLIError("%s" % output)
InvalidCLIError: % Malformed community-list value
line 2: Failure to communicate[13] to bgpd, line: bgp community-list standard ANY permit 0:65536 



2021-06-18 21:33:31,638 ERROR: Traceback (most recent call last):
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 2530, in create_bgp_community_lists
    tgen, router, config_data, "bgp_community_list", build=build
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 355, in create_common_configuration
    load_config_to_router(tgen, router)
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 619, in load_config_to_router
    raise InvalidCLIError("%s" % output)
InvalidCLIError: % Malformed community-list value
line 2: Failure to communicate[13] to bgpd, line: bgp large-community-list standard ANY permit 0:4294967296 



2021-06-18 21:33:31,919 ERROR: Traceback (most recent call last):
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 2530, in create_bgp_community_lists
    tgen, router, config_data, "bgp_community_list", build=build
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 355, in create_common_configuration
    load_config_to_router(tgen, router)
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 619, in load_config_to_router
    raise InvalidCLIError("%s" % output)
InvalidCLIError: % Malformed community-list value
line 2: Failure to communicate[13] to bgpd, line: bgp large-community-list standard ANY permit 0:-1:1 



2021-06-18 21:33:33,706 ERROR: Traceback (most recent call last):
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 2530, in create_bgp_community_lists
    tgen, router, config_data, "bgp_community_list", build=build
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 355, in create_common_configuration
    load_config_to_router(tgen, router)
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 619, in load_config_to_router
    raise InvalidCLIError("%s" % output)
InvalidCLIError: % Malformed community-list value
line 2: Failure to communicate[13] to bgpd, line: bgp large-community-list standard ANY permit 1:a:2 



2021-06-18 21:36:45,813 ERROR: Traceback (most recent call last):
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 2530, in create_bgp_community_lists
    tgen, router, config_data, "bgp_community_list", build=build
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 355, in create_common_configuration
    load_config_to_router(tgen, router)
  File "/root/bamboo-agent-home/xml-data/build-dir/FRR-FRRPULLREQ-TOPO4U18I386/topotests/lib/common_config.py", line 619, in load_config_to_router
    raise InvalidCLIError("%s" % output)
InvalidCLIError: line 2: % Command incomplete[4]: bgp large-community-list standard Test1 permit  

see full log at https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19694/artifact/TOPO4U18I386/ErrorLog/log_topotests.txt

Successful on other platforms/tests
  • Addresssanitizer topotests part 4
  • Topotests Ubuntu 18.04 amd64 part 1
  • Addresssanitizer topotests part 2
  • Topotests Ubuntu 18.04 arm8 part 3
  • Topotests debian 10 amd64 part 6
  • CentOS 7 rpm pkg check
  • Topotests Ubuntu 18.04 amd64 part 8
  • Topotests Ubuntu 18.04 i386 part 2
  • Topotests debian 10 amd64 part 1
  • Debian 9 deb pkg check
  • Topotests Ubuntu 18.04 i386 part 7
  • Addresssanitizer topotests part 9
  • Topotests Ubuntu 18.04 arm8 part 7
  • Topotests Ubuntu 18.04 arm8 part 2
  • Topotests debian 10 amd64 part 5
  • Topotests Ubuntu 18.04 i386 part 8
  • Topotests debian 10 amd64 part 0
  • Topotests Ubuntu 18.04 i386 part 3
  • Topotests Ubuntu 18.04 i386 part 6
  • Topotests Ubuntu 18.04 i386 part 1
  • Topotests debian 10 amd64 part 2
  • Topotests Ubuntu 18.04 amd64 part 2
  • Topotests Ubuntu 18.04 arm8 part 8
  • Addresssanitizer topotests part 7
  • Topotests debian 10 amd64 part 4
  • Topotests Ubuntu 18.04 amd64 part 6
  • Topotests Ubuntu 18.04 arm8 part 0
  • Static analyzer (clang)
  • Topotests Ubuntu 18.04 arm8 part 5
  • Topotests debian 10 amd64 part 3
  • Ubuntu 18.04 deb pkg check
  • Addresssanitizer topotests part 5
  • Ubuntu 16.04 deb pkg check
  • Topotests Ubuntu 18.04 amd64 part 5
  • Topotests Ubuntu 18.04 i386 part 0
  • Topotests Ubuntu 18.04 i386 part 5
  • Addresssanitizer topotests part 3
  • Addresssanitizer topotests part 0
  • IPv4 ldp protocol on Ubuntu 18.04
  • Topotests Ubuntu 18.04 i386 part 9
  • Topotests Ubuntu 18.04 arm8 part 1
  • Topotests Ubuntu 18.04 amd64 part 3
  • Addresssanitizer topotests part 1
  • IPv6 protocols on Ubuntu 18.04
  • Topotests Ubuntu 18.04 amd64 part 0
  • Topotests Ubuntu 18.04 amd64 part 4
  • Topotests Ubuntu 18.04 arm8 part 9
  • Topotests Ubuntu 18.04 arm8 part 4
  • Addresssanitizer topotests part 6
  • Addresssanitizer topotests part 8
  • Topotests Ubuntu 18.04 amd64 part 7
  • Debian 10 deb pkg check
  • Topotests debian 10 amd64 part 7
  • Topotests Ubuntu 18.04 arm8 part 6
  • IPv4 protocols on Ubuntu 18.04
  • Topotests Ubuntu 18.04 amd64 part 9
  • Topotests debian 10 amd64 part 9
  • Fedora 29 rpm pkg check
  • Ubuntu 20.04 deb pkg check
  • Topotests debian 10 amd64 part 8

@NetDEF-CI
Copy link
Collaborator

Continuous Integration Result: SUCCESSFUL

Congratulations, this patch passed basic tests

Tested-by: NetDEF / OpenSourceRouting.org CI System

CI System Testrun URL: https://ci1.netdef.org/browse/FRR-FRRPULLREQ-19698/

This is a comment from an automated CI system.
For questions and feedback in regards to this CI system, please feel free to email
Martin Winter - mwinter (at) opensourcerouting.org.

@eqvinox eqvinox merged commit ca4849b into FRRouting:master Jun 19, 2021
@eqvinox
Copy link
Contributor

eqvinox commented Jun 19, 2021

This should be very widely backported even to branches we no longer consider active; @idryzhov do you want me to do that or are you going to?

@idryzhov
Copy link
Contributor Author

@Mergifyio backport dev/8.0 stable/7.5 stable/7.4

@idryzhov
Copy link
Contributor Author

@eqvinox you can just post a comment similar to the one above if you want to backport to older branches.

@mergify
Copy link

mergify bot commented Jun 19, 2021

Command backport dev/8.0 stable/7.5 stable/7.4: success

Backports have been created

@eqvinox
Copy link
Contributor

eqvinox commented Jun 19, 2021

@eqvinox you can just post a comment similar to the one above if you want to backport to older branches.

ah cool I hadn't used the bot yet. let's see what the oldest branch this affects is...

(i would even backport to 6.0 if applicable, since that's what's in upstream Debian 10)

@eqvinox
Copy link
Contributor

eqvinox commented Jun 19, 2021

@Mergifyio backport stable/7.3 stable/7.2

@mergify
Copy link

mergify bot commented Jun 19, 2021

Command backport stable/7.3 stable/7.2: success

Backports have been created

@eqvinox
Copy link
Contributor

eqvinox commented Jun 19, 2021

(7.2 is the old release this affects.)

donaldsharp added a commit that referenced this pull request Jun 19, 2021
lib: remove pure attribute from functions that modify memory (backport #8876)
donaldsharp added a commit that referenced this pull request Jun 19, 2021
lib: remove pure attribute from functions that modify memory (backport #8876)
donaldsharp added a commit that referenced this pull request Jun 19, 2021
lib: remove pure attribute from functions that modify memory (backport #8876)
donaldsharp added a commit that referenced this pull request Jun 19, 2021
lib: remove pure attribute from functions that modify memory (backport #8876)
donaldsharp added a commit that referenced this pull request Jun 19, 2021
lib: remove pure attribute from functions that modify memory (backport #8876)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants