Hold PeerState in an RwLock rather than a Mutex

[tnull]

Previously, we would hold the PeerState in a Mutex, which disallows concurrent read-only operations. Here we switch to an RwLock making this possible.

To this end, we first switch all instances of Mutex::lock to RwLock::write, and then selectively adapt some usages permitting for it to RwLock::read.

[TheBlueMatt]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.34884% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 89.37%. Comparing base (1d9e541) to head (cf2d418).

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	95.13%	1 Missing and 6 partials ⚠️
lightning/src/ln/functional_test_utils.rs	83.33%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2968 +/- ##
==========================================
+ Coverage 89.31% 89.37% +0.05% 
==========================================
Files 117 117 Lines 95618 95618 Branches 95618 95618 ==========================================
+ Hits 85404 85456 +52 + Misses 7974 7926 -48 + Partials 2240 2236 -4 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tnull]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

Right, the users who reported this should def. investigate why it was blocking for so long. I now went through all usages and found several places where we could also just us a RwLockReadGuard (forward_intercepted_htlcs, compute_inflight_htlcs, get_relevant_txids), a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

IMO, the other cases make this a worthwhile improvement, independently of the concrete reported issue.

[TheBlueMatt]

a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

Writing the manager basically halts the world no matter what due to the consistency write lock.

I'm not entirely convinced by the remaining changes that they're worth it.

[tnull]

Writing the manager basically halts the world no matter what due to the consistency write lock.

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

[TheBlueMatt]

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

Ah, fair point, duh. Note that write currently takes the top-level per_peer_statewrite lock, but that's an easy swap in this PR.

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

[tnull]

Ah, fair point, duh. Note that write currently takes the top-level per_peer_statewrite lock, but that's an easy swap in this PR.

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

So, IIUC, we can have during access during parts of the serialization, but not for the full write call?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

Ah, okay, the doc's explanation around 'different branches' had me confused whether we're allowed to skip the root or not. Logically it makes sense, the docs aren't fully clear on that though, IMO.

[TheBlueMatt]

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Yea, I guess I was figuring that loop would be pretty quick, but I guess it does write all the channels so its not free, even if pretty quick.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

Yea, its...delicate. IIRC this is the only place we actually take per-peer locks recursively, so the total_consistency_guard saves us, but of course it is a little less fragile to keep relying on the per_peer write lock for that.

[tnull]

Rebased on main to resolve minor conflicts.

[TheBlueMatt]

Still not entirely convinced this improves parallelism in any practical cases, honestly.

[TheBlueMatt]

Is there a race condition here where we decide whether or not to include the channel at this step but then the channel gets updated and we write the wrong number of channels? Doesn't seem like it was introduced in this PR but still worth fixing.

[tnull]

Yeah, I noticed that as well and created a TODO for it but so far intended to fix it elsewhere. Looking at it again it seems that this could be fixed by a) only iterating the peer states once and persisting to a temporary buffer while keeping track of serializable_peer_count and number_of_funded_channels or b) taking the per_peer_state.write() lock earlier and holding it for the remainder of the method.

Seems a) is rather inefficient and b) would mean dropping any chance of concurrent access during at least parts of write. I think switching to an RwLock might still be nice in general and going forward, but if you don't agree, possibly we should just do b) in a separate PR and close this one?

[TheBlueMatt]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

[tnull]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

Now opened to this effect: #2998

Closing this as the benefits of switching to RwLock are not immediately clear, and we can always do so if we find a compelling reason in the future.

[tnull]

Previously, we would hold the PeerState in a Mutex, which disallows concurrent read-only operations. Here we switch to an RwLock making this possible.

To this end, we first switch all instances of Mutex::lock to RwLock::write, and then selectively adapt some usages permitting for it to RwLock::read.

[TheBlueMatt]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.34884% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 89.37%. Comparing base (1d9e541) to head (cf2d418).

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	95.13%	1 Missing and 6 partials ⚠️
lightning/src/ln/functional_test_utils.rs	83.33%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2968 +/- ##
==========================================
+ Coverage 89.31% 89.37% +0.05% 
==========================================
Files 117 117 Lines 95618 95618 Branches 95618 95618 ==========================================
+ Hits 85404 85456 +52 + Misses 7974 7926 -48 + Partials 2240 2236 -4 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tnull]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

Right, the users who reported this should def. investigate why it was blocking for so long. I now went through all usages and found several places where we could also just us a RwLockReadGuard (forward_intercepted_htlcs, compute_inflight_htlcs, get_relevant_txids), a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

IMO, the other cases make this a worthwhile improvement, independently of the concrete reported issue.

[TheBlueMatt]

a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

Writing the manager basically halts the world no matter what due to the consistency write lock.

I'm not entirely convinced by the remaining changes that they're worth it.

[tnull]

Writing the manager basically halts the world no matter what due to the consistency write lock.

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

[TheBlueMatt]

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

Ah, fair point, duh. Note that write currently takes the top-level per_peer_statewrite lock, but that's an easy swap in this PR.

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

[tnull]

Ah, fair point, duh. Note that write currently takes the top-level per_peer_statewrite lock, but that's an easy swap in this PR.

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

So, IIUC, we can have during access during parts of the serialization, but not for the full write call?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

Ah, okay, the doc's explanation around 'different branches' had me confused whether we're allowed to skip the root or not. Logically it makes sense, the docs aren't fully clear on that though, IMO.

[TheBlueMatt]

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Yea, I guess I was figuring that loop would be pretty quick, but I guess it does write all the channels so its not free, even if pretty quick.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

Yea, its...delicate. IIRC this is the only place we actually take per-peer locks recursively, so the total_consistency_guard saves us, but of course it is a little less fragile to keep relying on the per_peer write lock for that.

[tnull]

Rebased on main to resolve minor conflicts.

[TheBlueMatt]

Still not entirely convinced this improves parallelism in any practical cases, honestly.

[TheBlueMatt]

Is there a race condition here where we decide whether or not to include the channel at this step but then the channel gets updated and we write the wrong number of channels? Doesn't seem like it was introduced in this PR but still worth fixing.

[tnull]

Yeah, I noticed that as well and created a TODO for it but so far intended to fix it elsewhere. Looking at it again it seems that this could be fixed by a) only iterating the peer states once and persisting to a temporary buffer while keeping track of serializable_peer_count and number_of_funded_channels or b) taking the per_peer_state.write() lock earlier and holding it for the remainder of the method.

Seems a) is rather inefficient and b) would mean dropping any chance of concurrent access during at least parts of write. I think switching to an RwLock might still be nice in general and going forward, but if you don't agree, possibly we should just do b) in a separate PR and close this one?

[TheBlueMatt]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

[tnull]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

Now opened to this effect: #2998

Closing this as the benefits of switching to RwLock are not immediately clear, and we can always do so if we find a compelling reason in the future.

[tnull]

Previously, we would hold the PeerState in a Mutex, which disallows concurrent read-only operations. Here we switch to an RwLock making this possible.

To this end, we first switch all instances of Mutex::lock to RwLock::write, and then selectively adapt some usages permitting for it to RwLock::read.

[TheBlueMatt]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.34884% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 89.37%. Comparing base (1d9e541) to head (cf2d418).

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	95.13%	1 Missing and 6 partials ⚠️
lightning/src/ln/functional_test_utils.rs	83.33%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2968 +/- ##
==========================================
+ Coverage 89.31% 89.37% +0.05% 
==========================================
Files 117 117 Lines 95618 95618 Branches 95618 95618 ==========================================
+ Hits 85404 85456 +52 + Misses 7974 7926 -48 + Partials 2240 2236 -4 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tnull]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

Right, the users who reported this should def. investigate why it was blocking for so long. I now went through all usages and found several places where we could also just us a RwLockReadGuard (forward_intercepted_htlcs, compute_inflight_htlcs, get_relevant_txids), a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

IMO, the other cases make this a worthwhile improvement, independently of the concrete reported issue.

[TheBlueMatt]

a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

Writing the manager basically halts the world no matter what due to the consistency write lock.

I'm not entirely convinced by the remaining changes that they're worth it.

[tnull]

Writing the manager basically halts the world no matter what due to the consistency write lock.

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

[TheBlueMatt]

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

Ah, fair point, duh. Note that write currently takes the top-level per_peer_statewrite lock, but that's an easy swap in this PR.

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

[tnull]

Ah, fair point, duh. Note that write currently takes the top-level per_peer_statewrite lock, but that's an easy swap in this PR.

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

So, IIUC, we can have during access during parts of the serialization, but not for the full write call?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

Ah, okay, the doc's explanation around 'different branches' had me confused whether we're allowed to skip the root or not. Logically it makes sense, the docs aren't fully clear on that though, IMO.

[TheBlueMatt]

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Yea, I guess I was figuring that loop would be pretty quick, but I guess it does write all the channels so its not free, even if pretty quick.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

Yea, its...delicate. IIRC this is the only place we actually take per-peer locks recursively, so the total_consistency_guard saves us, but of course it is a little less fragile to keep relying on the per_peer write lock for that.

[tnull]

Rebased on main to resolve minor conflicts.

[TheBlueMatt]

Still not entirely convinced this improves parallelism in any practical cases, honestly.

[TheBlueMatt]

Is there a race condition here where we decide whether or not to include the channel at this step but then the channel gets updated and we write the wrong number of channels? Doesn't seem like it was introduced in this PR but still worth fixing.

[tnull]

Yeah, I noticed that as well and created a TODO for it but so far intended to fix it elsewhere. Looking at it again it seems that this could be fixed by a) only iterating the peer states once and persisting to a temporary buffer while keeping track of serializable_peer_count and number_of_funded_channels or b) taking the per_peer_state.write() lock earlier and holding it for the remainder of the method.

Seems a) is rather inefficient and b) would mean dropping any chance of concurrent access during at least parts of write. I think switching to an RwLock might still be nice in general and going forward, but if you don't agree, possibly we should just do b) in a separate PR and close this one?

[TheBlueMatt]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

[tnull]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

Now opened to this effect: #2998

Closing this as the benefits of switching to RwLock are not immediately clear, and we can always do so if we find a compelling reason in the future.

[tnull]

Previously, we would hold the PeerState in a Mutex, which disallows concurrent read-only operations. Here we switch to an RwLock making this possible.

To this end, we first switch all instances of Mutex::lock to RwLock::write, and then selectively adapt some usages permitting for it to RwLock::read.

[TheBlueMatt]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.34884% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 89.37%. Comparing base (1d9e541) to head (cf2d418).

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	95.13%	1 Missing and 6 partials ⚠️
lightning/src/ln/functional_test_utils.rs	83.33%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2968 +/- ##
==========================================
+ Coverage 89.31% 89.37% +0.05% 
==========================================
Files 117 117 Lines 95618 95618 Branches 95618 95618 ==========================================
+ Hits 85404 85456 +52 + Misses 7974 7926 -48 + Partials 2240 2236 -4 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tnull]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

Right, the users who reported this should def. investigate why it was blocking for so long. I now went through all usages and found several places where we could also just us a RwLockReadGuard (forward_intercepted_htlcs, compute_inflight_htlcs, get_relevant_txids), a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

IMO, the other cases make this a worthwhile improvement, independently of the concrete reported issue.

[TheBlueMatt]

a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

Writing the manager basically halts the world no matter what due to the consistency write lock.

I'm not entirely convinced by the remaining changes that they're worth it.

[tnull]

Writing the manager basically halts the world no matter what due to the consistency write lock.

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

[TheBlueMatt]

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

Ah, fair point, duh. Note that write currently takes the top-level per_peer_statewrite lock, but that's an easy swap in this PR.

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

[tnull]

Ah, fair point, duh. Note that write currently takes the top-level per_peer_statewrite lock, but that's an easy swap in this PR.

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

So, IIUC, we can have during access during parts of the serialization, but not for the full write call?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

Ah, okay, the doc's explanation around 'different branches' had me confused whether we're allowed to skip the root or not. Logically it makes sense, the docs aren't fully clear on that though, IMO.

[TheBlueMatt]

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Yea, I guess I was figuring that loop would be pretty quick, but I guess it does write all the channels so its not free, even if pretty quick.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

Yea, its...delicate. IIRC this is the only place we actually take per-peer locks recursively, so the total_consistency_guard saves us, but of course it is a little less fragile to keep relying on the per_peer write lock for that.

[tnull]

Rebased on main to resolve minor conflicts.

[TheBlueMatt]

Still not entirely convinced this improves parallelism in any practical cases, honestly.

[TheBlueMatt]

Is there a race condition here where we decide whether or not to include the channel at this step but then the channel gets updated and we write the wrong number of channels? Doesn't seem like it was introduced in this PR but still worth fixing.

[tnull]

Yeah, I noticed that as well and created a TODO for it but so far intended to fix it elsewhere. Looking at it again it seems that this could be fixed by a) only iterating the peer states once and persisting to a temporary buffer while keeping track of serializable_peer_count and number_of_funded_channels or b) taking the per_peer_state.write() lock earlier and holding it for the remainder of the method.

Seems a) is rather inefficient and b) would mean dropping any chance of concurrent access during at least parts of write. I think switching to an RwLock might still be nice in general and going forward, but if you don't agree, possibly we should just do b) in a separate PR and close this one?

[TheBlueMatt]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

[tnull]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

Now opened to this effect: #2998

Closing this as the benefits of switching to RwLock are not immediately clear, and we can always do so if we find a compelling reason in the future.

[tnull]

Previously, we would hold the PeerState in a Mutex, which disallows concurrent read-only operations. Here we switch to an RwLock making this possible.

To this end, we first switch all instances of Mutex::lock to RwLock::write, and then selectively adapt some usages permitting for it to RwLock::read.

[TheBlueMatt]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.34884% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 89.37%. Comparing base (1d9e541) to head (cf2d418).

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	95.13%	1 Missing and 6 partials ⚠️
lightning/src/ln/functional_test_utils.rs	83.33%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2968 +/- ##
==========================================
+ Coverage 89.31% 89.37% +0.05% 
==========================================
Files 117 117 Lines 95618 95618 Branches 95618 95618 ==========================================
+ Hits 85404 85456 +52 + Misses 7974 7926 -48 + Partials 2240 2236 -4 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tnull]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

Right, the users who reported this should def. investigate why it was blocking for so long. I now went through all usages and found several places where we could also just us a RwLockReadGuard (forward_intercepted_htlcs, compute_inflight_htlcs, get_relevant_txids), a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

IMO, the other cases make this a worthwhile improvement, independently of the concrete reported issue.

[TheBlueMatt]

a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

Writing the manager basically halts the world no matter what due to the consistency write lock.

I'm not entirely convinced by the remaining changes that they're worth it.

[tnull]

Writing the manager basically halts the world no matter what due to the consistency write lock.

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

[TheBlueMatt]

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

Ah, fair point, duh. Note that write currently takes the top-level per_peer_statewrite lock, but that's an easy swap in this PR.

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

[tnull]

Ah, fair point, duh. Note that write currently takes the top-level per_peer_statewrite lock, but that's an easy swap in this PR.

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

So, IIUC, we can have during access during parts of the serialization, but not for the full write call?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

Ah, okay, the doc's explanation around 'different branches' had me confused whether we're allowed to skip the root or not. Logically it makes sense, the docs aren't fully clear on that though, IMO.

[TheBlueMatt]

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Yea, I guess I was figuring that loop would be pretty quick, but I guess it does write all the channels so its not free, even if pretty quick.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

Yea, its...delicate. IIRC this is the only place we actually take per-peer locks recursively, so the total_consistency_guard saves us, but of course it is a little less fragile to keep relying on the per_peer write lock for that.

[tnull]

Rebased on main to resolve minor conflicts.

[TheBlueMatt]

Still not entirely convinced this improves parallelism in any practical cases, honestly.

[TheBlueMatt]

Is there a race condition here where we decide whether or not to include the channel at this step but then the channel gets updated and we write the wrong number of channels? Doesn't seem like it was introduced in this PR but still worth fixing.

[tnull]

Yeah, I noticed that as well and created a TODO for it but so far intended to fix it elsewhere. Looking at it again it seems that this could be fixed by a) only iterating the peer states once and persisting to a temporary buffer while keeping track of serializable_peer_count and number_of_funded_channels or b) taking the per_peer_state.write() lock earlier and holding it for the remainder of the method.

Seems a) is rather inefficient and b) would mean dropping any chance of concurrent access during at least parts of write. I think switching to an RwLock might still be nice in general and going forward, but if you don't agree, possibly we should just do b) in a separate PR and close this one?

[TheBlueMatt]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

[tnull]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

Now opened to this effect: #2998

Closing this as the benefits of switching to RwLock are not immediately clear, and we can always do so if we find a compelling reason in the future.

[tnull]

Previously, we would hold the PeerState in a Mutex, which disallows concurrent read-only operations. Here we switch to an RwLock making this possible.

To this end, we first switch all instances of Mutex::lock to RwLock::write, and then selectively adapt some usages permitting for it to RwLock::read.

[TheBlueMatt]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.34884% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 89.37%. Comparing base (1d9e541) to head (cf2d418).

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	95.13%	1 Missing and 6 partials ⚠️
lightning/src/ln/functional_test_utils.rs	83.33%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2968 +/- ##
==========================================
+ Coverage 89.31% 89.37% +0.05% 
==========================================
Files 117 117 Lines 95618 95618 Branches 95618 95618 ==========================================
+ Hits 85404 85456 +52 + Misses 7974 7926 -48 + Partials 2240 2236 -4 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tnull]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

Right, the users who reported this should def. investigate why it was blocking for so long. I now went through all usages and found several places where we could also just us a RwLockReadGuard (forward_intercepted_htlcs, compute_inflight_htlcs, get_relevant_txids), a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

IMO, the other cases make this a worthwhile improvement, independently of the concrete reported issue.

[TheBlueMatt]

a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

Writing the manager basically halts the world no matter what due to the consistency write lock.

I'm not entirely convinced by the remaining changes that they're worth it.

[tnull]

Writing the manager basically halts the world no matter what due to the consistency write lock.

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

[TheBlueMatt]

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

Ah, fair point, duh. Note that write currently takes the top-level per_peer_statewrite lock, but that's an easy swap in this PR.

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

[tnull]

Ah, fair point, duh. Note that write currently takes the top-level per_peer_statewrite lock, but that's an easy swap in this PR.

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

So, IIUC, we can have during access during parts of the serialization, but not for the full write call?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

Ah, okay, the doc's explanation around 'different branches' had me confused whether we're allowed to skip the root or not. Logically it makes sense, the docs aren't fully clear on that though, IMO.

[TheBlueMatt]

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Yea, I guess I was figuring that loop would be pretty quick, but I guess it does write all the channels so its not free, even if pretty quick.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

Yea, its...delicate. IIRC this is the only place we actually take per-peer locks recursively, so the total_consistency_guard saves us, but of course it is a little less fragile to keep relying on the per_peer write lock for that.

[tnull]

Rebased on main to resolve minor conflicts.

[TheBlueMatt]

Still not entirely convinced this improves parallelism in any practical cases, honestly.

[TheBlueMatt]

Is there a race condition here where we decide whether or not to include the channel at this step but then the channel gets updated and we write the wrong number of channels? Doesn't seem like it was introduced in this PR but still worth fixing.

[tnull]

Yeah, I noticed that as well and created a TODO for it but so far intended to fix it elsewhere. Looking at it again it seems that this could be fixed by a) only iterating the peer states once and persisting to a temporary buffer while keeping track of serializable_peer_count and number_of_funded_channels or b) taking the per_peer_state.write() lock earlier and holding it for the remainder of the method.

Seems a) is rather inefficient and b) would mean dropping any chance of concurrent access during at least parts of write. I think switching to an RwLock might still be nice in general and going forward, but if you don't agree, possibly we should just do b) in a separate PR and close this one?

[TheBlueMatt]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

[tnull]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

Now opened to this effect: #2998

Closing this as the benefits of switching to RwLock are not immediately clear, and we can always do so if we find a compelling reason in the future.

[tnull]

Previously, we would hold the PeerState in a Mutex, which disallows concurrent read-only operations. Here we switch to an RwLock making this possible.

To this end, we first switch all instances of Mutex::lock to RwLock::write, and then selectively adapt some usages permitting for it to RwLock::read.

[TheBlueMatt]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.34884% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 89.37%. Comparing base (1d9e541) to head (cf2d418).

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	95.13%	1 Missing and 6 partials ⚠️
lightning/src/ln/functional_test_utils.rs	83.33%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2968 +/- ##
==========================================
+ Coverage 89.31% 89.37% +0.05% 
==========================================
Files 117 117 Lines 95618 95618 Branches 95618 95618 ==========================================
+ Hits 85404 85456 +52 + Misses 7974 7926 -48 + Partials 2240 2236 -4 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tnull]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

Right, the users who reported this should def. investigate why it was blocking for so long. I now went through all usages and found several places where we could also just us a RwLockReadGuard (forward_intercepted_htlcs, compute_inflight_htlcs, get_relevant_txids), a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

IMO, the other cases make this a worthwhile improvement, independently of the concrete reported issue.

[TheBlueMatt]

a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

Writing the manager basically halts the world no matter what due to the consistency write lock.

I'm not entirely convinced by the remaining changes that they're worth it.

[tnull]

Writing the manager basically halts the world no matter what due to the consistency write lock.

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

[TheBlueMatt]

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

Ah, fair point, duh. Note that write currently takes the top-level per_peer_statewrite lock, but that's an easy swap in this PR.

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

[tnull]

Ah, fair point, duh. Note that write currently takes the top-level per_peer_statewrite lock, but that's an easy swap in this PR.

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

So, IIUC, we can have during access during parts of the serialization, but not for the full write call?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

Ah, okay, the doc's explanation around 'different branches' had me confused whether we're allowed to skip the root or not. Logically it makes sense, the docs aren't fully clear on that though, IMO.

[TheBlueMatt]

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Yea, I guess I was figuring that loop would be pretty quick, but I guess it does write all the channels so its not free, even if pretty quick.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

Yea, its...delicate. IIRC this is the only place we actually take per-peer locks recursively, so the total_consistency_guard saves us, but of course it is a little less fragile to keep relying on the per_peer write lock for that.

[tnull]

Rebased on main to resolve minor conflicts.

[TheBlueMatt]

Still not entirely convinced this improves parallelism in any practical cases, honestly.

[TheBlueMatt]

Is there a race condition here where we decide whether or not to include the channel at this step but then the channel gets updated and we write the wrong number of channels? Doesn't seem like it was introduced in this PR but still worth fixing.

[tnull]

Yeah, I noticed that as well and created a TODO for it but so far intended to fix it elsewhere. Looking at it again it seems that this could be fixed by a) only iterating the peer states once and persisting to a temporary buffer while keeping track of serializable_peer_count and number_of_funded_channels or b) taking the per_peer_state.write() lock earlier and holding it for the remainder of the method.

Seems a) is rather inefficient and b) would mean dropping any chance of concurrent access during at least parts of write. I think switching to an RwLock might still be nice in general and going forward, but if you don't agree, possibly we should just do b) in a separate PR and close this one?

[TheBlueMatt]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

[tnull]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

Now opened to this effect: #2998

Closing this as the benefits of switching to RwLock are not immediately clear, and we can always do so if we find a compelling reason in the future.

[tnull]

Previously, we would hold the PeerState in a Mutex, which disallows concurrent read-only operations. Here we switch to an RwLock making this possible.

To this end, we first switch all instances of Mutex::lock to RwLock::write, and then selectively adapt some usages permitting for it to RwLock::read.

[TheBlueMatt]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 95.34884% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 89.37%. Comparing base (1d9e541) to head (cf2d418).

Files	Patch %	Lines
lightning/src/ln/channelmanager.rs	95.13%	1 Missing and 6 partials ⚠️
lightning/src/ln/functional_test_utils.rs	83.33%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@ Coverage Diff @@## main #2968 +/- ##
==========================================
+ Coverage 89.31% 89.37% +0.05% 
==========================================
Files 117 117 Lines 95618 95618 Branches 95618 95618 ==========================================
+ Hits 85404 85456 +52 + Misses 7974 7926 -48 + Partials 2240 2236 -4 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tnull]

I'd like to better understand the reported use for this - list_channels, specifically, should be plenty fast enough that the time under a single lock is ~instant and this doesn't change much of anything. If there's some other places where we can be read-only for longer (eg I/O) then I'd say lets do it.

Right, the users who reported this should def. investigate why it was blocking for so long. I now went through all usages and found several places where we could also just us a RwLockReadGuard (forward_intercepted_htlcs, compute_inflight_htlcs, get_relevant_txids), a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

IMO, the other cases make this a worthwhile improvement, independently of the concrete reported issue.

[TheBlueMatt]

a particularly promising case being taking a RwLockReadGuard in ChannelManager::write, during channel persistence.

Writing the manager basically halts the world no matter what due to the consistency write lock.

I'm not entirely convinced by the remaining changes that they're worth it.

[tnull]

Writing the manager basically halts the world no matter what due to the consistency write lock.

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

[TheBlueMatt]

Yes, for many operations, but we don't acquire total_consistency_lock in a lot of places, e.g. list_channels and get_relevant_txids. So they could still be executed concurrently during a ChannelManager::write operation (at least during persisting channels, at some point we then acquire the write lock on per_peer_state).

Ah, fair point, duh. Note that write currently takes the top-level per_peer_statewrite lock, but that's an easy swap in this PR.

What am I missing? Is not acquiring it a lock order violation we need to fix? The docs seem to indicate so, but I'm not sure taking total_consistency_lock for each list_channels makes a lot of sense?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

[tnull]

Ah, fair point, duh. Note that write currently takes the top-level per_peer_statewrite lock, but that's an easy swap in this PR.

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

So, IIUC, we can have during access during parts of the serialization, but not for the full write call?

No, there's no lockorder violation if we skip a lock, only if they're in the wrong order.

Ah, okay, the doc's explanation around 'different branches' had me confused whether we're allowed to skip the root or not. Logically it makes sense, the docs aren't fully clear on that though, IMO.

[TheBlueMatt]

I don't think so? During channel data serialization we take the read lock here, which would allow concurrent access with the changes in this PR.

Yea, I guess I was figuring that loop would be pretty quick, but I guess it does write all the channels so its not free, even if pretty quick.

Then later we take the write lock here, but this comment has me believe we can't change that to assert we'd never violate lock order?

Yea, its...delicate. IIRC this is the only place we actually take per-peer locks recursively, so the total_consistency_guard saves us, but of course it is a little less fragile to keep relying on the per_peer write lock for that.

[tnull]

Rebased on main to resolve minor conflicts.

[TheBlueMatt]

Still not entirely convinced this improves parallelism in any practical cases, honestly.

[TheBlueMatt]

Is there a race condition here where we decide whether or not to include the channel at this step but then the channel gets updated and we write the wrong number of channels? Doesn't seem like it was introduced in this PR but still worth fixing.

[tnull]

Yeah, I noticed that as well and created a TODO for it but so far intended to fix it elsewhere. Looking at it again it seems that this could be fixed by a) only iterating the peer states once and persisting to a temporary buffer while keeping track of serializable_peer_count and number_of_funded_channels or b) taking the per_peer_state.write() lock earlier and holding it for the remainder of the method.

Seems a) is rather inefficient and b) would mean dropping any chance of concurrent access during at least parts of write. I think switching to an RwLock might still be nice in general and going forward, but if you don't agree, possibly we should just do b) in a separate PR and close this one?

[TheBlueMatt]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

[tnull]

IIRC we discussed offline and concluded to abandon this in favor of fixing the issue at #2968 (comment) by holding the top-level write lock for longer. Leaving this open until we move that to an issue or open the followup pr.

Now opened to this effect: #2998

Closing this as the benefits of switching to RwLock are not immediately clear, and we can always do so if we find a compelling reason in the future.