pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-pcap/+/39370?usp=email ) Change subject: server: zero-copy msgb from tcp read to file write ...................................................................... server: zero-copy msgb from tcp read to file write Related: SYS#7080 Change-Id: Iae9b9eaec42ad8ec86d1a8144d676115fa057766 --- M include/osmo-pcap/osmo_pcap_server.h M src/osmo_server_core.c M src/osmo_server_network.c 3 files changed, 97 insertions(+), 67 deletions(-) Approvals: osmith: Looks good to me, but someone else must approve fixeria: Looks good to me, but someone else must approve Jenkins Builder: Verified pespin: Looks good to me, approved diff --git a/include/osmo-pcap/osmo_pcap_server.h b/include/osmo-pcap/osmo_pcap_server.h index e71096b..f97e4ae 100644 --- a/include/osmo-pcap/osmo_pcap_server.h +++ b/include/osmo-pcap/osmo_pcap_server.h @@ -121,15 +121,13 @@ /* pcap stuff */ enum osmo_pcap_fmt file_fmt; bool pcapng_endian_swapped; - uint8_t *file_hdr; - uint32_t file_hdr_len; + struct msgb *file_hdr_msg; /* last time */ struct tm last_write; /* read buffering */ int state; - int pend; bool reopen_delayed; size_t data_max_len; /* size of allocated buffer in data->data. */ @@ -141,13 +139,13 @@ size_t tls_limit_read; struct osmo_tls_session tls_session; struct osmo_wqueue rem_wq; - struct osmo_pcap_data *data; /* Used to store TLS decoded data */ + struct msgb *rx_tls_dec_msg; /* Used to store TLS decoded data */ }; void osmo_pcap_conn_free(struct osmo_pcap_conn *conn); void vty_server_init(void); void osmo_pcap_conn_close(struct osmo_pcap_conn *conn); -int osmo_pcap_conn_process_data(struct osmo_pcap_conn *conn, const uint8_t *data, size_t len); +int osmo_pcap_conn_process_data(struct osmo_pcap_conn *conn, struct msgb *msg); void osmo_pcap_conn_restart_trace(struct osmo_pcap_conn *conn); void osmo_pcap_conn_close_trace(struct osmo_pcap_conn *conn); void osmo_pcap_conn_event(struct osmo_pcap_conn *conn, diff --git a/src/osmo_server_core.c b/src/osmo_server_core.c index 04c14c1..9629423 100644 --- a/src/osmo_server_core.c +++ b/src/osmo_server_core.c @@ -109,7 +109,7 @@ talloc_free(event_name); pcap_zmq_send(conn->server->zmq_publ, - conn->file_hdr, conn->file_hdr_len, + msgb_data(conn->file_hdr_msg), msgb_length(conn->file_hdr_msg), ZMQ_SNDMORE); pcap_zmq_send(conn->server->zmq_publ, data, len, @@ -199,7 +199,6 @@ INIT_LLIST_HEAD(&conn->wrf_flushing_list); conn->data_max_len = calc_data_max_len(server); - conn->data = talloc_zero_size(conn, sizeof(struct osmo_pcap_data) + conn->data_max_len); /* a bit nasty. we do not work with ids but names */ desc = talloc_zero(conn, struct rate_ctr_group_desc); if (!desc) { @@ -289,9 +288,11 @@ close(conn->rem_wq.bfd.fd); conn->rem_wq.bfd.fd = -1; osmo_tls_release(&conn->tls_session); + msgb_free(conn->rx_tls_dec_msg); + conn->rx_tls_dec_msg = NULL; } - TALLOC_FREE(conn->file_hdr); - conn->file_hdr_len = 0; + msgb_free(conn->file_hdr_msg); + conn->file_hdr_msg = NULL; osmo_pcap_conn_close_trace(conn); osmo_pcap_conn_event(conn, "disconnect", NULL); @@ -318,6 +319,7 @@ struct tm tm; int rc; char *real_base_path, *curr_filename; + struct msgb *msg; osmo_pcap_conn_close_trace(conn); @@ -352,10 +354,10 @@ if (rc < 0) return; - /* TODO: get msgb from conn object stored: */ - struct msgb *msg = msgb_alloc_c(conn->wrf, conn->file_hdr_len, "local_iofd_hdr"); - memcpy(msgb_put(msg, conn->file_hdr_len), conn->file_hdr, conn->file_hdr_len); - + /* We need to keep a clone assigned to conn to check for incoming hdr changes: */ + OSMO_ASSERT(conn->file_hdr_msg); + msg = msgb_copy_c(conn->wrf, conn->file_hdr_msg, "wrf_hdr"); + OSMO_ASSERT(msg); rc = osmo_pcap_wr_file_write_msgb(conn->wrf, msg); if (rc < 0) { LOGP(DSERVER, LOGL_ERROR, "Failed to write the header: %d\n", errno); @@ -518,16 +520,17 @@ } /* New recorded packet is received. - * Returns 0 on success, negative on error. */ -int osmo_pcap_conn_process_data(struct osmo_pcap_conn *conn, const uint8_t *data, size_t len) + * Returns 0 on success (and owns msgb), negative on error (msgb to be freed by caller). */ +int osmo_pcap_conn_process_data(struct osmo_pcap_conn *conn, struct msgb *msg) { time_t now = time(NULL); int rc; - zmq_send_client_data(conn, data, len); + zmq_send_client_data(conn, msgb_data(msg), msgb_length(msg)); if (!conn->store) { update_last_write(conn, now); + msgb_free(msg); return 0; } @@ -537,20 +540,18 @@ } /* Check if we are past the limit or on a day change. */ - if (!check_restart_pcap_max_size(conn, len)) + if (!check_restart_pcap_max_size(conn, msgb_length(msg))) check_restart_pcap_localtime(conn, now); - /* TODO: get msgb from caller: */ - struct msgb *msg = msgb_alloc_c(conn->wrf, len, "local_iofd_msg"); - memcpy(msgb_put(msg, len), data, len); - + talloc_steal(conn->wrf, msg); rc = osmo_pcap_wr_file_write_msgb(conn->wrf, msg); if (rc < 0) { LOGP(DSERVER, LOGL_ERROR, "%s: Failed writing to file\n", conn->name); - msgb_free(msg); + /* msgb will be freed by caller */ return -1; } update_last_write(conn, now); + /* msgb is now owned by conn->wrf. */ return 0; } diff --git a/src/osmo_server_network.c b/src/osmo_server_network.c index 5c7a008..da14480 100644 --- a/src/osmo_server_network.c +++ b/src/osmo_server_network.c @@ -121,9 +121,29 @@ } } -/* returns >0 on success, <= 0 on failure (closes conn) */ -static int rx_link_hdr(struct osmo_pcap_conn *conn, const struct osmo_pcap_data *data) +/* Updates conn->file_hdr_msg, owns (frees) msg. */ +static void update_conn_file_hdr_msg(struct osmo_pcap_conn *conn, struct msgb *msg) { + /* The msg chunk of memory to hold the hdr data may actually be a lot bigger than + * the actual data (len << data_len). + * Hence, since we may keep this around for a big time (life of the conn), let's + * better keep a reduced memory footprint msgb instead of the original one: */ + msgb_free(conn->file_hdr_msg); + conn->file_hdr_msg = msgb_copy_resize_c(conn, msg, msgb_length(msg), "conn_file_hdr_msg"); + OSMO_ASSERT(conn->file_hdr_msg); + msgb_free(msg); + + /* pull to l2 is done here at the copy since anyway msgb_copy_resize + * cannot shrink available headroom: */ + msgb_pull_to_l2(conn->file_hdr_msg); + + osmo_pcap_conn_restart_trace(conn); +} + +/* returns >0 on success (msg becomes owned), <= 0 on failure (closes conn) */ +static int rx_link_hdr(struct osmo_pcap_conn *conn, struct msgb *msg) +{ + struct osmo_pcap_data *data = (struct osmo_pcap_data *)msg->l1h; int rc; rc = osmo_pcap_file_discover_fmt(data->data, data->len, &conn->file_fmt); @@ -148,19 +168,11 @@ if (conn->store && !conn->wrf) { /* First received link hdr in conn */ - talloc_free(conn->file_hdr); - conn->file_hdr = talloc_size(conn, data->len); - memcpy(conn->file_hdr, data->data, data->len); - conn->file_hdr_len = data->len; - osmo_pcap_conn_restart_trace(conn); - } else if (conn->file_hdr_len != data->len || - memcmp(&conn->file_hdr, data->data, data->len) != 0) { + update_conn_file_hdr_msg(conn, msg); + } else if (msgb_l2len(conn->file_hdr_msg) != msgb_l2len(msg) || + memcmp(msgb_l2(conn->file_hdr_msg), msgb_l2(msg), msgb_l2len(msg)) != 0) { /* Client changed the link hdr in conn */ - talloc_free(conn->file_hdr); - conn->file_hdr = talloc_size(conn, data->len); - memcpy(conn->file_hdr, data->data, data->len); - conn->file_hdr_len = data->len; - osmo_pcap_conn_restart_trace(conn); + update_conn_file_hdr_msg(conn, msg); } return 1; @@ -247,22 +259,25 @@ } /* returns >0 on success, <= 0 on failure (closes conn) */ -static int rx_link_data(struct osmo_pcap_conn *conn, const struct osmo_pcap_data *data) +static int rx_link_data(struct osmo_pcap_conn *conn, struct msgb *msg) { int rc; - if ((rc = validate_link_data(conn, data)) < 0) + if ((rc = validate_link_data(conn, (struct osmo_pcap_data *)msg->l1h)) < 0) return rc; - if ((rc = osmo_pcap_conn_process_data(conn, &data->data[0], data->len)) < 0) + msgb_pull_to_l2(msg); + if ((rc = osmo_pcap_conn_process_data(conn, msg)) < 0) return rc; return 1; } /* Read segment payload, of size data->len. - * returns >0 on success, <= 0 on failure (closes conn) */ -static int rx_link(struct osmo_pcap_conn *conn, const struct osmo_pcap_data *data) + * returns >0 on success, <= 0 on failure (closes conn). + * Message is always owned or freed here. */ +static int rx_link(struct osmo_pcap_conn *conn, struct msgb *msg) { + struct osmo_pcap_data *data = (struct osmo_pcap_data *)msg->l1h; int rc; /* count the full packet we got */ @@ -275,15 +290,18 @@ switch (data->type) { case PKT_LINK_HDR: - rc = rx_link_hdr(conn, data); + rc = rx_link_hdr(conn, msg); break; case PKT_LINK_DATA: - rc = rx_link_data(conn, data); + rc = rx_link_data(conn, msg); break; default: OSMO_ASSERT(0); } + if (rc <= 0) + msgb_free(msg); + if (conn->reopen_delayed) { LOGP(DSERVER, LOGL_INFO, "Reopening log for %s now.\n", conn->name); osmo_pcap_conn_restart_trace(conn); @@ -306,60 +324,69 @@ static int tls_read_cb_initial(struct osmo_pcap_conn *conn) { int rc; + struct msgb *msg = conn->rx_tls_dec_msg; + msg->l1h = msgb_data(msg); + struct osmo_pcap_data *pdata = (struct osmo_pcap_data *)msg->l1h; + size_t pend = sizeof(*pdata) - msgb_length(msg); - rc = do_read_tls(conn, ((uint8_t *)conn->data) + sizeof(*conn->data) - conn->pend, conn->pend); + OSMO_ASSERT(sizeof(*pdata) > msgb_length(msg)); + + rc = do_read_tls(conn, msg->tail, pend); if (rc <= 0) { LOGP(DSERVER, LOGL_ERROR, - "Too short packet. Got %d, wanted %d\n", rc, conn->data->len); + "Too short packet. Got %d, wanted %zu\n", rc, pend); return -1; } - - conn->pend -= rc; - if (conn->pend < 0) { + if (rc > pend) { LOGP(DSERVER, LOGL_ERROR, - "Someone got the pending read wrong: %d\n", conn->pend); + "Someone got the pending read wrong: %zu\n", pend); return -1; } - if (conn->pend > 0) + msgb_put(msg, rc); + + if (pend > rc) return 1; /* Wait for more data before continuing */ - conn->data->len = ntohs(conn->data->len); + pdata->len = ntohs(pdata->len); - if (conn->data->len > conn->data_max_len) { + if (pdata->len + sizeof(*pdata) > conn->data_max_len) { LOGP(DSERVER, LOGL_ERROR, "Implausible data length: %u > %zu (snaplen %u)\n", - conn->data->len, conn->data_max_len, conn->server->max_snaplen); + pdata->len, conn->data_max_len, conn->server->max_snaplen); return -1; } conn->state = STATE_DATA; - conn->pend = conn->data->len; + msg->l2h = msg->tail; return 1; } static int tls_read_cb_data(struct osmo_pcap_conn *conn) { int rc; + struct msgb *msg = conn->rx_tls_dec_msg; + struct osmo_pcap_data *pdata = (struct osmo_pcap_data *)msg->l1h; + size_t pend = pdata->len - msgb_l2len(msg); - rc = do_read_tls(conn, &conn->data->data[conn->data->len - conn->pend], conn->pend); + rc = do_read_tls(conn, msg->tail, pend); if (rc <= 0) { LOGP(DSERVER, LOGL_ERROR, - "Too short packet. Got %d, wanted %d\n", rc, conn->data->len); + "Too short packet. Got %d, wanted %u\n", rc, pdata->len); return -1; } - - conn->pend -= rc; - if (conn->pend < 0) { + if (rc > pend) { LOGP(DSERVER, LOGL_ERROR, - "Someone got the pending read wrong: %d\n", conn->pend); + "Someone got the pending read wrong: %zu\n", pend); return -1; } - if (conn->pend > 0) + msgb_put(msg, rc); + + if (pend > rc) return 1; /* Wait for more data before continuing */ conn->state = STATE_INITIAL; - conn->pend = sizeof(*conn->data); + conn->rx_tls_dec_msg = msgb_alloc_c(conn, conn->data_max_len, "rx_tls_dec_data"); - return rx_link(conn, conn->data); + return rx_link(conn, msg); } /* returns >0 on success, <= 0 on failure (closes conn) */ @@ -422,11 +449,15 @@ return 0; } - data = (struct osmo_pcap_data *)msgb_data(msg); + OSMO_ASSERT(msgb_length(msg) >= sizeof(*data)); + + msg->l1h = msgb_data(msg); + data = (struct osmo_pcap_data *)msg->l1h; data->len = osmo_ntohs(data->len); - rc = rx_link(conn, data); - msgb_free(msg); + msg->l2h = msg->l1h + sizeof(*data); + + rc = rx_link(conn, msg); if (rc <= 0) osmo_pcap_conn_close(conn); return 0; @@ -485,7 +516,7 @@ } /* Prepare for first read of segment header: */ conn->state = STATE_INITIAL; - conn->pend = sizeof(struct osmo_pcap_data); + conn->rx_tls_dec_msg = msgb_alloc_c(conn, conn->data_max_len, "rx_tls_dec_data"); if (!osmo_tls_init_server_session(conn, server)) { osmo_pcap_conn_close(conn); return; -- To view, visit https://gerrit.osmocom.org/c/osmo-pcap/+/39370?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: osmo-pcap Gerrit-Branch: master Gerrit-Change-Id: Iae9b9eaec42ad8ec86d1a8144d676115fa057766 Gerrit-Change-Number: 39370 Gerrit-PatchSet: 2 Gerrit-Owner: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria &lt;vyanitskiy(a)sysmocom.de&gt; Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Reviewer: osmith &lt;osmith(a)sysmocom.de&gt; Gerrit-Reviewer: pespin &lt;pespin(a)sysmocom.de&gt;