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;
Tue, Sep 01, 2020 at 07:59:27AM CEST, lucien.xin@gmail.com wrote:
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.
Done. Thanks!
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
Tue, Sep 01, 2020 at 07:59:27AM CEST, lucien.xin@gmail.com wrote:
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.
Hi Xin. Any progress with the fix? I would like to do a libteam release soon but I'm happy to wait couple of days if the fix is in pipes.
Thanks!
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
On Mon, Sep 14, 2020 at 4:06 PM Jiri Pirko jiri@resnulli.us wrote:
Tue, Sep 01, 2020 at 07:59:27AM CEST, lucien.xin@gmail.com wrote:
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.
Hi Xin. Any progress with the fix? I would like to do a libteam release soon but I'm happy to wait couple of days if the fix is in pipes.
Hi, Jiri,
I could never reproduce this issue by:
# ./mirror_gre_lag_lacp.sh veth{5..12}
especially after the kernel patch:
commit 1c0522b4a2e143fa6e55e4bd2308415c81184ec7 Author: Petr Machata <petrm at mellanox.com> Date: Fri May 29 14:16:53 2020 +0300
selftests: forwarding: mirror_lib: Use mausezahn
I will need to add some code to try to reproduce it. Give me another 2 days, I will let you know soon.
Thanks.
Thanks!
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
On Tue, Sep 15, 2020 at 1:39 PM Xin Long lucien.xin@gmail.com wrote:
On Mon, Sep 14, 2020 at 4:06 PM Jiri Pirko jiri@resnulli.us wrote:
Tue, Sep 01, 2020 at 07:59:27AM CEST, lucien.xin@gmail.com wrote:
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.
Hi Xin. Any progress with the fix? I would like to do a libteam release soon but I'm happy to wait couple of days if the fix is in pipes.
Hi, Jiri,
I could never reproduce this issue by:
# ./mirror_gre_lag_lacp.sh veth{5..12}
especially after the kernel patch:
commit 1c0522b4a2e143fa6e55e4bd2308415c81184ec7 Author: Petr Machata <petrm at mellanox.com> Date: Fri May 29 14:16:53 2020 +0300
selftests: forwarding: mirror_lib: Use mausezahn
I will need to add some code to try to reproduce it. Give me another 2 days, I will let you know soon.
Hi, Jiri
Just noticed that the original issue is exactly the same one fixed by:
commit 90e5279ce0241838d5687739b3bc795235b7d53b Author: Xin Long lucien.xin@gmail.com Date: Mon Apr 15 16:56:35 2019 +0800
teamd: use enabled option_changed to sync enabled to link_up for lb runner
pls see the changelog. You can go with the release now.
Thanks.
libteam@lists.fedorahosted.org