pespin has uploaded this change for review.
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(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-pcap refs/changes/70/39370/1
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 bc8c08c..c2f880e 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 change 39370. To unsubscribe, or for help writing mail filters, visit settings.