Remove cs_main lock annotation from ChainstateManager.m_blockman by ryanofsky · Pull Request #24024 · bitcoin/bitcoin

ryanofsky · 2022-01-10T21:28:36Z

BlockManager is a large data structure, and cs_main is not required to take its address or access every part of it. Individual BlockManager fields and methods which do require cs_main like m_block_index and LookupBlockIndex are already annotated separately, and these other annotations describe locking requirements more accurately and do a better job enforcing thread safety.

Since cs_main is not needed to access the address of the m_block object, this commit drops cs_main LOCK calls which were added pointlessly to satisfy this annotation in the past.

Code changes were made by dongcarl, I just wrote the commit description

DrahtBot · 2022-01-11T02:13:39Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

maflcko · 2022-01-11T09:25:45Z

cr ACK 5a1c413

cs_main is a validation lock. It has nothing to do with writing or reading blocks from disk (other than being historically used for that). Ideally nothing in blockman depends on cs_main.

BlockManager is a large data structure, and cs_main is not required to take its address or access every part of it. Individual BlockManager fields and methods which do require cs_main like m_block_index and LookupBlockIndex are already annotated separately, and these other annotations describe locking requirements more accurately and do a better job enforcing thread safety. Since cs_main is not needed to access the address of the m_block object, this commit drops cs_main LOCK calls which were added pointlessly to satisfy this annotation in the past. Co-authored-by: Carl Dong <contact@carldong.me>

jonatack

Light ACK. Attempted removing a few additional related cs_main locks and saw the expected warnings.

jonatack · 2022-01-11T13:02:12Z

src/validation.h

    //! A single BlockManager instance is shared across each constructed
    //! chainstate to avoid duplicating block metadata.
-    BlockManager m_blockman GUARDED_BY(::cs_main);
+    BlockManager m_blockman{};


Why add the braces? m_blockman is initialized in the ~~ctor (unless I'm missing something). Question for my benefit :)~~ constructor implementation.

re: #24024 (comment)

Why add the braces? m_blockman is initialized in the ctor (unless I'm missing something). Question for my benefit :)

Not sure since Carl sent me this commit, but it seems good to clean this up. I dropped the braces just to avoid unnecessary punctuation. The braces would never do anything since BlockManager isn't a primitive type.

This is the ctor

I liked them, because they clarified that there is no ctor and the initialization happens inline

I liked them, because they clarified that there is no ctor and the initialization happens inline

I wouldn't object to adding them back if people have strong opinions, but they don't guarantee this. Someone could add a ChainStateMananger constructor in the future which does pass arguments to the m_blockman constructor, and this code would still compile as long as m_blockman also has a default constructor. The nice thing about plain BlockManager m_blockman; is it says "ChainStateMananger has a m_blockman member. If you care about how it will be constructed, go look at the constructor implementation, like you need to do anyway."

Oh indeed, thanks for the responses!

ryanofsky

Rebased 5a1c413 -> ce95fb3 (pr/block.1 -> pr/block.2, compare) due to conflict with #23497

ryanofsky · 2022-01-11T13:55:52Z

src/validation.h

    //! A single BlockManager instance is shared across each constructed
    //! chainstate to avoid duplicating block metadata.
-    BlockManager m_blockman GUARDED_BY(::cs_main);
+    BlockManager m_blockman{};


re: #24024 (comment)

Why add the braces? m_blockman is initialized in the ctor (unless I'm missing something). Question for my benefit :)

Not sure since Carl sent me this commit, but it seems good to clean this up. I dropped the braces just to avoid unnecessary punctuation. The braces would never do anything since BlockManager isn't a primitive type.

ryanofsky · 2022-01-11T14:33:38Z

re: #24024 (comment)

cs_main is a validation lock. It has nothing to do with writing or reading blocks from disk (other than being historically used for that). Ideally nothing in blockman depends on cs_main.

That's a good point. It is important to have some lock guarding things like m_block_index but it it doesn't need to be cs_main and maybe ideally it would be a more narrow lock. It looks like there is more discussion about this at #22932 (comment)

maflcko · 2022-01-11T15:04:46Z

cr ACK ce95fb3

dongcarl · 2022-01-11T17:10:52Z

crACK ce95fb3

Thanks for PRing this!

jonatack · 2022-01-12T09:18:45Z

ACK ce95fb3 per git range-diff db1f04f 5a1c413 ce95fb3 change since last push is rebase and dropping braces on the member initialization

…nager.m_blockman ce95fb3 Remove cs_main lock annotation from ChainstateManager.m_blockman (Ryan Ofsky) Pull request description: `BlockManager` is a large data structure, and `cs_main` is not required to take its address or access every part of it. Individual `BlockManager` fields and methods which do require `cs_main` like `m_block_index` and `LookupBlockIndex` are already annotated separately, and these other annotations describe locking requirements more accurately and do a better job enforcing thread safety. Since `cs_main` is not needed to access the address of the m_block object, this commit drops `cs_main` LOCK calls which were added pointlessly to satisfy this annotation in the past. Code changes were made by dongcarl, I just wrote the commit description ACKs for top commit: MarcoFalke: cr ACK ce95fb3 dongcarl: crACK ce95fb3 jonatack: ACK ce95fb3 per `git range-diff db1f04f 5a1c413 ce95fb3` change since last push is rebase and dropping braces on the member initialization Tree-SHA512: b18a6ebcc70bea750485f04d4feb7bb28450fea2176e513be9cc242e9f63b24254c5659e74eb6d6045c706a3aaeb94688937b25b7ca7653f8aa3cf8c18845d5a

Summary: BlockManager is a large data structure, and cs_main is not required to take its address or access every part of it. Individual BlockManager fields and methods which do require cs_main like m_block_index and LookupBlockIndex are already annotated separately, and these other annotations describe locking requirements more accurately and do a better job enforcing thread safety. Since cs_main is not needed to access the address of the m_block object, this commit drops cs_main LOCK calls which were added pointlessly to satisfy this annotation in the past. Co-authored-by: Carl Dong <contact@carldong.me> This is a backport of [[bitcoin/bitcoin#24024 | core#24024]] Test Plan: With clang and Debug `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12909

DrahtBot added the Validation label Jan 10, 2022

DrahtBot mentioned this pull request Jan 11, 2022

Add src/node/ and src/wallet/ code to node:: and wallet:: namespaces #23497

Merged

DrahtBot added the Needs rebase label Jan 11, 2022

jonatack reviewed Jan 11, 2022

View reviewed changes

ryanofsky force-pushed the pr/block branch from 5a1c413 to ce95fb3 Compare January 11, 2022 14:27

ryanofsky commented Jan 11, 2022

View reviewed changes

DrahtBot removed the Needs rebase label Jan 11, 2022

maflcko merged commit 1f7acfd into bitcoin:master Jan 12, 2022

dongcarl mentioned this pull request Jan 13, 2022

Q: cs_main protection for referencing m_blockman/g_chainman? #20325

Closed

bitcoin locked and limited conversation to collaborators Jan 19, 2023

Conversation

ryanofsky commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Jan 11, 2022

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

maflcko Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

jonatack Jan 12, 2022

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jan 11, 2022

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Jan 11, 2022

Uh oh!

maflcko commented Jan 11, 2022

Uh oh!

dongcarl commented Jan 11, 2022

Uh oh!

jonatack commented Jan 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ryanofsky commented Jan 10, 2022 •

edited

Loading

DrahtBot commented Jan 11, 2022 •

edited

Loading

jonatack Jan 11, 2022 •

edited

Loading