[PATCH] Revert "teamd: Disregard current state when considering port enablement"
by Xin Long
This reverts commit deadb5b715227429a1879b187f5906b39151eca9.
As Patrick noticed, with that commit, teamd_port_check_enable()
would set the team port to the new state unconditionally, which
triggers another change message from kernel to userspace, then
teamd_port_check_enable() is called again to set the team port
to the new state.
This would go around and around to update the team port state,
and even cause teamd to consume 100% cpu.
As the issue caused by that commit is serious, it has to be
reverted. As for the issued fixed by that commit, I would
propose a new fix later.
---
teamd/teamd_per_port.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/teamd/teamd_per_port.c b/teamd/teamd_per_port.c
index 166da57..d429753 100644
--- a/teamd/teamd_per_port.c
+++ b/teamd/teamd_per_port.c
@@ -442,14 +442,18 @@ int teamd_port_check_enable(struct teamd_context *ctx,
bool should_enable, bool should_disable)
{
bool new_enabled_state;
+ bool curr_enabled_state;
int err;
if (!teamd_port_present(ctx, tdport))
return 0;
+ err = teamd_port_enabled(ctx, tdport, &curr_enabled_state);
+ if (err)
+ return err;
- if (should_enable)
+ if (!curr_enabled_state && should_enable)
new_enabled_state = true;
- else if (should_disable)
+ else if (curr_enabled_state && should_disable)
new_enabled_state = false;
else
return 0;
--
2.1.0
3 years, 2 months
Re: [PATCH] teamd: Disregard current state when considering port enablement
by Xin Long
On Wed, Sep 2, 2020 at 9:25 PM Petr Machata <petrm(a)nvidia.com> wrote:
>
>
> Xin Long <lucien.xin(a)gmail.com> writes:
>
> > do you have a reproducer for this race issue?
>
> I think they were spurious failures in
> tools/testing/selftests/net/forwarding/mirror_gre_lag_lacp.sh run on a
> simulated switch under qemu. This might work as an approximation:
>
> ./mirror_gre_lag_lacp.sh veth{5..12}
>
Hi, Petr, I couldn't reproduce it on my env.
Can you please try with this kernel commit?
commit 1c0522b4a2e143fa6e55e4bd2308415c81184ec7
Author: Petr Machata <petrm at mellanox.com>
Date: Fri May 29 14:16:53 2020 +0300
selftests: forwarding: mirror_lib: Use mausezahn
Thanks.
3 years, 2 months
Re: [PATCH] teamd: Disregard current state when considering port
enablement
by Jiri Pirko
Wed, Nov 13, 2019 at 02:26:47PM CET, petrm(a)mellanox.com wrote:
>On systems where carrier is gained very quickly, there is a race between
>teamd and the kernel that sometimes leads to all team slaves being stuck in
>enabled=false state.
>
>When a port is enslaved to a team device, the kernel sends a netlink
>message marking the port as enabled. teamd's lb_event_watch_port_added()
>calls team_set_port_enabled(false), because link is down at that point. The
>kernel responds with a message marking the port as disabled. At this point,
>there are two outstanding messages: the initial one marking port as
>enabled, and the second one marking it as disabled. teamd has not processed
>either of these.
>
>Next teamd gets the netlink message that sets enabled=true, and updates its
>internal cache accordingly. If at this point ethtool link-watch wakes up,
>teamd considers (in teamd_port_check_enable()) enabling the port. After
>consulting the cache, it concludes the port is already up, and neglects to
>do so. Only then does teamd get the netlink message informing it of setting
>enabled=false.
>
>The problem is that the teamd cache is not synchronous with respect to the
>kernel state. If the carrier takes a while to come up (as is normally the
>case), this is not a problem, because teamd caches up quickly enough. But
>this may not always be the case, and particularly on a simulated system,
>the carrier is gained almost immediately.
>
>Fix this by not suppressing the enablement message.
>
>Signed-off-by: Petr Machata <petrm(a)mellanox.com>
applied. Thanks!
3 years, 2 months