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
[patch libteam] teamd: fix possible race in master ifname callback
by Jiri Pirko
From: Jiri Pirko <jiri(a)mellanox.com>
In teamd the messages from kernel are processed in order:
options
ifinfo
ports
However, kernel sends the messages in a different order. See:
team_upper_dev_link() which generates ifinfo notification
__team_port_change_port_added() which generates ports notification
__team_options_change_check() which generates options notification
So there is a chance that the teamd processed only the port without
options and right after that it processes ifinfo.
Fix this by introducing teamd_port_enabled_check() which does not log
error and ignore the port in case this call fails. This is safe
as this is going to be handled by future
link_watch_enabled_option_changed() call.
Signed-off-by: Jiri Pirko <jiri(a)mellanox.com>
Fixes: 5f351666409f ("teamd: add port_master_ifindex_changed for link_watch_port_watch_ops")
Tested-by: Stepan Blyshchak <stepanb(a)mellanox.com>
---
teamd/teamd.h | 2 ++
teamd/teamd_link_watch.c | 13 ++++++++++---
teamd/teamd_per_port.c | 24 +++++++++++++++++++-----
3 files changed, 31 insertions(+), 8 deletions(-)
diff --git a/teamd/teamd.h b/teamd/teamd.h
index fb2872efd0ac..f94c918cfe52 100644
--- a/teamd/teamd.h
+++ b/teamd/teamd.h
@@ -325,6 +325,8 @@ int teamd_port_remove_all(struct teamd_context *ctx);
void teamd_port_obj_remove_all(struct teamd_context *ctx);
int teamd_port_enabled(struct teamd_context *ctx, struct teamd_port *tdport,
bool *enabled);
+int teamd_port_enabled_check(struct teamd_context *ctx,
+ struct teamd_port *tdport, bool *enabled);
int teamd_port_prio(struct teamd_context *ctx, struct teamd_port *tdport);
int teamd_port_check_enable(struct teamd_context *ctx,
struct teamd_port *tdport,
diff --git a/teamd/teamd_link_watch.c b/teamd/teamd_link_watch.c
index 5c626eb3e636..cae6549d613a 100644
--- a/teamd/teamd_link_watch.c
+++ b/teamd/teamd_link_watch.c
@@ -423,9 +423,16 @@ static int link_watch_refresh_forced_send(struct teamd_context *ctx)
int err;
teamd_for_each_tdport(tdport, ctx) {
- err = teamd_port_enabled(ctx, tdport, &port_enabled);
- if (err)
- return err;
+ err = teamd_port_enabled_check(ctx, tdport, &port_enabled);
+ if (err) {
+ /* Looks like the options are not ready for this port.
+ * This can happen when called from
+ * link_watch_port_master_ifindex_changed(). Skip this
+ * for now, let it be handled by future call of
+ * link_watch_enabled_option_changed().
+ */
+ continue;
+ }
__set_forced_send_for_port(tdport, port_enabled);
if (port_enabled)
enabled_port_count++;
diff --git a/teamd/teamd_per_port.c b/teamd/teamd_per_port.c
index 327aefbe9a44..166da576ef7c 100644
--- a/teamd/teamd_per_port.c
+++ b/teamd/teamd_per_port.c
@@ -389,19 +389,21 @@ int teamd_port_remove_ifname(struct teamd_context *ctx, const char *port_name)
return teamd_port_remove(ctx, tdport);
}
-int teamd_port_enabled(struct teamd_context *ctx, struct teamd_port *tdport,
- bool *enabled)
+int __teamd_port_enabled(struct teamd_context *ctx, struct teamd_port *tdport,
+ bool *enabled, bool may_fail)
{
struct team_option *option;
option = team_get_option(ctx->th, "np", "enabled", tdport->ifindex);
if (!option) {
- teamd_log_err("%s: Failed to find \"enabled\" option.",
- tdport->ifname);
+ if (!may_fail)
+ teamd_log_err("%s: Failed to find \"enabled\" option.",
+ tdport->ifname);
return -ENOENT;
}
if (team_get_option_type(option) != TEAM_OPTION_TYPE_BOOL) {
- teamd_log_err("Unexpected type of \"enabled\" option.");
+ if (!may_fail)
+ teamd_log_err("Unexpected type of \"enabled\" option.");
return -EINVAL;
}
@@ -409,6 +411,18 @@ int teamd_port_enabled(struct teamd_context *ctx, struct teamd_port *tdport,
return 0;
}
+int teamd_port_enabled(struct teamd_context *ctx, struct teamd_port *tdport,
+ bool *enabled)
+{
+ return __teamd_port_enabled(ctx, tdport, enabled, false);
+}
+
+int teamd_port_enabled_check(struct teamd_context *ctx,
+ struct teamd_port *tdport, bool *enabled)
+{
+ return __teamd_port_enabled(ctx, tdport, enabled, true);
+}
+
int teamd_port_prio(struct teamd_context *ctx, struct teamd_port *tdport)
{
int prio;
--
2.21.0
3 years, 10 months
Re: [patch libteam] poll instead of select
by Jiri Pirko
Thu, Jan 30, 2020 at 08:29:22PM CET, jerome99(a)internet.lu wrote:
>Hello,
>
>A firewall application is using libteamdctl to read the configuration of the
>bonding interfaces. This application has a lot of open file descriptors and
>is therefore crashing in the libteadmdctl at this select function.
>
>The select function is a very old function and is using the FD_(*) macros to
>the set the bit of monitored file descriptors in a fixed size array. In the
>glibc this array can only contain 1024 entries ( file descritor 0->1023).
>
>The select function can therefore only monitor the first 1024 file
>descriptors. If the used file descriptor used in the select function is
>greater then 1023, the select can/will crash.
Fair enough, I'm applying this.
Thanks!
>
>
>
>Sincerely,
>Jérôme
>
>On 1/29/20 6:16 PM, Jiri Pirko wrote:
>> Tue, Jan 28, 2020 at 01:11:00AM CET, jerome99(a)internet.lu wrote:
>> > The select function cannot be used in application if the application has
>> > already more than 1024 open files. The select will crash if an file
>> > descriptor greater or equal than 1023 is monitored.
>> Okay, how we can come close that?
>>
>> > Signed-off-by: Jerome Freilinger <jerome99(a)internet.lu>
>> > ---
>> > libteamdctl/cli_usock.c | 25 +++++++++----------------
>> > 1 file changed, 9 insertions(+), 16 deletions(-)
>> >
>> > diff --git a/libteamdctl/cli_usock.c b/libteamdctl/cli_usock.c
>> > index 0dc97ae..431b12d 100644
>> > --- a/libteamdctl/cli_usock.c
>> > +++ b/libteamdctl/cli_usock.c
>> > @@ -25,6 +25,7 @@
>> > #include <sys/socket.h>
>> > #include <unistd.h>
>> > #include <teamdctl.h>
>> > +#include <poll.h>
>> > #include "teamdctl_private.h"
>> > #include "../teamd/teamd_usock_common.h"
>> >
>> > @@ -79,26 +80,18 @@ static int cli_usock_send(int sock, char *msg)
>> > return 0;
>> > }
>> >
>> > -#define WAIT_SEC (TEAMDCTL_REPLY_TIMEOUT / 1000)
>> > -#define WAIT_USEC (TEAMDCTL_REPLY_TIMEOUT % 1000 * 1000)
>> > -
>> > static int cli_usock_wait_recv(int sock)
>> > {
>> > - fd_set rfds;
>> > - int fdmax;
>> > - int ret;
>> > - struct timeval tv;
>> > + struct pollfd fds[1];
>> > +
>> > + fds[0].fd = sock;
>> > + fds[0].events = POLLIN;
>> > + fds[0].revents = 0;
>> > + int ret = poll(fds, 1, TEAMDCTL_REPLY_TIMEOUT);
>> >
>> > - tv.tv_sec = WAIT_SEC;
>> > - tv.tv_usec = WAIT_USEC;
>> > - FD_ZERO(&rfds);
>> > - FD_SET(sock, &rfds);
>> > - fdmax = sock + 1;
>> > - ret = select(fdmax, &rfds, NULL, NULL, &tv);
>> > - if (ret == -1)
>> > - return -errno;
>> > - if (!FD_ISSET(sock, &rfds))
>> > + if (ret == 0)
>> > return -ETIMEDOUT;
>> > + else if (ret < 0)
>> > + return -errno;
>> > return 0;
>> > }
>> >
>> > --
>> > 2.20.1
>> >
>
3 years, 10 months