On Saturday 19 December 2015 19:09:02 Antonio Quartulli wrote:
On 19/12/15 17:32, Sven Eckelmann wrote:
On Saturday 19 December 2015 09:04:05 Linus Lüssing wrote:
From: Simon Wunderlich simon@open-mesh.com
While testing, we got something like this:
WARNING: CPU: 0 PID: 238 at net/batman-adv/multicast.c:142 batadv_mcast_mla_tt_retract+0x94/0x205 [batman_adv]() [...] Call Trace: [<ffffffff815fc597>] dump_stack+0x4b/0x64 [<ffffffff810b34dc>] warn_slowpath_common+0xbc/0x120 [<ffffffffa0024ec5>] ? batadv_mcast_mla_tt_retract+0x94/0x205 [batman_adv] [<ffffffff810b3705>] warn_slowpath_null+0x15/0x20 [<ffffffffa0024ec5>] batadv_mcast_mla_tt_retract+0x94/0x205 [batman_adv] [<ffffffffa00273fe>] batadv_mcast_free+0x36/0x39 [batman_adv] [<ffffffffa0020c77>] batadv_mesh_free+0x7d/0x13f [batman_adv] [<ffffffffa0036a6b>] batadv_softif_free+0x15/0x25 [batman_adv] [...]
Signed-off-by: Simon Wunderlich simon@open-mesh.com
Fixes: 5b95c427d187 ("batman-adv: Annotate deleting functions with external lock via lockdep") Acked-by: Linus Lüssing linus.luessing@c0d3.blue
I am confused why this should fix a commit which is there to find such problems.
Am I wrong or also the commit message is misleading? Looks like it is "fixing" a bogus lockdep check...but as far as I can say this patch is making sure that the lock is held in that particular point.
No, it doesn't say this anywhere. It says that it fixes the "lockdep splat"
If we want to "fix the lockdep" then we should remove the lockdep_assert_held(), but I don't think this is the goal of the whole discussion.
No, the lockdep check should not be removed because a lock is necessary for other situations when this list is modified as explained earlier [1]. You have multiple concurrent calling contexts for this function (hard_if_event with something like mtu update + something like batadv_iv_ogm_schedule) which require exclusive access to this list. Right now it is just protected by a weird "named" lock (from the translation table and not from the mcast).
And no, the solution here is not to add a special codepath which (for no sane reason) requires no locks or synchronized frees - even when this seemed to have become the new trend in the batman-adv codebase.
Kind regards, Sven
[1] https://lists.open-mesh.org/pipermail/b.a.t.m.a.n/2015-December/013876.html