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.
[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]
--
Sabrina
