[ISSUE] Failed to find "enabled" option.
by Xin Long
I found this err in "loadbalance" and "lacp" runner when adding ports.
It's caused by trying to set "enabled" option in .port_link_changed()
or .port_changed().
When a new port is added, the first 'port changed event' process is
earlier than CMD TEAM_CMD_OPTIONS_GET, in this CMD, all
the options are synchronized from kernel.
It means there's no 'enabled' option yet when calling port_link_changed
in the first 'port changed event' process. In lb_event_watch_port_link_changed
and lacp_event_watch_port_changed, they call teamd_port_check_enable
to set 'enabled' option. this err is triggered.
I'm not sure why teamd_port_check_enable needs to check if
'enabled' option exists. I checked the ab's .port_link_changed(),
it just sets it by calling team_set_port_enabled(), instead of
checking 'enabled' option first.
can we just use team_set_port_enabled to set it directly in
.port(_link)_changed OR improve teamd_port_check_enable
to avoid this err ?
Thanks.
4 years, 2 months
[PATCHv2] teamd: add port_hwaddr_changed for ab runner
by Xin Long
This patch to fix an events processing race issue when adding two ports
into one team dev with ab mode with same_all hwaddr policy:
team0 original hwaddr: 00:00:00:00:00:0a
port1 original hwaddr: 00:00:00:00:00:01
port2 original hwaddr: 00:00:00:00:00:02
There are two sockets in teamd: nl_cli.sock_event for ifinfo updates
and nl_sock_event for ports/options changes. During adding two ports,
the events on these two sockets could be:
nl_sock_event:
[1] -- [2] --
[1]: port1 added event (added by enslaving port1)
[2]: port2 added event (added by enslaving port2)
nl_cli.sock_event:
[a1] -- [b0] -- [c1] -- [d2] -- [e2] -- [f1] --
[a1]: port1 ifinfo event (added by setting port1's master)
[b0]: team0 ifinfo event (added by setting team0's hwaddr)
[c1]: port1 ifinfo event (added by set port1's hwaddr)
[d2]: port2 ifinfo event (added by set port2's master)
[e2]: port2 ifinfo event (added by set port2's hwaddr)
[f1]: port1 ifinfo event (added by set port1's hwaddr)
teamd can make sure the order for their processing is as above on the
same socket, but not between two sockets. So if these events processing
order is (monitoring team/ports' ifinfo, hwaddr, master):
[ 1]: team0->ifinfo = 00:00:00:00:00:0a
team0->hwaddr = 00:00:00:00:00:01
port1->hwaddr = 00:00:00:00:00:0a
[a1]: port1->ifinfo = 00:00:00:00:00:01
port1->master = team0
[ 2]: port2->ifinfo = 00:00:00:00:00:02
port2->hwaddr = 00:00:00:00:00:0a
(team0->ifinfo is not updated, it's still 00:00:00:00:00:0a)
[b0]: team0->ifinfo = 00:00:00:00:00:01
port1->hwaddr = 00:00:00:00:00:01
(port2->master is not yet set, port2->hwaddr couldn't be updated)
[c1]: no changes
[d2]: port2->ifinfo = 00:00:00:00:00:0a
port2->master = team0
(too late !!!)
[e2]: no changes
[f1]: no changes
Then:
team0 final hwaddr: 00:00:00:00:00:01
port1 final hwaddr: 00:00:00:00:00:01
port2 final hwaddr: 00:00:00:00:00:0a <----- issue
This patch is to add port_hwaddr_changed for ab runner, in [e2] where
we set it's hwaddr with team0 (port2->hwaddr = 00:00:00:00:00:01) IF
port2->hwaddr != team0->ifinfo.
I think the same issue also exists in lacp and lb mode for which I will
fix them in another patches.
v1 -> v2:
fix some typos in changelog and couple of style problems in codes
Reported-by: Jon Nikolakakis <jnikolak(a)redhat.com>
Signed-off-by: Xin Long <lucien.xin(a)gmail.com>
---
teamd/teamd_runner_activebackup.c | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/teamd/teamd_runner_activebackup.c b/teamd/teamd_runner_activebackup.c
index aec3a73..8a3447f 100644
--- a/teamd/teamd_runner_activebackup.c
+++ b/teamd/teamd_runner_activebackup.c
@@ -39,6 +39,8 @@ struct ab_hwaddr_policy {
const char *name;
int (*hwaddr_changed)(struct teamd_context *ctx,
struct ab *ab);
+ int (*port_hwaddr_changed)(struct teamd_context *ctx, struct ab *ab,
+ struct teamd_port *tdport);
int (*port_added)(struct teamd_context *ctx, struct ab *ab,
struct teamd_port *tdport);
int (*active_set)(struct teamd_context *ctx, struct ab *ab,
@@ -95,6 +97,26 @@ static int ab_hwaddr_policy_same_all_hwaddr_changed(struct teamd_context *ctx,
return 0;
}
+static int
+ab_hwaddr_policy_same_all_port_hwaddr_changed(struct teamd_context *ctx,
+ struct ab *ab,
+ struct teamd_port *tdport)
+{
+ int err;
+
+ if (!memcmp(team_get_ifinfo_hwaddr(tdport->team_ifinfo),
+ ctx->hwaddr, ctx->hwaddr_len))
+ return 0;
+
+ err = team_hwaddr_set(ctx->th, tdport->ifindex, ctx->hwaddr,
+ ctx->hwaddr_len);
+ if (err)
+ teamd_log_err("%s: Failed to set port hardware address.",
+ tdport->ifname);
+
+ return err;
+}
+
static int ab_hwaddr_policy_same_all_port_added(struct teamd_context *ctx,
struct ab *ab,
struct teamd_port *tdport)
@@ -114,6 +136,7 @@ static int ab_hwaddr_policy_same_all_port_added(struct teamd_context *ctx,
static const struct ab_hwaddr_policy ab_hwaddr_policy_same_all = {
.name = "same_all",
.hwaddr_changed = ab_hwaddr_policy_same_all_hwaddr_changed,
+ .port_hwaddr_changed = ab_hwaddr_policy_same_all_port_hwaddr_changed,
.port_added = ab_hwaddr_policy_same_all_port_added,
};
@@ -411,6 +434,21 @@ static int ab_event_watch_hwaddr_changed(struct teamd_context *ctx, void *priv)
return 0;
}
+static int ab_event_watch_port_hwaddr_changed(struct teamd_context *ctx,
+ struct teamd_port *tdport,
+ void *priv)
+{
+ struct ab *ab = priv;
+
+ if (!teamd_port_present(ctx, tdport))
+ return 0;
+
+ if (ab->hwaddr_policy->port_hwaddr_changed)
+ return ab->hwaddr_policy->port_hwaddr_changed(ctx, ab, tdport);
+
+ return 0;
+}
+
static int ab_port_load_config(struct teamd_context *ctx,
struct ab_port *ab_port)
{
@@ -491,6 +529,7 @@ static int ab_event_watch_prio_option_changed(struct teamd_context *ctx,
static const struct teamd_event_watch_ops ab_event_watch_ops = {
.hwaddr_changed = ab_event_watch_hwaddr_changed,
+ .port_hwaddr_changed = ab_event_watch_port_hwaddr_changed,
.port_added = ab_event_watch_port_added,
.port_link_changed = ab_event_watch_port_link_changed,
.option_changed = ab_event_watch_prio_option_changed,
--
2.1.0
6 years, 1 month
[PATCH] teamd: do not process lacpdu before the port ifinfo is set
by Xin Long
Now the port ifinfo will be set in obj_input_newlink when a RTM_NEWLINK
event is received.
But when a port is being added, if a lacpdu gets received on this port
before the RTM_NEWLINK event, lacpdu_recv will process the packet with
incorrect port ifinfo.
In Patrick's case, as ifinfo->master_ifindex was 0, it would skip this
port in teamd_for_each_tdport, which caused lacp_port->agg_lead not to
be updated in lacp_switch_agg_lead. Later the lacp_port actor would go
to a unexpected state.
This patch is to avoid it by checking teamd_port_present in lacpdu_recv
so that it would not process lacpdu before the port ifinfo is set.
Reported-by: Patrick Talbert <ptalbert(a)redhat.com>
Tested-by: Patrick Talbert <ptalbert(a)redhat.com>
Signed-off-by: Xin Long <lucien.xin(a)gmail.com>
---
teamd/teamd_runner_lacp.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/teamd/teamd_runner_lacp.c b/teamd/teamd_runner_lacp.c
index 5601278..1310f67 100644
--- a/teamd/teamd_runner_lacp.c
+++ b/teamd/teamd_runner_lacp.c
@@ -1075,6 +1075,9 @@ static int lacpdu_recv(struct lacp_port *lacp_port)
if (err <= 0)
return err;
+ if (!teamd_port_present(lacp_port->ctx, lacp_port->tdport))
+ return 0;
+
if (!lacpdu_check(&lacpdu)) {
teamd_log_warn("malformed LACP PDU came.");
return 0;
--
2.1.0
6 years, 1 month
[PATCH] teamd: add port_hwaddr_changed for ab runner
by Xin Long
This patch to fix a events processing race issue when adding two ports
into one team dev with ab mode with same_all hwaddr policy:
team0 original hwaddr: 00:00:00:00:00:0a
port1 original hwaddr: 00:00:00:00:00:01
port2 original hwaddr: 00:00:00:00:00:02
There are two sockets in teamd: nl_cli.sock_event for ifinfo updates
and nl_sock_event for ports/options changes. Druing adding two ports,
the events on these two sockets could be:
nl_sock_event:
[1] -- [2] --
[1]: port1 added event (added by enslaving port1)
[2]: port2 added event (added by enslaving port2)
nl_cli.sock_event:
[a1] -- [b0] -- [c1] -- [d2] -- [e2] -- [f1] --
[a1]: port1 ifinfo event (added by setting port1's master)
[b0]: team0 ifinfo event (added by setting team0's hwaddr)
[c1]: port1 ifinfo event (added by set port1's hwaddr)
[d2]: port2 ifinfo event (added by set port2's master)
[e2]: port2 ifinfo event (added by set port2's hwaddr)
[f1]: port1 ifinfo event (added by set port1's hwaddr)
teamd can make sure the order for their processing is as above on the
same socket, but not between two sockets. So if these events processing
order is (monitoring team/ports' ifinfo, hwaddr, master):
[ 1]: team0->ifinfo = 00:00:00:00:00:0a
team0->hwaddr = 00:00:00:00:00:01
port1->hwaddr = 00:00:00:00:00:0a
[a1]: port1->ifinfo = 00:00:00:00:00:01
port1->master = team0
[ 2]: port2->ifinfo = 00:00:00:00:00:02
port2->hwaddr = 00:00:00:00:00:0a
(team0->ifinfo is not set updated, it's still 00:00:00:00:00:0a)
[b0]: team0->ifinfo = 00:00:00:00:00:01
port1->hwaddr = 00:00:00:00:00:01
(port2->master is not yet set, port2->hwaddr couldn't be updated)
[c1]: no changes
[d2]: port2->ifinfo = 00:00:00:00:00:0a
port2->master = team0
(too late !!!)
[e2]: no changes
[f1]: no changes
Then:
team0 final hwaddr: 00:00:00:00:00:01
port1 final hwaddr: 00:00:00:00:00:01
port2 final hwaddr: 00:00:00:00:00:0a <----- issue
This patch is to add port_hwaddr_changed for ab runner, in [e2] where
we set it's hwaddr with team0 (port2->hwaddr = 00:00:00:00:00:01) IF
port2->hwaddr != team0->ifinfo.
I think the same issue also exists in lacp and lb mode for which I will
fix them in another patches.
Reported-by: Jon Nikolakakis <jnikolak(a)redhat.com>
Signed-off-by: Xin Long <lucien.xin(a)gmail.com>
---
teamd/teamd_runner_activebackup.c | 39 +++++++++++++++++++++++++++++++++++++++
1 file changed, 39 insertions(+)
diff --git a/teamd/teamd_runner_activebackup.c b/teamd/teamd_runner_activebackup.c
index aec3a73..2745a4f 100644
--- a/teamd/teamd_runner_activebackup.c
+++ b/teamd/teamd_runner_activebackup.c
@@ -39,6 +39,8 @@ struct ab_hwaddr_policy {
const char *name;
int (*hwaddr_changed)(struct teamd_context *ctx,
struct ab *ab);
+ int (*port_hwaddr_changed)(struct teamd_context *ctx, struct ab *ab,
+ struct teamd_port *tdport);
int (*port_added)(struct teamd_context *ctx, struct ab *ab,
struct teamd_port *tdport);
int (*active_set)(struct teamd_context *ctx, struct ab *ab,
@@ -95,6 +97,26 @@ static int ab_hwaddr_policy_same_all_hwaddr_changed(struct teamd_context *ctx,
return 0;
}
+static int ab_hwaddr_policy_same_all_port_hwaddr_changed(
+ struct teamd_context *ctx,
+ struct ab *ab,
+ struct teamd_port *tdport)
+{
+ int err;
+
+ if (!memcmp(team_get_ifinfo_hwaddr(tdport->team_ifinfo),
+ ctx->hwaddr, ctx->hwaddr_len))
+ return 0;
+
+ err = team_hwaddr_set(ctx->th, tdport->ifindex, ctx->hwaddr,
+ ctx->hwaddr_len);
+ if (err)
+ teamd_log_err("%s: Failed to set port hardware address.",
+ tdport->ifname);
+
+ return err;
+}
+
static int ab_hwaddr_policy_same_all_port_added(struct teamd_context *ctx,
struct ab *ab,
struct teamd_port *tdport)
@@ -114,6 +136,7 @@ static int ab_hwaddr_policy_same_all_port_added(struct teamd_context *ctx,
static const struct ab_hwaddr_policy ab_hwaddr_policy_same_all = {
.name = "same_all",
.hwaddr_changed = ab_hwaddr_policy_same_all_hwaddr_changed,
+ .port_hwaddr_changed = ab_hwaddr_policy_same_all_port_hwaddr_changed,
.port_added = ab_hwaddr_policy_same_all_port_added,
};
@@ -411,6 +434,21 @@ static int ab_event_watch_hwaddr_changed(struct teamd_context *ctx, void *priv)
return 0;
}
+static int ab_event_watch_port_hwaddr_changed(struct teamd_context *ctx,
+ struct teamd_port *tdport,
+ void *priv)
+{
+ struct ab *ab = priv;
+
+ if (!teamd_port_present(ctx, tdport))
+ return 0;
+
+ if (ab->hwaddr_policy->port_hwaddr_changed)
+ return ab->hwaddr_policy->port_hwaddr_changed(ctx, ab, tdport);
+
+ return 0;
+}
+
static int ab_port_load_config(struct teamd_context *ctx,
struct ab_port *ab_port)
{
@@ -491,6 +529,7 @@ static int ab_event_watch_prio_option_changed(struct teamd_context *ctx,
static const struct teamd_event_watch_ops ab_event_watch_ops = {
.hwaddr_changed = ab_event_watch_hwaddr_changed,
+ .port_hwaddr_changed = ab_event_watch_port_hwaddr_changed,
.port_added = ab_event_watch_port_added,
.port_link_changed = ab_event_watch_port_link_changed,
.option_changed = ab_event_watch_prio_option_changed,
--
2.1.0
6 years, 1 month