Re: [PATCH net v2] net: team: get rid of team->lock in team module

[Jiri Pirko <jiri@resnulli.us>]

Sat, Sep 16, 2023 at 03:11:15PM CEST, ap420073@gmail.com wrote:
>The purpose of team->lock is to protect the private data of the team
>interface. But RTNL already protects it all well.
>The precise purpose of the team->lock is to reduce contention of
>RTNL due to GENL operations such as getting the team port list, and
>configuration dump.
>
>team interface has used a dynamic lockdep key to avoid false-positive
>lockdep deadlock detection. Virtual interfaces such as team usually
>have their own lock for protecting private data.
>These interfaces can be nested.
>team0
>  |
>team1
>
>Each interface's lock is actually different(team0->lock and team1->lock).
>So,
>mutex_lock(&team0->lock);
>mutex_lock(&team1->lock);
>mutex_unlock(&team1->lock);
>mutex_unlock(&team0->lock);
>The above case is absolutely safe. But lockdep warns about deadlock.
>Because the lockdep understands these two locks are same. This is a
>false-positive lockdep warning.
>
>So, in order to avoid this problem, the team interfaces started to use
>dynamic lockdep key. The false-positive problem was fixed, but it
>introduced a new problem.
>
>When the new team virtual interface is created, it registers a dynamic
>lockdep key(creates dynamic lockdep key) and uses it. But there is the
>limitation of the number of lockdep keys.
>So, If so many team interfaces are created, it consumes all lockdep keys.
>Then, the lockdep stops to work and warns about it.

What about fixing the lockdep instead? I bet this is not the only
occurence of this problem.


>
>So, in order to fix this issue, It just removes team->lock and uses
>RTNL instead.
>
>The previous approach to fix this issue was to use the subclass lockdep
>key instead of the dynamic lockdep key. It requires RTNL before acquiring
>a nested lock because the subclass variable(dev->nested_lock) is
>protected by RTNL.
>However, the coverage of team->lock is too wide so sometimes it should
>use a subclass variable before initialization.
>So, it can't work well in the port initialization and unregister logic.
>
>This approach is just removing the team->lock clearly.
>So there is no special locking scenario in the team module.
>Also, It may convert RTNL to RCU for the read-most operations such as
>GENL dump but not yet adopted.
>
>Reproducer:
>   for i in {0..1000}
>   do
>           ip link add team$i type team
>           ip link add dummy$i master team$i type dummy
>           ip link set dummy$i up
>           ip link set team$i up
>   done
>