On 22/11/2024 17:18, Sabrina Dubroca wrote:
2024-11-22, 10:41:26 +0100, Antonio Quartulli wrote:
On 12/11/2024 14:20, Antonio Quartulli wrote:
[...]
+static int ovpn_peer_del_nolock(struct ovpn_peer *peer,
+ enum ovpn_del_peer_reason reason)
+{
+ switch (peer->ovpn->mode) {
+ case OVPN_MODE_MP:
I think it would be nice to add
lockdep_assert_held(&peer->ovpn->peers->lock);
Sabrina, in other places I have used the sparse notation __must_hold()
instead.
Is there any preference in regards to lockdep vs sparse?
I could switch them all to lockdep_assert_held if needed.
__must_hold has the advantage of being checked at compile time (though
I'm not sure it's that reliable [1]), so you don't need to run a test
that actually hits that particular code path.
In this case I see lockdep_assert_held as mainly documenting that the
locking that makes ovpn_peer_del_nolock safe (as safe as
ovpn_peer_del) is provided by its caller. The splat for incorrect use
on debug kernels is a bonus. Sprinkling lockdep_assert_held all over
ovpn might be bloating the code too much, but I'm not opposed to
adding them if it helps.
[1] I ran sparse on drivers/net/ovpn/peer.c before/after removing the
locking from ovpn_peer_del and didn't get any warnings. sparse is good
to detect imbalances (function that locks without unlocking), but
maybe don't trust __must_hold for more than documenting expectations.
Same here. I didn't expect that.
Then I think it's better to rely on lockdep_assert_held() for this kind
of assumptions.
[note: if you end up merging ovpn->peers->lock with ovpn->lock as
we've discussed somewhere else, the locking around keepalive and
ovpn_peer_del becomes a bit less hairy]
Yeah, this is happening.
Thanks a lot!
Regards,
--
Antonio Quartulli
OpenVPN Inc.
