From 1831c27adeb851b7b28e57342f4893a1d3b7dce2 Mon Sep 17 00:00:00 2001 From: Jordan Augé Date: Fri, 30 Sep 2022 12:29:05 +0200 Subject: refactor(hicn-light): cleanup towards optimizations to UDP socket face MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change-Id: If5bc3c4dfcdde386ac02674f3c03d32d495b2231 Ticket: HICN-771 Signed-off-by: Jordan Augé --- hicn-light/src/hicn/base/loop.c | 14 ++++--- hicn-light/src/hicn/base/loop.h | 7 +++- hicn-light/src/hicn/core/connection.c | 3 +- hicn-light/src/hicn/core/forwarder.c | 45 ++++++++++++---------- hicn-light/src/hicn/core/listener.c | 21 ++++++---- hicn-light/src/hicn/core/listener.h | 9 +++-- hicn-light/src/hicn/core/mapme.c | 5 ++- hicn-light/src/hicn/core/policy_stats.c | 4 +- hicn-light/src/hicn/test/test-loop.cc | 18 ++++++--- .../src/hicn/test/test-strategy-local-remote.cc | 2 +- 10 files changed, 78 insertions(+), 50 deletions(-) (limited to 'hicn-light') diff --git a/hicn-light/src/hicn/base/loop.c b/hicn-light/src/hicn/base/loop.c index 68970ed3a..3407bfc1c 100644 --- a/hicn-light/src/hicn/base/loop.c +++ b/hicn-light/src/hicn/base/loop.c @@ -40,6 +40,7 @@ loop_t *MAIN_LOOP = NULL; typedef struct { void *owner; fd_callback_t callback; + unsigned id; void *data; } cb_wrapper_args_t; @@ -111,16 +112,19 @@ int loop_undispatch(loop_t *loop) { return 0; } void cb_wrapper(evutil_socket_t fd, short what, void *arg) { cb_wrapper_args_t *cb_wrapper_args = arg; - cb_wrapper_args->callback(cb_wrapper_args->owner, fd, cb_wrapper_args->data); + cb_wrapper_args->callback(cb_wrapper_args->owner, fd, cb_wrapper_args->id, + cb_wrapper_args->data); } static inline void _event_create(event_t **event, loop_t *loop, event_type_t type, void *callback_owner, - fd_callback_t callback, void *callback_data) { + fd_callback_t callback, unsigned id, + void *callback_data) { *event = malloc(sizeof(event_t)); (*event)->callback = (cb_wrapper_args_t){ .owner = callback_owner, .callback = callback, + .id = id, .data = callback_data, }; (*event)->event_type = type; @@ -129,8 +133,8 @@ static inline void _event_create(event_t **event, loop_t *loop, int loop_fd_event_create(event_t **event, loop_t *loop, int fd, void *callback_owner, fd_callback_t callback, - void *callback_data) { - _event_create(event, loop, EVTYPE_FD, callback_owner, callback, + unsigned id, void *callback_data) { + _event_create(event, loop, EVTYPE_FD, callback_owner, callback, id, callback_data); evutil_make_socket_nonblocking(fd); @@ -161,7 +165,7 @@ int loop_event_unregister(event_t *event) { int loop_timer_create(event_t **timer, loop_t *loop, void *callback_owner, fd_callback_t callback, void *callback_data) { - _event_create(timer, loop, EVTYPE_TIMER, callback_owner, callback, + _event_create(timer, loop, EVTYPE_TIMER, callback_owner, callback, 0, callback_data); evtimer_assign(&(*timer)->raw_event, loop->event_base, cb_wrapper, diff --git a/hicn-light/src/hicn/base/loop.h b/hicn-light/src/hicn/base/loop.h index f81af333b..0eecb60fe 100644 --- a/hicn-light/src/hicn/base/loop.h +++ b/hicn-light/src/hicn/base/loop.h @@ -23,12 +23,13 @@ /* fd & timer callbacks */ -typedef int (*fd_callback_t)(void *owner, int fd, void *data); +typedef int (*fd_callback_t)(void *owner, int fd, unsigned id, void *data); typedef struct { int fd; void *owner; fd_callback_t callback; + unsigned id; void *data; } fd_callback_data_t; @@ -81,12 +82,14 @@ int loop_undispatch(loop_t *loop); * \param [in] callback_owner - Pointer to the owner of the callack (first * parameter of callback function) * \param [in] callback - Callback function + * \param [in] id - User parameter to pass alongside callback invocation, + * allowing to pass an id without allocating memory. * \param [in] callback_data - User data to pass alongside callback invocation * \return 0 in case of success, -1 otherwise */ int loop_fd_event_create(event_t **event, loop_t *loop, int fd, void *callback_owner, fd_callback_t callback, - void *callback_data); + unsigned id, void *callback_data); /** * Register event in corresponding event loop. diff --git a/hicn-light/src/hicn/core/connection.c b/hicn-light/src/hicn/core/connection.c index 40802368f..a9d632d52 100644 --- a/hicn-light/src/hicn/core/connection.c +++ b/hicn-light/src/hicn/core/connection.c @@ -207,7 +207,8 @@ int connection_initialize(connection_t *connection, face_type_t type, * the listener. */ loop_fd_event_create(&connection->event_data, MAIN_LOOP, fd, listener, - (fd_callback_t)listener_read_callback, NULL); + (fd_callback_t)listener_read_callback, connection->id, + NULL); if (!connection->event_data) { goto ERR_REGISTER_FD; diff --git a/hicn-light/src/hicn/core/forwarder.c b/hicn-light/src/hicn/core/forwarder.c index face35fd6..d0472b520 100644 --- a/hicn-light/src/hicn/core/forwarder.c +++ b/hicn-light/src/hicn/core/forwarder.c @@ -1460,43 +1460,38 @@ ssize_t forwarder_receive(forwarder_t *forwarder, listener_t *listener, assert(msgbuf_id_is_valid(msgbuf_id)); assert(pair); + hicn_name_t name; msgbuf_pool_t *msgbuf_pool = forwarder_get_msgbuf_pool(forwarder); msgbuf_t *msgbuf = msgbuf_pool_at(msgbuf_pool, msgbuf_id); assert(msgbuf); size_t size = msgbuf_get_len(msgbuf); - /* Connection lookup */ const connection_table_t *table = forwarder_get_connection_table(listener->forwarder); - connection_t *connection = connection_table_get_by_pair(table, pair); - unsigned conn_id = connection ? (unsigned)connection_table_get_connection_id( - table, connection) - : CONNECTION_ID_UNDEFINED; - assert((conn_id != CONNECTION_ID_UNDEFINED) || listener); - -#if 0 - /* - * We have a msgbuf with payload and size, we nee to populate other - * information, including packet type etc. - */ - msgbuf_type_t type = get_type_from_packet(msgbuf_get_packet(msgbuf)); + /* Connection lookup */ + if (msgbuf_get_connection_id(msgbuf) == CONNECTION_ID_UNDEFINED) { + connection_t *connection = connection_table_get_by_pair(table, pair); + unsigned conn_id = + connection + ? (unsigned)connection_table_get_connection_id(table, connection) + : CONNECTION_ID_UNDEFINED; + + assert((conn_id != CONNECTION_ID_UNDEFINED) || listener); + msgbuf->connection_id = conn_id; + } - forwarder->stats.countReceived++; - msgbuf->type = type; -#endif forwarder->stats.countReceived++; /* Initialize packet buffer stored in msgbuf through libhicn */ msgbuf_initialize_from_packet(msgbuf); + + /* Detect packet type */ hicn_packet_analyze(msgbuf_get_pkbuf(msgbuf)); - msgbuf->connection_id = conn_id; msgbuf->recv_ts = now; - hicn_name_t name; - RETRY: switch (msgbuf_get_type(msgbuf)) { @@ -1516,7 +1511,6 @@ RETRY: goto DROP; } msgbuf->connection_id = connection_id; - connection = connection_table_get_by_id(table, connection_id); } msgbuf->path_label = 0; // not used for interest packets hicn_interest_get_name(msgbuf_get_pkbuf(msgbuf), &name); @@ -1546,13 +1540,17 @@ RETRY: pkt_cache_log(forwarder->pkt_cache); break; +#ifdef WITH_WLDR case HICN_PACKET_TYPE_WLDR_NOTIFICATION: if (!connection_id_is_valid(msgbuf->connection_id)) { ERROR("Invalid connection for WLDR packet"); goto DROP; } + connection_t *connection = + connection_table_get_by_id(table, msgbuf->connection_id); connection_wldr_handle_notification(connection, msgbuf); break; +#endif case HICN_PACKET_TYPE_MAPME: // XXX what about acks ? @@ -1592,7 +1590,6 @@ RETRY: goto DROP; } msgbuf->connection_id = connection_id; - connection = connection_table_get_by_id(table, connection_id); } msg_header_t *msg = (msg_header_t *)msgbuf_get_packet(msgbuf); @@ -1602,6 +1599,12 @@ RETRY: goto DROP; } + /* + * We need to retrieve the connection (in case it is useful) before + * proceeding through the removal + */ + connection_t *connection = + connection_table_get_by_id(table, msgbuf_get_connection_id(msgbuf)); size = command_process_msgbuf(forwarder, msgbuf); if (msgbuf->command.type == COMMAND_TYPE_CONNECTION_REMOVE) _forwarder_finalize_connection_if_self(connection, msgbuf); diff --git a/hicn-light/src/hicn/core/listener.c b/hicn-light/src/hicn/core/listener.c index 58ba53fe1..d2f547863 100644 --- a/hicn-light/src/hicn/core/listener.c +++ b/hicn-light/src/hicn/core/listener.c @@ -111,7 +111,8 @@ int listener_initialize(listener_t *listener, face_type_t type, // XXX data should be pre-allocated here loop_fd_event_create(&listener->event_data, MAIN_LOOP, listener->fd, listener, - (fd_callback_t)listener_read_callback, NULL); + (fd_callback_t)listener_read_callback, + CONNECTION_ID_UNDEFINED, NULL); if (!listener->event_data) { goto ERR_REGISTER_FD; @@ -267,7 +268,8 @@ int listener_punt(const listener_t *listener, const char *prefix_s) { return listener_vft[get_protocol(listener->type)]->punt(listener, prefix_s); } -ssize_t listener_read_single(listener_t *listener, int fd) { +ssize_t listener_read_single(listener_t *listener, int fd, + unsigned connection_id) { assert(listener); msgbuf_pool_t *msgbuf_pool = forwarder_get_msgbuf_pool(listener->forwarder); @@ -291,6 +293,7 @@ ssize_t listener_read_single(listener_t *listener, int fd) { } msgbuf_pool_acquire(msgbuf); + msgbuf_set_connection_id(msgbuf, connection_id); // Process received packet size_t processed_bytes = forwarder_receive(listener->forwarder, listener, @@ -308,7 +311,8 @@ ssize_t listener_read_single(listener_t *listener, int fd) { return processed_bytes; } -ssize_t listener_read_batch(listener_t *listener, int fd) { +ssize_t listener_read_batch(listener_t *listener, int fd, + unsigned connection_id) { assert(listener); size_t total_processed_bytes = 0; @@ -356,6 +360,7 @@ ssize_t listener_read_batch(listener_t *listener, int fd) { } msgbuf_pool_acquire(msgbufs[i]); + msgbuf_set_connection_id(msgbufs[i], connection_id); forwarder_acquired_msgbuf_ids_push(forwarder, msgbuf_ids[i]); } @@ -392,10 +397,10 @@ ssize_t listener_read_batch(listener_t *listener, int fd) { * This might be called for a connection on the listener too. The listener is * the entity that owns the buffers used for reading. */ -ssize_t listener_read_callback(listener_t *listener, int fd, void *user_data) { - // DEBUG("[listener_read_callback]"); - // XXX make a single callback and arbitrate between read and readbatch +ssize_t listener_read_callback(listener_t *listener, int fd, + unsigned connection_id, void *user_data) { assert(listener); + assert(!user_data); /* * As the listener callback is shared between the listener and the different @@ -404,9 +409,9 @@ ssize_t listener_read_callback(listener_t *listener, int fd, void *user_data) { // assert(fd == listener->fd); if (listener_vft[get_protocol(listener->type)]->read_batch) - return listener_read_batch(listener, fd); + return listener_read_batch(listener, fd, connection_id); - return listener_read_single(listener, fd); + return listener_read_single(listener, fd, connection_id); } void listener_setup_local(forwarder_t *forwarder, uint16_t port) { diff --git a/hicn-light/src/hicn/core/listener.h b/hicn-light/src/hicn/core/listener.h index 5d0384329..76d865c5d 100644 --- a/hicn-light/src/hicn/core/listener.h +++ b/hicn-light/src/hicn/core/listener.h @@ -115,8 +115,10 @@ void listener_setup_local(struct forwarder_s *forwarder, uint16_t port); void listener_process_packet(const listener_t *listener, const uint8_t *packet, size_t size); -ssize_t listener_read_single(listener_t *listener, int fd); -ssize_t listener_read_batch(listener_t *listener, int fd); +ssize_t listener_read_single(listener_t *listener, int fd, + unsigned connection_id); +ssize_t listener_read_batch(listener_t *listener, int fd, + unsigned connection_id); /** * @brief Callback helper function for batch reading data from listener fd. @@ -129,7 +131,8 @@ ssize_t listener_read_batch(listener_t *listener, int fd); * NOTE: the function returns size_t as for TCP we might need to know how much * data we can consume from the socket. */ -ssize_t listener_read_callback(listener_t *listener, int fd, void *user_data); +ssize_t listener_read_callback(listener_t *listener, int fd, + unsigned connection_id, void *user_data); #define listener_get_forwarder(listener) (listener->forwarder) #define listener_get_fd(listener) (listener->fd) diff --git a/hicn-light/src/hicn/core/mapme.c b/hicn-light/src/hicn/core/mapme.c index b151d6cb0..fede52cae 100644 --- a/hicn-light/src/hicn/core/mapme.c +++ b/hicn-light/src/hicn/core/mapme.c @@ -218,7 +218,7 @@ static mapme_t mapme_default = { /******************************************************************************/ -int mapme_on_timeout(void *mapme_arg, int fd, void *data); +int mapme_on_timeout(void *mapme_arg, int fd, unsigned id, void *data); mapme_t *mapme_create(void *forwarder) { mapme_t *mapme = malloc(sizeof(mapme_t)); @@ -575,9 +575,10 @@ int mapme_create_fib_entry(const mapme_t *mapme, const Name *name, } #endif -int mapme_on_timeout(void *mapme_arg, int fd, void *data) { +int mapme_on_timeout(void *mapme_arg, int fd, unsigned id, void *data) { mapme_t *mapme = mapme_arg; assert(mapme); + assert(id == 0); assert(!data); /* Timeout occurred, we have to retransmit IUs for all pending * prefixes having entries in TFIB diff --git a/hicn-light/src/hicn/core/policy_stats.c b/hicn-light/src/hicn/core/policy_stats.c index 470e74a24..8706a9244 100644 --- a/hicn-light/src/hicn/core/policy_stats.c +++ b/hicn-light/src/hicn/core/policy_stats.c @@ -30,9 +30,11 @@ #define ALPHA 0.9 #define STATS_INTERVAL 1000 /* ms */ -static int policy_stats_mgr_tick(void* mgr_arg, int fd, void* data) { +static int policy_stats_mgr_tick(void* mgr_arg, int fd, unsigned id, + void* data) { policy_stats_mgr_t* mgr = mgr_arg; assert(mgr); + assert(id == 0); assert(!data); uint64_t now = ticks_now(); diff --git a/hicn-light/src/hicn/test/test-loop.cc b/hicn-light/src/hicn/test/test-loop.cc index f783b0ada..0ebf7a22a 100644 --- a/hicn-light/src/hicn/test/test-loop.cc +++ b/hicn-light/src/hicn/test/test-loop.cc @@ -57,20 +57,25 @@ class LoopTest : public ::testing::Test { // before the destructor). } - static int onFirstTimerExpiration(void *owner, int fd, void *arg) { + static int onFirstTimerExpiration(void *owner, int fd, unsigned id, + void *arg) { + assert(id == 0); std::cout << "This function should never be called" << std::endl; EXPECT_TRUE(false); return -1; } - static int onSecondTimerExpiration(void *owner, int fd, void *arg) { + static int onSecondTimerExpiration(void *owner, int fd, unsigned id, + void *arg) { + assert(id == 0); std::cout << "First timer expired. Cancel second timer." << std::endl; LoopTest *test = (LoopTest *)(arg); loop_event_unregister(test->timer_); return 0; } - static int onTimerExpiration(void *owner, int fd, void *arg) { + static int onTimerExpiration(void *owner, int fd, unsigned id, void *arg) { + assert(id == 0); // Create client socket struct sockaddr_in addr; int client_socket; @@ -103,7 +108,8 @@ class LoopTest : public ::testing::Test { return 0; } - static int onNewConnection(void *owner, int fd, void *arg) { + static int onNewConnection(void *owner, int fd, unsigned id, void *arg) { + assert(id == 0); LoopTest *test = (LoopTest *)arg; struct sockaddr_in addr; addr.sin_addr.s_addr = INADDR_ANY; @@ -193,7 +199,7 @@ TEST_F(LoopTest, EventCreateAndFree) { loop_ = loop_create(); ret = loop_fd_event_create(&event_, loop_, fd, nullptr, - &LoopTest::onNewConnection, this); + &LoopTest::onNewConnection, 0, this); EXPECT_TRUE(ret >= 0); EXPECT_TRUE(event_); @@ -257,7 +263,7 @@ TEST_F(LoopTest, LoopDispatch) { loop_ = loop_create(); ret = loop_fd_event_create(&event_, loop_, connection_socket_, nullptr, - &LoopTest::onNewConnection, this); + &LoopTest::onNewConnection, 0, this); EXPECT_TRUE(ret >= 0); EXPECT_TRUE(event_); diff --git a/hicn-light/src/hicn/test/test-strategy-local-remote.cc b/hicn-light/src/hicn/test/test-strategy-local-remote.cc index 8230b9482..413ac4080 100644 --- a/hicn-light/src/hicn/test/test-strategy-local-remote.cc +++ b/hicn-light/src/hicn/test/test-strategy-local-remote.cc @@ -66,7 +66,7 @@ class StrategyLocalRemoteTest : public ::testing::Test { } strategy_entry_t entry_; - nexthops_t available_nexthops_; + nexthops_t available_nexthops_ = NEXTHOPS_EMPTY; configuration_t* conf_; forwarder_t* fwd_; msgbuf_t msgbuf_; -- cgit 1.2.3-korg