On 04/03/2025 15:35, Sabrina Dubroca wrote:
2025-03-04, 01:33:49 +0100, Antonio Quartulli wrote:
@@ -1317,11 +1336,16 @@ void ovpn_peer_keepalive_work(struct work_struct *work)
/* prevent rearming if the interface is being destroyed */
if (next_run > 0 && ovpn->registered) {
+ time64_t delta = next_run - now;
+
netdev_dbg(ovpn->dev,
"scheduling keepalive work: now=%llu next_run=%llu delta=%llu\n",
- next_run, now, next_run - now);
+ next_run, now, delta > 0 ? delta : 0);
+ /* due to the waiting above, the next_run deadline may have
+ * passed: in this case we reschedule the worker immediately
+ */
I don't understand this bit. I don't see what waiting you're refering
to (in particular within this patch), and I don't see how we could get
next_run < now based on how next_run is computed in
ovpn_peer_keepalive_work_single (next_run1/next_run2 is always set to
now + X or something that we just tested to be > now).
Am I missing something?
Sorry - this is just noise.
Before getting to this final solution, I went for an intermediate
implementation where I was calling wait_for_completion() on a list of
removed peers (this is "the waiting above" the comment refers to).
Since I had no clue how long I waited, I had to update 'now'.
But at that point there was a chance that we had passed the deadline for
the next run, hence ending up with a negative delta.
This said, all this logic was thrown away, but I accidentally kept the
check on delta (that I even argued about in a previous email when you
saw the bogus ternary-op)
so yeah, the ternary-op can go away.
Thanks for double/triple checking with me.
Cheers,
schedule_delayed_work(&ovpn->keepalive_work,
- (next_run - now) * HZ);
+ delta * HZ > 0 ? delta * HZ : 0);
}
unlock_ovpn(ovpn, &release_list);
}
--
Antonio Quartulli
OpenVPN Inc.
