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

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