Only mark timers to update #3

Hasan6979 · 2024-11-28T15:00:12Z

Timers are updated on every single packet call, but their accuracy is limited to 250ms where currentTime is updated. So just mark timers to update, and update them only once in update_timers.

This PR is part 1 of several PR's to revamp the neptun noise logic. The next PR would remove the lock from the peer.tunnel and introduce interior locks to tunnel members instead. In the hotpath, we would only be reading the session so no need to lock the entire tunnel for that. Other members of tunnel are either lockable or can be made atomic. Locking timers internally is not possible, otherwise it can lead to a deadlock as update_timers also tries to lock() the timers then.

tomaszklak · 2024-11-29T09:51:37Z

Please add some more detail to the second part:

This is also needed in order to have a lock free tunnel, because locking timers internally is not possible, otherwise it can lead to a deadlock as update_timers also tries to lock() the timers then.

I don't fully understand this.

tomaszklak · 2024-11-29T09:42:21Z

neptun/src/noise/timers.rs

+        let timer_mask = self
+            .timers
+            .timers_to_update_mask
+            .load(std::sync::atomic::Ordering::Relaxed);
+        for timer_name in TimerName::VALUES {
+            if (timer_mask & (1 << (timer_name as u16))) != 0 {
+                self.timer_tick(timer_name);
+            }
+        }
+        // Reset all marked bits
+        self.timers
+            .timers_to_update_mask
+            .store(0, std::sync::atomic::Ordering::Relaxed);


I think this will miss mark_timer_to_update calls that happen during the for loop. I think you want to call let timer_mask = self.timers.timers_to_update_mask.swap(0, Relaxed) in the beginning.

If I'm correct, please also add a test that could catch this issue.

Changed and added test

I don't understand how this test, verifies the fix for the above issue. The issue happens only when the for loop (now in tick_marked_timers) runs in parallel with mark_timer_to_update. Also, reverting the swap change from implementation, doesn't break the test - please add a test that fails with the old version of the code, but passes with the new one.

Yes, but because of a bit difficulty for running them in parallel to make sure the issue happens, I decided to mark the timer after reading the bit mask. That timer should be updated in the next cycle of update timers, but in previous version that update was being missed.

neptun/src/noise/timers.rs

packgron

+1.0

packgron · 2024-12-09T11:13:30Z

neptun/src/noise/timers.rs

+        my_tun.update_timers(&mut [0]);
+
+        // Only those timers marked should be udpated
+        assert!(!my_tun.timers[TimerName::TimeLastDataPacketSent].is_zero());


NIT: use assert_eq!(timers[x], timers[TimeCurrent])

Hasan6979 · 2024-12-09T13:54:29Z

Performance test from base

ID	Role	Interval	Transfer	Bitrate	Retransmissions
5	Sender	0.00-120.00 sec	5.17 GBytes	370 Mbits/sec	40,779
5	Receiver	0.00-120.00 sec	5.17 GBytes	370 Mbits/sec	N/A
7	Sender	0.00-120.00 sec	5.21 GBytes	373 Mbits/sec	42,817
7	Receiver	0.00-120.00 sec	5.21 GBytes	373 Mbits/sec	N/A

Performance of current branch

ID	Role	Interval	Transfer	Bitrate	Retransmissions
5	Sender	0.00-120.00 sec	5.24 GBytes	375 Mbits/sec	43,217
5	Receiver	0.00-120.00 sec	5.24 GBytes	375 Mbits/sec	N/A
7	Sender	0.00-120.00 sec	5.08 GBytes	364 Mbits/sec	41,078
7	Receiver	0.00-120.00 sec	5.08 GBytes	364 Mbits/sec	N/A

jjanowsk · 2024-12-17T09:33:20Z

I've done a ton of testing yesterday on my setup (two physical machines connected directly via cable). Unsurprisingly the most consistent results I get when measuring only one way UDP traffic. On multiple 6 minute runs I got an average of 684 Mbps with this branch vs 662 Mbps with the main branch. Looks like 3% improvement which is nice.

Hasan6979 · 2024-12-17T09:46:26Z

@jjanowsk I think this amount is good enough because I would expect only a marginal gain, if any, by this change. Either way this change is needed for future improvements.

tomaszklak · 2024-12-18T09:45:06Z

neptun/src/noise/timers.rs

@@ -82,6 +96,7 @@ pub struct Timers {
    persistent_keepalive: usize,
    /// Should this timer call reset rr function (if not a shared rr instance)
    pub(super) should_reset_rr: bool,
+    timers_to_update_mask: AtomicU16,


do we need AtomicU16 here? can we use instead the parking_lot::Mutex<u16> or even parking_lot::Mutex<[bool;8]>?

So this is main area that should be bringing improment, as then we are just swtiching from a mutex to a mutex.
Now we fundamentaly move away from any wait for lock's.

This change is adding atomicu16, not removing any mutex

Oh right, it's not with self as &self, but &mut self.
We don't even need Mutex or Atomic then in reality, just u16 would be enough

Timers are updated on every single packet call, but their accuracy is limited to 250ms where currentTime is updated. So just mark timers to update, and update them only once in update_timers.

jjanowsk · 2024-12-19T13:05:20Z

More updates from the measurements on the CI. When run in the CI on 15 minute runs of single way UDP traffic the gains are not significant. There is less then 0.5% of the difference between main and this branch, sometimes main being faster and sometimes this branch. In total I've run 4 pipelines like this.

tomaszklak · 2024-12-19T13:13:27Z

Traffic the gains are not significant. There is less then 0.5% of the difference between main and this branch, sometimes main being faster and sometimes this branch.

In that case, maybe we can close this PR?

Hasan6979 force-pushed the LLT-5805_update_timers_periodically branch from 5e1292c to 8092b80 Compare November 28, 2024 15:35

tomaszklak reviewed Nov 29, 2024

View reviewed changes

packgron reviewed Dec 9, 2024

View reviewed changes

Hasan6979 force-pushed the LLT-5805_update_timers_periodically branch from 4f1d22c to 4ba9f5e Compare December 9, 2024 13:36

Hasan6979 force-pushed the LLT-5805_update_timers_periodically branch from 4326a1f to e64f7d8 Compare December 18, 2024 08:56

tomaszklak reviewed Dec 18, 2024

View reviewed changes

Hasan6979 added 2 commits December 18, 2024 12:05

Only mark timers to update

a9b31e7

Timers are updated on every single packet call, but their accuracy is limited to 250ms where currentTime is updated. So just mark timers to update, and update them only once in update_timers.

Add test for update timers

d4e74e9

Hasan6979 force-pushed the LLT-5805_update_timers_periodically branch from e64f7d8 to d4e74e9 Compare December 18, 2024 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only mark timers to update #3

Only mark timers to update #3

Hasan6979 commented Nov 28, 2024 •

edited

Loading

tomaszklak commented Nov 29, 2024

tomaszklak Nov 29, 2024

Hasan6979 Dec 5, 2024

tomaszklak Dec 5, 2024

Hasan6979 Dec 5, 2024

packgron left a comment

packgron Dec 9, 2024

Hasan6979 commented Dec 9, 2024 •

edited

Loading

jjanowsk commented Dec 17, 2024

Hasan6979 commented Dec 17, 2024

tomaszklak Dec 18, 2024 •

edited

Loading

packgron Dec 18, 2024

tomaszklak Dec 18, 2024

packgron Dec 19, 2024

jjanowsk commented Dec 19, 2024

tomaszklak commented Dec 19, 2024

Only mark timers to update #3

Are you sure you want to change the base?

Only mark timers to update #3

Conversation

Hasan6979 commented Nov 28, 2024 • edited Loading

tomaszklak commented Nov 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

packgron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hasan6979 commented Dec 9, 2024 • edited Loading

jjanowsk commented Dec 17, 2024

Hasan6979 commented Dec 17, 2024

tomaszklak Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjanowsk commented Dec 19, 2024

tomaszklak commented Dec 19, 2024

Hasan6979 commented Nov 28, 2024 •

edited

Loading

Hasan6979 commented Dec 9, 2024 •

edited

Loading

tomaszklak Dec 18, 2024 •

edited

Loading