[PATCH 2/2] alfred: Switch to epoll for socket handling
The select syscall has various problems when working with many sockets:
* only a fd less than FD_SETSIZE (usually 1024) is supported
* it is necessary to register all relevant fds manually before each select
call
* when events are received, all the fds must be checked again the returned
data
This can be solved by using epoll. File descriptors are only registered
once and only the relevant file descriptors are returned. And epoll will
also take care of walking in a round-robin fashion through all relevant
file descriptors in case a lot of file descriptors could be returned by the
kernel.
Signed-off-by: Sven Eckelmann
---
alfred.h | 20 --
batadv_querynl.c | 4 --
epoll_handle.h | 25 +++
netsock.c| 171 ++-
server.c | 127 ---
unix_sock.c | 59 ++--
6 files changed, 243 insertions(+), 163 deletions(-)
create mode 100644 epoll_handle.h
diff --git a/alfred.h b/alfred.h
index 2679515..4839c85 100644
--- a/alfred.h
+++ b/alfred.h
@@ -16,9 +16,10 @@
#include
#include
#include
-#include
+#include
#include
#include "bitops.h"
+#include "epoll_handle.h"
#include "list.h"
#include "packet.h"
@@ -30,6 +31,10 @@
#define ALFRED_SOCK_PATH_DEFAULT "/var/run/alfred.sock"
#define NO_FILTER -1
+#ifndef __unused
+#define __unused __attribute__((unused))
+#endif
+
#define FIXED_TLV_LEN(__tlv_type) \
htons(sizeof(__tlv_type) - sizeof((__tlv_type).header))
@@ -101,9 +106,12 @@ struct interface {
alfred_addr address;
uint32_t scope_id;
char *interface;
+
int netsock;
int netsock_mcast;
- int netsock_arp;
+
+ struct epoll_handle netsock_epoll;
+ struct epoll_handle netsock_mcast_epoll;
struct hashtable_t *server_hash;
@@ -124,9 +132,13 @@ struct globals {
uint8_t ipv4mode:1;
uint8_t force:1;
+ int epollfd;
+
int check_timerfd;
+ struct epoll_handle check_epoll;
int unix_sock;
+ struct epoll_handle unix_epoll;
const char *unix_path;
const char *update_command;
@@ -182,7 +194,6 @@ int sync_data(struct globals *globals);
ssize_t send_alfred_packet(struct globals *globals, struct interface
*interface,
const alfred_addr *dest, void *buf, int length);
/* unix_sock.c */
-int unix_sock_read(struct globals *globals);
int unix_sock_open_daemon(struct globals *globals);
int unix_sock_open_client(struct globals *globals);
int unix_sock_close(struct globals *globals);
@@ -197,9 +208,6 @@ void netsock_close_all(struct globals *globals);
int netsock_set_interfaces(struct globals *globals, char *interfaces);
struct interface *netsock_first_interface(struct globals *globals);
void netsock_reopen(struct globals *globals);
-int netsock_prepare_select(struct globals *globals, fd_set *fds, int maxsock);
-void netsock_check_error(struct globals *globals, fd_set *errfds);
-int netsock_receive_packet(struct globals *globals, fd_set *fds);
int netsock_own_address(const struct globals *globals,
const alfred_addr *address);
/* util.c */
diff --git a/batadv_querynl.c b/batadv_querynl.c
index 872cb85..7c1b115 100644
--- a/batadv_querynl.c
+++ b/batadv_querynl.c
@@ -26,10 +26,6 @@
#include "batadv_query.h"
#include "netlink.h"
-#ifndef __unused
-#define __unused __attribute__((unused))
-#endif
-
static const int translate_mac_netlink_mandatory[] = {
BATADV_ATTR_TT_ADDRESS,
BATADV_ATTR_ORIG_ADDRESS,
diff --git a/epoll_handle.h b/epoll_handle.h
new file mode 100644
index 000..4ec69b9
--- /dev/null
+++ b/epoll_handle.h
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (C) B.A.T.M.A.N. contributors:
+ *
+ * Sven Eckelmann
+ *
+ * License-Filename: LICENSES/preferred/GPL-2.0
+ */
+
+#ifndef _ALFRED_EPOLL_HANDLE_H
+#define _ALFRED_EPOLL_HANDLE_H
+
+#include
+
+struct globals;
+struct epoll_handle;
+
+typedef void (*epoll_handler)(struct globals *globals,
+ struct epoll_handle *handle,
+ struct epoll_event *ev);
+
+struct epoll_handle {
+ epoll_handler handler;
+};
+
+#endif /* _ALFRED_EPOLL_HANDLE_H */
diff --git a/netsock.c b/netsock.c
index 128e768..feed21d 100644
--- a/netsock.c
+++ b/netsock.c
@@ -20,7 +20,7 @@
#include
#include
#include
-#include
+#include
#ifdef CONFIG_ALFRED_CAPABILITIES
#include
#endif
@@ -203,12 +203,59 @@ out:
return ret;
}
-static int netsock_open(struct interface *interface)
+static void netsock_close_error(struct interface *interface)
+{
+ fprintf(stderr, "Error on netsock detected\n");
+
+ if (interface->netsock >= 0)
+ close(interface->netsock);
+
+ if (interface->netsock_mcast >= 0)
+ close(interface->netsock_mcast);
+
+ interface->netsock = -1;
+
[PATCH 1/2] alfred: Stabilize synchronization period using timerfd
The current way of scheduling the synchronization related events tends to
cause drift from a perfect periodic timer. This happens because it doesn't
calculate the next event based on a fixed start time + period * the number
of past synchronization periods. Instead, the next event was scheduled
based on the time before the last select() - ignoring that non-zero time
was spend processing events.
For a 10 second period, this usually looks somthing like:
[24.043904208] announce primary ...
[34.044216187] announce primary ...
[44.053485658] announce primary ...
[54.063562062] announce primary ...
[64.073517069] announce primary ...
To avoid this drift, just use timerfd as rather stable periodic timer event
sources. It also has the benefit of making it easier to use multiple
periodic timers with different periods.
Only some small jitter can be seen with this external timer implementation:
[12.673756426] announce primary ...
[22.673779811] announce primary ...
[32.673778362] announce primary ...
[42.673775216] announce primary ...
Signed-off-by: Sven Eckelmann
---
alfred.h | 2 ++
server.c | 89 +++-
2 files changed, 58 insertions(+), 33 deletions(-)
diff --git a/alfred.h b/alfred.h
index 2d98a30..2679515 100644
--- a/alfred.h
+++ b/alfred.h
@@ -124,6 +124,8 @@ struct globals {
uint8_t ipv4mode:1;
uint8_t force:1;
+ int check_timerfd;
+
int unix_sock;
const char *unix_path;
diff --git a/server.c b/server.c
index bfc37bc..b5ec7b2 100644
--- a/server.c
+++ b/server.c
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -366,17 +367,44 @@ static void execute_update_command(struct globals
*globals)
free(command);
}
+static int create_sync_period_timer(struct globals *globals)
+{
+ struct itimerspec sync_timer;
+ int ret;
+
+ globals->check_timerfd = timerfd_create(CLOCK_MONOTONIC, TFD_CLOEXEC);
+ if (globals->check_timerfd < 0) {
+ perror("Failed to create periodic timer");
+ return -1;
+ }
+
+ sync_timer.it_value = globals->sync_period;
+ sync_timer.it_interval = globals->sync_period;
+
+ ret = timerfd_settime(globals->check_timerfd, 0, _timer, NULL);
+ if (ret < 0) {
+ perror("Failed to arm synchronization timer");
+ return -1;
+ }
+
+ return 0;
+}
+
int alfred_server(struct globals *globals)
{
int maxsock, ret, recvs;
- struct timespec last_check, now, tv;
+ struct timespec now;
fd_set fds, errfds;
size_t num_interfaces;
+ uint64_t timer_exp;
int num_socks;
if (create_hashes(globals))
return -1;
+ if (create_sync_period_timer(globals))
+ return -1;
+
if (unix_sock_open_daemon(globals))
return -1;
@@ -414,25 +442,10 @@ int alfred_server(struct globals *globals)
return -1;
}
- clock_gettime(CLOCK_MONOTONIC, _check);
- globals->if_check = last_check;
+ clock_gettime(CLOCK_MONOTONIC, );
+ globals->if_check = now;
while (1) {
- clock_gettime(CLOCK_MONOTONIC, );
-
- /* subtract the synchronization period from the current time
-* NOTE: this is an atypical usage of time_diff as it ignores
the return
-* value and store the result back into now, essentially
performing the
-* operation:
-* now -= globals->sync_period;
-*/
- time_diff(, >sync_period, );
-
- if (!time_diff(_check, , )) {
- tv.tv_sec = 0;
- tv.tv_nsec = 0;
- }
-
netsock_reopen(globals);
FD_ZERO();
@@ -440,10 +453,14 @@ int alfred_server(struct globals *globals)
FD_SET(globals->unix_sock, );
maxsock = globals->unix_sock;
+ FD_SET(globals->check_timerfd, );
+ if (maxsock < globals->check_timerfd)
+ maxsock = globals->check_timerfd;
+
maxsock = netsock_prepare_select(globals, , maxsock);
maxsock = netsock_prepare_select(globals, , maxsock);
- ret = pselect(maxsock + 1, , NULL, , , NULL);
+ ret = pselect(maxsock + 1, , NULL, , NULL, NULL);
if (ret == -1) {
perror("main loop select failed ...");
@@ -459,21 +476,27 @@ int alfred_server(struct globals *globals)
continue;
}
}
- clock_gettime(CLOCK_MONOTONIC, _check);
-
- if (globals->opmode == OPMODE_PRIMARY) {
- /* we are a primary */
- printf("[%ld.%09ld] announce primary ...\n",
Re: [PATCH] alfred: notify event listener via unix socket
On Sunday, 1 May 2022 11:10:13 CEST Marek Lindner wrote:
> > > +static void unix_sock_event_listener_free(struct event_listener
> > > *listener)
> > > +{
> > > + list_del_init(>list);
> > > + close(listener->fd);
> > > + free(listener);
> >
> > list_del_init has no benefit (only downsides) when you free the memory
> > anyway at the end of the function
>
> What are those downsides you are referring to ?
Additional writes for something which is dropped anyway.
Kind regards,
Sven
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH] alfred: notify event listener via unix socket
On Sunday, 1 May 2022 09:54:26 CEST Sven Eckelmann wrote:
> > printf("\n");
> > printf("server mode options:\n");
> > printf(" -i, --interface specify the interface
(or
> > comma separated list of interfaces) to listen on\n");>
> > @@ -164,6 +165,7 @@ static struct globals *alfred_init(int argc, char
> > *argv[])>
> > {"change-interface",required_argument, NULL,
'I'},
> > {"change-bat-iface",required_argument, NULL,
'B'},
> > {"server-status", required_argument, NULL, 'S'},
> >
> > + {"event-monitor", required_argument, NULL, 'E'},
>
> Why does it require an argument but the usage doesn't describe the argument?
Copy & paste error from server-status which does not require an argument
either. Might need to fix in a separate patch.
> I am wondering now if it could be interesting for the listener whether the
> data is from us or some remote (for example via the source mac). Does anyone
> else have an opinion about that?
Currently, no use-case comes to my mind.
> > +static void unix_sock_event_listener_free(struct event_listener
> > *listener)
> > +{
> > + list_del_init(>list);
> > + close(listener->fd);
> > + free(listener);
>
> list_del_init has no benefit (only downsides) when you free the memory
> anyway at the end of the function
What are those downsides you are referring to ?
Thanks for the review!
Kind regards,
Marek Lindner
signature.asc
Description: This is a digitally signed message part.
Re: [PATCH] alfred: notify event listener via unix socket
On Saturday, 30 April 2022 12:56:47 CEST Marek Lindner wrote:
> The alfred server instance accepts event notification registration
> via the unix socket. These notification sockets only inform
> registered parties of the availibility of an alfred datatype change.
availability
> +int alfred_client_event_monitor(struct globals *globals)
> +{
[...]
> + fprintf(stdout, "Event: type = %d\n", event_notify.type);
event_notify.type is unsigned and not signed.
> diff --git a/main.c b/main.c
> index 68d6efd..98bf64d 100644
> --- a/main.c
> +++ b/main.c
> @@ -39,6 +39,7 @@ static void alfred_usage(void)
> printf(" -I, --change-interface [interface] change to the specified
> interface(s)\n");
> printf(" -B, --change-bat-iface [interface] change to the specified
> batman-adv interface\n");
> printf(" -S, --server-status request server status
> info such as mode & interfaces\n");
> + printf(" -E, --event-monitor monitor alfred data
> record update eventss\n");
events
> printf("\n");
> printf("server mode options:\n");
> printf(" -i, --interface specify the interface (or
> comma separated list of interfaces) to listen on\n");
> @@ -164,6 +165,7 @@ static struct globals *alfred_init(int argc, char *argv[])
> {"change-interface",required_argument, NULL, 'I'},
> {"change-bat-iface",required_argument, NULL, 'B'},
> {"server-status", required_argument, NULL, 'S'},
> + {"event-monitor", required_argument, NULL, 'E'},
Why does it require an argument but the usage doesn't describe the argument?
See also getopt_long which also doesn't expect an argument
> @@ -138,10 +140,17 @@ static int unix_sock_add_data(struct globals *globals,
> free(dataset);
> goto err;
> }
> + new_entry_created = true;
> }
> dataset->data_source = SOURCE_LOCAL;
> clock_gettime(CLOCK_MONOTONIC, >last_seen);
>
> + /* check that data was changed */
> + if (new_entry_created ||
> + dataset->data.header.length != data_len ||
> + memcmp(dataset->buf, data->data, data_len) != 0)
> + unix_sock_event_notify(globals, data->header.type);
> +
I am wondering now if it could be interesting for the listener whether the
data is from us or some remote (for example via the source mac). Does anyone
else have an opinion about that?
> +static int unix_sock_register_listener(struct globals *globals, int
> client_sock)
> +{
> + struct event_listener *listener;
> + int ret;
> +
> + ret = fcntl(client_sock, F_GETFL, 0);
> + if (ret < 0) {
> + perror("failed to get file status flags");
> + goto err;
> + }
> +
> + ret = fcntl(client_sock, F_SETFL, ret | O_NONBLOCK);
> + if (ret < 0) {
> + perror("failed to set file status flags");
> + goto err;
> + }
> +
> + listener = malloc(sizeof(*listener));
> + if (!listener)
> + goto err;
> +
> + listener->fd = client_sock;
> + INIT_LIST_HEAD(>list);
> + list_add_tail(>list, >event_listeners);
INIT_LIST_HEAD (of the prev/next pointer) is not necessary when you just
overwrite the next/prev pointer in the next line via list_add_tail
> +
> +static void unix_sock_event_listener_free(struct event_listener *listener)
> +{
> + list_del_init(>list);
> + close(listener->fd);
> + free(listener);
list_del_init has no benefit (only downsides) when you free the memory anyway
at the end of the function
> +int unix_sock_events_select_prepare(struct globals *globals, fd_set *fds,
> + fd_set *errfds, int maxsock)
> +{
> + struct event_listener *listener;
> +
> + list_for_each_entry(listener, >event_listeners, list) {
> + if (listener->fd < 0)
> + continue;
> +
> + FD_SET(listener->fd, fds);
> + FD_SET(listener->fd, errfds);
> +
> + if (maxsock < listener->fd)
> + maxsock = listener->fd;
> + }
> +
> + return maxsock;
> +}
I should really rewrite the socket mainloop using epoll but this is just a
side note and not a problem in your patch.
Kind regards,
Sven
signature.asc
Description: This is a digitally signed message part.
