From: Pablo Neira Ayuso pablo@netfilter.org
The following patchset, although it looks a bit large, it is composed of many small incremental changes to improve the existing osmux infrastructure and to add dummy padding support.
The dummy padding consists of a osmux header and payload that looks like this:
OSMUX seq=000 ccid=002 ft=2 ctr=3 amr_f=0 amr_q=0 amr_ft=03 amr_cmr=00 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ]
The frame type is OSMUX_FT_DUMMY (=2) in the batch that whose payload is padded with 0xff. Note that the message above is padding the circuit ID 2.
This is used for early bandwidth preallocation in links that involve relatively slow dynamic bandwidth allocation such as Iridium/OpenPort.
The osmux test infrastructure has been improved (to resolve some memleaks) and enhanced to cover this new dummy support.
The OpenBSC changeset is composed of one very small patch that will come after this batch.
If no major concerns, I'll push this initial batch soon into the repository. Thanks.
Pablo Neira Ayuso (18): osmux: discard non voice osmux message osmux: add osmux_input_state structure osmux: add circuit helper functions osmux: rename struct batch_list_node to osmux_circuit osmux: rename circuit->list to circuit->rtp_list osmux: pass up struct osmux_batch osmux: count pending messages to be transformed in the batch osmux: introduce osmux_xfrm_input_close_circuit() osmux: introduce osmux_xfrm_input_open_circuit() tests: osmux: adapt it to use the new circuit API tests: osmux: factor out main test loop tests: osmux: test online deactivation of dummy padding tests: osmux: validate dummy padding with no voice data interaction tests: osmux: test circuit reopening after closure osmux: kill osmux_get_hdr() tests: osmux: fix msgb leaks tests: compile tests with debugging symbols, ie. -g tests: osmux: iterate 64 times in osmo_test_loop()
include/osmocom/netif/osmux.h | 8 +- src/osmux.c | 374 ++++++++++++++++++++++++++--------------- tests/Makefile.am | 2 +- tests/osmux/osmux_test.c | 80 ++++++--- 4 files changed, 304 insertions(+), 160 deletions(-)
From: Pablo Neira Ayuso pablo@soleta.eu
We only support voice osmux messages by now. Discard unsupported types. --- src/osmux.c | 5 +++++ 1 file changed, 5 insertions(+)
diff --git a/src/osmux.c b/src/osmux.c index eb2c683..a3eccaa 100644 --- a/src/osmux.c +++ b/src/osmux.c @@ -65,6 +65,11 @@ struct osmux_hdr *osmux_xfrm_output_pull(struct msgb *msg)
osmuxh = (struct osmux_hdr *)msg->data;
+ if (osmuxh->ft != OSMUX_FT_VOICE_AMR) { + LOGP(DLMIB, LOGL_ERROR, "Discarding unsupported Osmux FT %d\n", + osmuxh->ft); + return NULL; + } if (!osmo_amr_ft_valid(osmuxh->amr_ft)) { LOGP(DLMIB, LOGL_ERROR, "Discarding bad AMR FT %d\n", osmuxh->amr_ft);
From: Pablo Neira Ayuso pablo@soleta.eu
This new structure serves as container of the internal osmux state during transformation from RTP AMR to the compressed osmux format.
This reduces the footprint of several functions and it makes them easier to extend if we need to pass new information between functions. --- src/osmux.c | 77 +++++++++++++++++++++++++++++++---------------------------- 1 file changed, 40 insertions(+), 37 deletions(-)
diff --git a/src/osmux.c b/src/osmux.c index a3eccaa..938b9a6 100644 --- a/src/osmux.c +++ b/src/osmux.c @@ -221,63 +221,65 @@ static void osmux_batch_dequeue(struct msgb *msg, struct batch_list_node *node) node->nmsgs--; }
-static int -osmux_batch_put(struct osmux_in_handle *h, struct msgb *out_msg, - struct msgb *msg, struct rtp_hdr *rtph, - struct amr_hdr *amrh, uint32_t amr_payload_len, - int ccid, int add_osmux_header) +struct osmux_input_state { + struct msgb *out_msg; + struct msgb *msg; + struct rtp_hdr *rtph; + struct amr_hdr *amrh; + uint32_t amr_payload_len; + int ccid; + int add_osmux_hdr; +}; + +static int osmux_batch_put(struct osmux_in_handle *h, + struct osmux_input_state *state) { struct osmux_batch *batch = (struct osmux_batch *)h->internal_data; struct osmux_hdr *osmuxh;
- if (add_osmux_header) { - osmuxh = (struct osmux_hdr *)out_msg->tail; + if (state->add_osmux_hdr) { + osmuxh = (struct osmux_hdr *)state->out_msg->tail; osmuxh->ft = OSMUX_FT_VOICE_AMR; osmuxh->ctr = 0; - osmuxh->amr_f = amrh->f; - osmuxh->amr_q= amrh->q; + osmuxh->amr_f = state->amrh->f; + osmuxh->amr_q= state->amrh->q; osmuxh->seq = batch->seq++; - osmuxh->circuit_id = ccid; - osmuxh->amr_cmr = amrh->cmr; - osmuxh->amr_ft = amrh->ft; - msgb_put(out_msg, sizeof(struct osmux_hdr)); + osmuxh->circuit_id = state->ccid; + osmuxh->amr_cmr = state->amrh->cmr; + osmuxh->amr_ft = state->amrh->ft; + msgb_put(state->out_msg, sizeof(struct osmux_hdr));
/* annotate current osmux header */ batch->osmuxh = osmuxh; } else { if (batch->osmuxh->ctr == 0x7) { LOGP(DLMIB, LOGL_ERROR, "cannot add msg=%p, " - "too many messages for this RTP ssrc=%u\n", - msg, rtph->ssrc); + "too many messages for this RTP ssrc=%u\n", + state->msg, state->rtph->ssrc); return 0; } batch->osmuxh->ctr++; }
- memcpy(out_msg->tail, osmo_amr_get_payload(amrh), amr_payload_len); - msgb_put(out_msg, amr_payload_len); + memcpy(state->out_msg->tail, osmo_amr_get_payload(state->amrh), + state->amr_payload_len); + msgb_put(state->out_msg, state->amr_payload_len);
return 0; }
-static int -osmux_xfrm_encode_amr(struct osmux_in_handle *h, - struct msgb *out_msg, - struct rtp_hdr *rtph, struct msgb *msg, - int ccid, int add_osmux_header) +static int osmux_xfrm_encode_amr(struct osmux_in_handle *h, + struct osmux_input_state *state) { - struct amr_hdr *amrh; uint32_t amr_len; - uint32_t amr_payload_len;
- amrh = osmo_rtp_get_payload(rtph, msg, &amr_len); - if (amrh == NULL) + state->amrh = osmo_rtp_get_payload(state->rtph, state->msg, &amr_len); + if (state->amrh == NULL) return -1;
- amr_payload_len = amr_len - sizeof(struct amr_hdr); + state->amr_payload_len = amr_len - sizeof(struct amr_hdr);
- if (osmux_batch_put(h, out_msg, msg, rtph, amrh, amr_payload_len, - ccid, add_osmux_header) < 0) + if (osmux_batch_put(h, state) < 0) return -1;
return 0; @@ -304,9 +306,11 @@ static struct msgb *osmux_build_batch(struct osmux_in_handle *h) int ctr = 0;
llist_for_each_entry_safe(cur, tmp, &node->list, list) { - struct rtp_hdr *rtph; - int add_osmux_hdr = 0; - + struct osmux_input_state state = { + .msg = cur, + .out_msg = batch_msg, + .ccid = node->ccid, + }; #ifdef DEBUG_MSG char buf[4096];
@@ -315,19 +319,18 @@ static struct msgb *osmux_build_batch(struct osmux_in_handle *h) LOGP(DLMIB, LOGL_DEBUG, "to BSC-NAT: %s\n", buf); #endif
- rtph = osmo_rtp_get_hdr(cur); - if (rtph == NULL) + state.rtph = osmo_rtp_get_hdr(cur); + if (state.rtph == NULL) return NULL;
if (ctr == 0) { #ifdef DEBUG_MSG LOGP(DLMIB, LOGL_DEBUG, "add osmux header\n"); #endif - add_osmux_hdr = 1; + state.add_osmux_hdr = 1; }
- osmux_xfrm_encode_amr(h, batch_msg, rtph, cur, - node->ccid, add_osmux_hdr); + osmux_xfrm_encode_amr(h, &state); osmux_batch_dequeue(cur, node); msgb_free(cur); ctr++;
From: Pablo Neira Ayuso pablo@soleta.eu
Add osmux_batch_add_circuit() and osmux_batch_find_circuit() helper functions. --- src/osmux.c | 65 ++++++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 46 insertions(+), 19 deletions(-)
diff --git a/src/osmux.c b/src/osmux.c index 938b9a6..c3e68a3 100644 --- a/src/osmux.c +++ b/src/osmux.c @@ -450,19 +450,50 @@ static void osmux_replay_lost_packets(struct batch_list_node *node, } }
+static struct batch_list_node * +osmux_batch_find_circuit(struct osmux_batch *batch, int ccid) +{ + struct batch_list_node *circuit; + + llist_for_each_entry(circuit, &batch->node_list, head) { + if (circuit->ccid == ccid) + return circuit; + } + return NULL; +} + +static struct batch_list_node * +osmux_batch_add_circuit(struct osmux_batch *batch, int ccid) +{ + struct batch_list_node *circuit; + + circuit = osmux_batch_find_circuit(batch, ccid); + if (circuit != NULL) { + LOGP(DLMIB, LOGL_ERROR, "circuit %u already exists!\n", ccid); + return NULL; + } + + circuit = talloc_zero(osmux_ctx, struct batch_list_node); + if (circuit == NULL) { + LOGP(DLMIB, LOGL_ERROR, "OOM on circuit %u\n", ccid); + return NULL; + } + + circuit->ccid = ccid; + INIT_LLIST_HEAD(&circuit->list); + llist_add_tail(&circuit->head, &batch->node_list); + + return circuit; +} + static int osmux_batch_add(struct osmux_batch *batch, struct msgb *msg, struct rtp_hdr *rtph, int ccid) { - struct batch_list_node *node; - int found = 0, bytes = 0, amr_payload_len; + int bytes = 0, amr_payload_len; + struct batch_list_node *circuit;
- llist_for_each_entry(node, &batch->node_list, head) { - if (node->ccid == ccid) { - found = 1; - break; - } - } + circuit = osmux_batch_find_circuit(batch, ccid);
amr_payload_len = osmux_rtp_amr_payload_len(msg, rtph); if (amr_payload_len < 0) @@ -470,21 +501,21 @@ osmux_batch_add(struct osmux_batch *batch, struct msgb *msg,
/* First check if there is room for this message in the batch */ bytes += amr_payload_len; - if (!found) + if (!circuit) bytes += sizeof(struct osmux_hdr);
/* No room, sorry. You'll have to retry */ if (bytes > batch->remaining_bytes) return 1;
- if (found) { + if (circuit) { struct msgb *cur;
/* Extra validation: check if this message already exists, * should not happen but make sure we don't propagate * duplicated messages. */ - llist_for_each_entry(cur, &node->list, list) { + llist_for_each_entry(cur, &circuit->list, list) { struct rtp_hdr *rtph2 = osmo_rtp_get_hdr(cur); if (rtph2 == NULL) return -1; @@ -498,21 +529,17 @@ osmux_batch_add(struct osmux_batch *batch, struct msgb *msg, } } /* Handle RTP packet loss scenario */ - osmux_replay_lost_packets(node, rtph); + osmux_replay_lost_packets(circuit, rtph);
} else { /* This is the first message with that ssrc we've seen */ - node = talloc_zero(osmux_ctx, struct batch_list_node); - if (node == NULL) + circuit = osmux_batch_add_circuit(batch, ccid); + if (!circuit) return -1; - - node->ccid = ccid; - INIT_LLIST_HEAD(&node->list); - llist_add_tail(&node->head, &batch->node_list); }
/* This batch is full, force batch delivery */ - if (osmux_batch_enqueue(msg, node) < 0) + if (osmux_batch_enqueue(msg, circuit) < 0) return 1;
#ifdef DEBUG_MSG
On 21 Jul 2015, at 16:23, pablo@gnumonks.org wrote:
Hi!
+static struct batch_list_node * +osmux_batch_add_circuit(struct osmux_batch *batch, int ccid) +{
- struct batch_list_node *circuit;
- circuit = osmux_batch_find_circuit(batch, ccid);
- if (circuit != NULL) {
LOGP(DLMIB, LOGL_ERROR, "circuit %u already exists!\n", ccid);return NULL;- }
Okay very defensive but no performance issue right now. Patches 1-3 look good!
holger
From: Pablo Neira Ayuso pablo@soleta.eu
I think this is a better name for this object. Basically, an input handle represents a batch that is composed of one or more circuit objects.
Each circuit object contains a list of RTP messages that are pending to be converted to the osmux format in one single batch. --- src/osmux.c | 66 +++++++++++++++++++++++++++++------------------------------ 1 file changed, 33 insertions(+), 33 deletions(-)
diff --git a/src/osmux.c b/src/osmux.c index c3e68a3..81ad9dc 100644 --- a/src/osmux.c +++ b/src/osmux.c @@ -181,24 +181,24 @@ int osmux_xfrm_output(struct osmux_hdr *osmuxh, struct osmux_out_handle *h, struct osmux_batch { struct osmo_timer_list timer; struct osmux_hdr *osmuxh; - struct llist_head node_list; + struct llist_head circuit_list; unsigned int remaining_bytes; uint8_t seq; };
-struct batch_list_node { +struct osmux_circuit { struct llist_head head; int ccid; struct llist_head list; int nmsgs; };
-static int osmux_batch_enqueue(struct msgb *msg, struct batch_list_node *node) +static int osmux_batch_enqueue(struct msgb *msg, struct osmux_circuit *circuit) { /* Too many messages per batch, discard it. The counter field of the * osmux header is just 3 bits long, so make sure it doesn't overflow. */ - if (node->nmsgs >= 8) { + if (circuit->nmsgs >= 8) { struct rtp_hdr *rtph;
rtph = osmo_rtp_get_hdr(msg); @@ -210,15 +210,15 @@ static int osmux_batch_enqueue(struct msgb *msg, struct batch_list_node *node) return -1; }
- llist_add_tail(&msg->list, &node->list); - node->nmsgs++; + llist_add_tail(&msg->list, &circuit->list); + circuit->nmsgs++; return 0; }
-static void osmux_batch_dequeue(struct msgb *msg, struct batch_list_node *node) +static void osmux_batch_dequeue(struct msgb *msg, struct osmux_circuit *circuit) { llist_del(&msg->list); - node->nmsgs--; + circuit->nmsgs--; }
struct osmux_input_state { @@ -288,7 +288,7 @@ static int osmux_xfrm_encode_amr(struct osmux_in_handle *h, static struct msgb *osmux_build_batch(struct osmux_in_handle *h) { struct msgb *batch_msg; - struct batch_list_node *node, *tnode; + struct osmux_circuit *circuit, *next; struct osmux_batch *batch = (struct osmux_batch *)h->internal_data;
#ifdef DEBUG_MSG @@ -301,15 +301,15 @@ static struct msgb *osmux_build_batch(struct osmux_in_handle *h) return NULL; }
- llist_for_each_entry_safe(node, tnode, &batch->node_list, head) { + llist_for_each_entry_safe(circuit, next, &batch->circuit_list, head) { struct msgb *cur, *tmp; int ctr = 0;
- llist_for_each_entry_safe(cur, tmp, &node->list, list) { + llist_for_each_entry_safe(cur, tmp, &circuit->list, list) { struct osmux_input_state state = { .msg = cur, .out_msg = batch_msg, - .ccid = node->ccid, + .ccid = circuit->ccid, }; #ifdef DEBUG_MSG char buf[4096]; @@ -331,12 +331,12 @@ static struct msgb *osmux_build_batch(struct osmux_in_handle *h) }
osmux_xfrm_encode_amr(h, &state); - osmux_batch_dequeue(cur, node); + osmux_batch_dequeue(cur, circuit); msgb_free(cur); ctr++; } - llist_del(&node->head); - talloc_free(node); + llist_del(&circuit->head); + talloc_free(circuit); } return batch_msg; } @@ -394,7 +394,7 @@ static int osmux_rtp_amr_payload_len(struct msgb *msg, struct rtp_hdr *rtph) return amr_payload_len; }
-static void osmux_replay_lost_packets(struct batch_list_node *node, +static void osmux_replay_lost_packets(struct osmux_circuit *circuit, struct rtp_hdr *cur_rtph) { int16_t diff; @@ -403,11 +403,11 @@ static void osmux_replay_lost_packets(struct batch_list_node *node, int i;
/* Have we see any RTP packet in this batch before? */ - if (llist_empty(&node->list)) + if (llist_empty(&circuit->list)) return;
/* Get last RTP packet seen in this batch */ - last = llist_entry(node->list.prev, struct msgb, list); + last = llist_entry(circuit->list.prev, struct msgb, list); rtph = osmo_rtp_get_hdr(last); if (rtph == NULL) return; @@ -441,7 +441,7 @@ static void osmux_replay_lost_packets(struct batch_list_node *node, DELTA_RTP_TIMESTAMP);
/* No more room in this batch, skip padding with more clones */ - if (osmux_batch_enqueue(clone, node) < 0) { + if (osmux_batch_enqueue(clone, circuit) < 0) { msgb_free(clone); break; } @@ -450,22 +450,22 @@ static void osmux_replay_lost_packets(struct batch_list_node *node, } }
-static struct batch_list_node * +static struct osmux_circuit * osmux_batch_find_circuit(struct osmux_batch *batch, int ccid) { - struct batch_list_node *circuit; + struct osmux_circuit *circuit;
- llist_for_each_entry(circuit, &batch->node_list, head) { + llist_for_each_entry(circuit, &batch->circuit_list, head) { if (circuit->ccid == ccid) return circuit; } return NULL; }
-static struct batch_list_node * +static struct osmux_circuit * osmux_batch_add_circuit(struct osmux_batch *batch, int ccid) { - struct batch_list_node *circuit; + struct osmux_circuit *circuit;
circuit = osmux_batch_find_circuit(batch, ccid); if (circuit != NULL) { @@ -473,7 +473,7 @@ osmux_batch_add_circuit(struct osmux_batch *batch, int ccid) return NULL; }
- circuit = talloc_zero(osmux_ctx, struct batch_list_node); + circuit = talloc_zero(osmux_ctx, struct osmux_circuit); if (circuit == NULL) { LOGP(DLMIB, LOGL_ERROR, "OOM on circuit %u\n", ccid); return NULL; @@ -481,7 +481,7 @@ osmux_batch_add_circuit(struct osmux_batch *batch, int ccid)
circuit->ccid = ccid; INIT_LLIST_HEAD(&circuit->list); - llist_add_tail(&circuit->head, &batch->node_list); + llist_add_tail(&circuit->head, &batch->circuit_list);
return circuit; } @@ -491,7 +491,7 @@ osmux_batch_add(struct osmux_batch *batch, struct msgb *msg, struct rtp_hdr *rtph, int ccid) { int bytes = 0, amr_payload_len; - struct batch_list_node *circuit; + struct osmux_circuit *circuit;
circuit = osmux_batch_find_circuit(batch, ccid);
@@ -597,7 +597,7 @@ int osmux_xfrm_input(struct osmux_in_handle *h, struct msgb *msg, int ccid) /* This is the first message in the batch, start the * batch timer to deliver it. */ - first_rtp_msg = llist_empty(&batch->node_list) ? 1 : 0; + first_rtp_msg = llist_empty(&batch->circuit_list) ? 1 : 0;
/* Add this RTP to the OSMUX batch */ ret = osmux_batch_add(batch, msg, rtph, ccid); @@ -638,7 +638,7 @@ void osmux_xfrm_input_init(struct osmux_in_handle *h) if (batch == NULL) return;
- INIT_LLIST_HEAD(&batch->node_list); + INIT_LLIST_HEAD(&batch->circuit_list); batch->remaining_bytes = h->batch_size; batch->timer.cb = osmux_batch_timer_expired; batch->timer.data = h; @@ -651,11 +651,11 @@ void osmux_xfrm_input_init(struct osmux_in_handle *h) void osmux_xfrm_input_fini(struct osmux_in_handle *h) { struct osmux_batch *batch = (struct osmux_batch *)h->internal_data; - struct batch_list_node *node, *next; + struct osmux_circuit *circuit, *next;
- llist_for_each_entry_safe(node, next, &batch->node_list, head) { - llist_del(&node->head); - talloc_free(node); + llist_for_each_entry_safe(circuit, next, &batch->circuit_list, head) { + llist_del(&circuit->head); + talloc_free(circuit); } osmo_timer_del(&batch->timer); talloc_free(batch);
From: Pablo Neira Ayuso pablo@soleta.eu
A circuit object contains a list of pending RTP messages to be converted to the osmux format. --- src/osmux.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/src/osmux.c b/src/osmux.c index 81ad9dc..187ca1c 100644 --- a/src/osmux.c +++ b/src/osmux.c @@ -189,7 +189,7 @@ struct osmux_batch { struct osmux_circuit { struct llist_head head; int ccid; - struct llist_head list; + struct llist_head msg_list; int nmsgs; };
@@ -210,7 +210,7 @@ static int osmux_batch_enqueue(struct msgb *msg, struct osmux_circuit *circuit) return -1; }
- llist_add_tail(&msg->list, &circuit->list); + llist_add_tail(&msg->list, &circuit->msg_list); circuit->nmsgs++; return 0; } @@ -305,7 +305,7 @@ static struct msgb *osmux_build_batch(struct osmux_in_handle *h) struct msgb *cur, *tmp; int ctr = 0;
- llist_for_each_entry_safe(cur, tmp, &circuit->list, list) { + llist_for_each_entry_safe(cur, tmp, &circuit->msg_list, list) { struct osmux_input_state state = { .msg = cur, .out_msg = batch_msg, @@ -403,11 +403,11 @@ static void osmux_replay_lost_packets(struct osmux_circuit *circuit, int i;
/* Have we see any RTP packet in this batch before? */ - if (llist_empty(&circuit->list)) + if (llist_empty(&circuit->msg_list)) return;
/* Get last RTP packet seen in this batch */ - last = llist_entry(circuit->list.prev, struct msgb, list); + last = llist_entry(circuit->msg_list.prev, struct msgb, list); rtph = osmo_rtp_get_hdr(last); if (rtph == NULL) return; @@ -480,7 +480,7 @@ osmux_batch_add_circuit(struct osmux_batch *batch, int ccid) }
circuit->ccid = ccid; - INIT_LLIST_HEAD(&circuit->list); + INIT_LLIST_HEAD(&circuit->msg_list); llist_add_tail(&circuit->head, &batch->circuit_list);
return circuit; @@ -515,7 +515,7 @@ osmux_batch_add(struct osmux_batch *batch, struct msgb *msg, * should not happen but make sure we don't propagate * duplicated messages. */ - llist_for_each_entry(cur, &circuit->list, list) { + llist_for_each_entry(cur, &circuit->msg_list, list) { struct rtp_hdr *rtph2 = osmo_rtp_get_hdr(cur); if (rtph2 == NULL) return -1;
From: Pablo Neira Ayuso pablo@soleta.eu
Instead of struct osmux_in_handle. This object contains the internal batching state information. --- src/osmux.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/src/osmux.c b/src/osmux.c index 187ca1c..bd400f6 100644 --- a/src/osmux.c +++ b/src/osmux.c @@ -231,10 +231,9 @@ struct osmux_input_state { int add_osmux_hdr; };
-static int osmux_batch_put(struct osmux_in_handle *h, +static int osmux_batch_put(struct osmux_batch *batch, struct osmux_input_state *state) { - struct osmux_batch *batch = (struct osmux_batch *)h->internal_data; struct osmux_hdr *osmuxh;
if (state->add_osmux_hdr) { @@ -268,7 +267,7 @@ static int osmux_batch_put(struct osmux_in_handle *h, return 0; }
-static int osmux_xfrm_encode_amr(struct osmux_in_handle *h, +static int osmux_xfrm_encode_amr(struct osmux_batch *batch, struct osmux_input_state *state) { uint32_t amr_len; @@ -279,23 +278,23 @@ static int osmux_xfrm_encode_amr(struct osmux_in_handle *h,
state->amr_payload_len = amr_len - sizeof(struct amr_hdr);
- if (osmux_batch_put(h, state) < 0) + if (osmux_batch_put(batch, state) < 0) return -1;
return 0; }
-static struct msgb *osmux_build_batch(struct osmux_in_handle *h) +static struct msgb *osmux_build_batch(struct osmux_batch *batch, + uint32_t batch_size) { struct msgb *batch_msg; struct osmux_circuit *circuit, *next; - struct osmux_batch *batch = (struct osmux_batch *)h->internal_data;
#ifdef DEBUG_MSG LOGP(DLMIB, LOGL_DEBUG, "Now building batch\n"); #endif
- batch_msg = msgb_alloc(h->batch_size, "osmux"); + batch_msg = msgb_alloc(batch_size, "osmux"); if (batch_msg == NULL) { LOGP(DLMIB, LOGL_ERROR, "Not enough memory\n"); return NULL; @@ -330,7 +329,7 @@ static struct msgb *osmux_build_batch(struct osmux_in_handle *h) state.add_osmux_hdr = 1; }
- osmux_xfrm_encode_amr(h, &state); + osmux_xfrm_encode_amr(batch, &state); osmux_batch_dequeue(cur, circuit); msgb_free(cur); ctr++; @@ -349,7 +348,7 @@ void osmux_xfrm_input_deliver(struct osmux_in_handle *h) #ifdef DEBUG_MSG LOGP(DLMIB, LOGL_DEBUG, "invoking delivery function\n"); #endif - batch_msg = osmux_build_batch(h); + batch_msg = osmux_build_batch(batch, h->batch_size);
h->stats.output_osmux_msgs++; h->stats.output_osmux_bytes += batch_msg->len;
From: Pablo Neira Ayuso pablo@soleta.eu
Add a new field to count the number of messages in the batch that are pending to be transformed to osmux. Use this new field to check when to enable the osmux timer.
The follow up patch keeps circuit objects in the batch until they are closed, so we won't be able to rely on this to know when to enable the timer anymore. --- src/osmux.c | 32 +++++++++++++++----------------- 1 file changed, 15 insertions(+), 17 deletions(-)
diff --git a/src/osmux.c b/src/osmux.c index bd400f6..82ec56e 100644 --- a/src/osmux.c +++ b/src/osmux.c @@ -184,6 +184,7 @@ struct osmux_batch { struct llist_head circuit_list; unsigned int remaining_bytes; uint8_t seq; + uint32_t nmsgs; };
struct osmux_circuit { @@ -333,6 +334,7 @@ static struct msgb *osmux_build_batch(struct osmux_batch *batch, osmux_batch_dequeue(cur, circuit); msgb_free(cur); ctr++; + batch->nmsgs--; } llist_del(&circuit->head); talloc_free(circuit); @@ -486,7 +488,7 @@ osmux_batch_add_circuit(struct osmux_batch *batch, int ccid) }
static int -osmux_batch_add(struct osmux_batch *batch, struct msgb *msg, +osmux_batch_add(struct osmux_batch *batch, int batch_factor, struct msgb *msg, struct rtp_hdr *rtph, int ccid) { int bytes = 0, amr_payload_len; @@ -549,6 +551,15 @@ osmux_batch_add(struct osmux_batch *batch, struct msgb *msg, /* Update remaining room in this batch */ batch->remaining_bytes -= bytes;
+ if (batch->nmsgs == 0) { +#ifdef DEBUG_MSG + LOGP(DLMIB, LOGL_DEBUG, "osmux start timer batch\n"); +#endif + osmo_timer_schedule(&batch->timer, 0, + batch_factor * DELTA_RTP_MSG); + } + batch->nmsgs++; + return 0; }
@@ -565,7 +576,7 @@ osmux_batch_add(struct osmux_batch *batch, struct msgb *msg, */ int osmux_xfrm_input(struct osmux_in_handle *h, struct msgb *msg, int ccid) { - int ret, first_rtp_msg; + int ret; struct rtp_hdr *rtph; struct osmux_batch *batch = (struct osmux_batch *)h->internal_data;
@@ -593,13 +604,9 @@ int osmux_xfrm_input(struct osmux_in_handle *h, struct msgb *msg, int ccid) * receive AMR traffic. */
- /* This is the first message in the batch, start the - * batch timer to deliver it. - */ - first_rtp_msg = llist_empty(&batch->circuit_list) ? 1 : 0; - /* Add this RTP to the OSMUX batch */ - ret = osmux_batch_add(batch, msg, rtph, ccid); + ret = osmux_batch_add(batch, h->batch_factor, + msg, rtph, ccid); if (ret < 0) { /* Cannot put this message into the batch. * Malformed, duplicated, OOM. Drop it and tell @@ -611,15 +618,6 @@ int osmux_xfrm_input(struct osmux_in_handle *h, struct msgb *msg, int ccid)
h->stats.input_rtp_msgs++; h->stats.input_rtp_bytes += msg->len; - - if (first_rtp_msg) { -#ifdef DEBUG_MSG - LOGP(DLMIB, LOGL_DEBUG, - "osmux start timer batch\n"); -#endif - osmo_timer_schedule(&batch->timer, 0, - h->batch_factor * DELTA_RTP_MSG); - } break; } return ret;
On 21 Jul 2015, at 16:23, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@soleta.eu
Add a new field to count the number of messages in the batch that are pending to be transformed to osmux. Use this new field to check when to enable the osmux timer.
okay, when one counts a list one needs to be careful to have it balanced. While having a look if it is balanced I noticed this in the original code:
static struct msgb *osmux_build_batch(struct osmux_in_handle *h) {
…
llist_for_each_entry_safe(node, tnode, &batch->node_list, head) { struct msgb *cur, *tmp; int ctr = 0;
llist_for_each_entry_safe(cur, tmp, &node->list, list) { struct rtp_hdr *rtph; int add_osmux_hdr = 0;
#ifdef DEBUG_MSG ... #endif
rtph = osmo_rtp_get_hdr(cur); if (rtph == NULL) return NULL;
When and how will this package be dropped?
Patches 4-7 look good too!
On Sat, Aug 08, 2015 at 09:00:30PM +0200, Holger Freyther wrote:
On 21 Jul 2015, at 16:23, pablo@gnumonks.org wrote:
From: Pablo Neira Ayuso pablo@soleta.eu
Add a new field to count the number of messages in the batch that are pending to be transformed to osmux. Use this new field to check when to enable the osmux timer.
okay, when one counts a list one needs to be careful to have it balanced. While having a look if it is balanced I noticed this in the original code:
static struct msgb *osmux_build_batch(struct osmux_in_handle *h) {
…
llist_for_each_entry_safe(node, tnode, &batch->node_list, head) { struct msgb *cur, *tmp; int ctr = 0; llist_for_each_entry_safe(cur, tmp, &node->list, list) { struct rtp_hdr *rtph; int add_osmux_hdr = 0;#ifdef DEBUG_MSG ... #endif
rtph = osmo_rtp_get_hdr(cur); if (rtph == NULL) return NULL;When and how will this package be dropped?
If the RTP message that we received is malformed.
Although we already checked this when enqueuing the packet into the circuit list before the transformation happens, so it should be safe to remove the NULL check.
But let me revisit this in a follow up patch.
Patches 4-7 look good too!
Thanks Holger.
From: Pablo Neira Ayuso pablo@soleta.eu
Add this new function to explicitly remove an existing circuit. Thus, the client application (openbsc) is in full control to release circuits.
Before this patch, the circuit object was added when the first RTP messages was seen, and it was removed when transforming the list of pending RTP messages to the Osmux message (once the timer expired).
Moreover, check circuit->nmsgs to account bytes that are consumed by the osmux header, given that !circuit doesn't mean "this is the first packet" anymore. --- include/osmocom/netif/osmux.h | 2 ++ src/osmux.c | 27 ++++++++++++++++++++++----- 2 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/include/osmocom/netif/osmux.h b/include/osmocom/netif/osmux.h index 0f6f5c9..14c967f 100644 --- a/include/osmocom/netif/osmux.h +++ b/include/osmocom/netif/osmux.h @@ -84,6 +84,8 @@ int osmux_snprintf(char *buf, size_t size, struct msgb *msg); void osmux_xfrm_input_init(struct osmux_in_handle *h); void osmux_xfrm_input_fini(struct osmux_in_handle *h);
+void osmux_xfrm_input_close_circuit(struct osmux_in_handle *h, int ccid); + int osmux_xfrm_input(struct osmux_in_handle *h, struct msgb *msg, int ccid); void osmux_xfrm_input_deliver(struct osmux_in_handle *h);
diff --git a/src/osmux.c b/src/osmux.c index 82ec56e..4451b5a 100644 --- a/src/osmux.c +++ b/src/osmux.c @@ -289,7 +289,7 @@ static struct msgb *osmux_build_batch(struct osmux_batch *batch, uint32_t batch_size) { struct msgb *batch_msg; - struct osmux_circuit *circuit, *next; + struct osmux_circuit *circuit;
#ifdef DEBUG_MSG LOGP(DLMIB, LOGL_DEBUG, "Now building batch\n"); @@ -301,7 +301,7 @@ static struct msgb *osmux_build_batch(struct osmux_batch *batch, return NULL; }
- llist_for_each_entry_safe(circuit, next, &batch->circuit_list, head) { + llist_for_each_entry(circuit, &batch->circuit_list, head) { struct msgb *cur, *tmp; int ctr = 0;
@@ -336,8 +336,6 @@ static struct msgb *osmux_build_batch(struct osmux_batch *batch, ctr++; batch->nmsgs--; } - llist_del(&circuit->head); - talloc_free(circuit); } return batch_msg; } @@ -487,6 +485,18 @@ osmux_batch_add_circuit(struct osmux_batch *batch, int ccid) return circuit; }
+static void osmux_batch_del_circuit(struct osmux_batch *batch, int ccid) +{ + struct osmux_circuit *circuit; + + circuit = osmux_batch_find_circuit(batch, ccid); + if (circuit == NULL) + return; + + llist_del(&circuit->head); + talloc_free(circuit); +} + static int osmux_batch_add(struct osmux_batch *batch, int batch_factor, struct msgb *msg, struct rtp_hdr *rtph, int ccid) @@ -502,7 +512,7 @@ osmux_batch_add(struct osmux_batch *batch, int batch_factor, struct msgb *msg,
/* First check if there is room for this message in the batch */ bytes += amr_payload_len; - if (!circuit) + if (!circuit || circuit->nmsgs == 0) bytes += sizeof(struct osmux_hdr);
/* No room, sorry. You'll have to retry */ @@ -645,6 +655,13 @@ void osmux_xfrm_input_init(struct osmux_in_handle *h) LOGP(DLMIB, LOGL_DEBUG, "initialized osmux input converter\n"); }
+void osmux_xfrm_input_close_circuit(struct osmux_in_handle *h, int ccid) +{ + struct osmux_batch *batch = (struct osmux_batch *)h->internal_data; + + osmux_batch_del_circuit(batch, ccid); +} + void osmux_xfrm_input_fini(struct osmux_in_handle *h) { struct osmux_batch *batch = (struct osmux_batch *)h->internal_data;
On 21 Jul 2015, at 16:23, pablo@gnumonks.org wrote:
- llist_del(&circuit->head);
- talloc_free(circuit);
What happens if there is a single circuit and the batch timer was running? I assume it will timeout not be rescheduled?
On Sat, Aug 08, 2015 at 09:04:12PM +0200, Holger Freyther wrote:
On 21 Jul 2015, at 16:23, pablo@gnumonks.org wrote:
- llist_del(&circuit->head);
- talloc_free(circuit);
What happens if there is a single circuit and the batch timer was running? I assume it will timeout not be rescheduled?
Good point.
Looking at osmux_xfrm_input_deliver() and assuming the ndummy counter became zero, it will not be rescheduled.
Regarding the normal voice path, I think it should be possible to pass an empty msgb to the deliver() callback if the timer is called after the circuit is gone, let me have a careful look at this.
From: Pablo Neira Ayuso pablo@soleta.eu
This new function allows you to create a circuit on an existing input handle.
We don't create the circuit anymore from the first packet seen, instead the client application is in full control of opening and closing the circuit.
This change includes a new feature to pad a circuit until we see the first packet that contains voice data. This is useful to preallocate bandwidth on satellite links such as Iridium/OpenPort. --- include/osmocom/netif/osmux.h | 4 +- src/osmux.c | 133 +++++++++++++++++++++++++++++++---------- 2 files changed, 103 insertions(+), 34 deletions(-)
diff --git a/include/osmocom/netif/osmux.h b/include/osmocom/netif/osmux.h index 14c967f..c1f527a 100644 --- a/include/osmocom/netif/osmux.h +++ b/include/osmocom/netif/osmux.h @@ -5,7 +5,7 @@
/* OSmux header: * - * ft (3 bits): 0=signalling, 1=voice + * ft (3 bits): 0=signalling, 1=voice, 2=dummy * ctr (3 bits): Number of batched AMR payloads (starting 0) * amr_f (1 bit): AMR F field (RFC3267) * amr_q (1 bit): AMR Q field (RFC3267) @@ -17,6 +17,7 @@
#define OSMUX_FT_SIGNAL 0 #define OSMUX_FT_VOICE_AMR 1 +#define OSMUX_FT_DUMMY 2
struct osmux_hdr { #if OSMO_IS_BIG_ENDIAN @@ -84,6 +85,7 @@ int osmux_snprintf(char *buf, size_t size, struct msgb *msg); void osmux_xfrm_input_init(struct osmux_in_handle *h); void osmux_xfrm_input_fini(struct osmux_in_handle *h);
+int osmux_xfrm_input_open_circuit(struct osmux_in_handle *h, int ccid, int dummy); void osmux_xfrm_input_close_circuit(struct osmux_in_handle *h, int ccid);
int osmux_xfrm_input(struct osmux_in_handle *h, struct msgb *msg, int ccid); diff --git a/src/osmux.c b/src/osmux.c index 4451b5a..74883d9 100644 --- a/src/osmux.c +++ b/src/osmux.c @@ -56,16 +56,29 @@ static uint32_t osmux_get_payload_len(struct osmux_hdr *osmuxh) return osmo_amr_bytes(osmuxh->amr_ft) * (osmuxh->ctr+1); }
-struct osmux_hdr *osmux_xfrm_output_pull(struct msgb *msg) +static uint32_t osmux_ft_dummy_size(uint8_t amr_ft, uint32_t batch_factor) { - struct osmux_hdr *osmuxh = NULL; + return sizeof(struct osmux_hdr) + (osmo_amr_bytes(amr_ft) * batch_factor); +}
+struct osmux_hdr *osmux_xfrm_output_pull(struct msgb *msg) +{ + struct osmux_hdr *osmuxh; +next: + osmuxh = NULL; if (msg->len > sizeof(struct osmux_hdr)) { size_t len;
osmuxh = (struct osmux_hdr *)msg->data;
- if (osmuxh->ft != OSMUX_FT_VOICE_AMR) { + switch (osmuxh->ft) { + case OSMUX_FT_VOICE_AMR: + break; + case OSMUX_FT_DUMMY: + msgb_pull(msg, osmux_ft_dummy_size(osmuxh->amr_ft, + osmuxh->ctr + 1)); + goto next; + default: LOGP(DLMIB, LOGL_ERROR, "Discarding unsupported Osmux FT %d\n", osmuxh->ft); return NULL; @@ -185,6 +198,7 @@ struct osmux_batch { unsigned int remaining_bytes; uint8_t seq; uint32_t nmsgs; + int dummy; };
struct osmux_circuit { @@ -192,6 +206,7 @@ struct osmux_circuit { int ccid; struct llist_head msg_list; int nmsgs; + int dummy; };
static int osmux_batch_enqueue(struct msgb *msg, struct osmux_circuit *circuit) @@ -285,8 +300,31 @@ static int osmux_xfrm_encode_amr(struct osmux_batch *batch, return 0; }
+static void osmux_encode_dummy(struct osmux_batch *batch, uint32_t batch_factor, + struct osmux_input_state *state) +{ + struct osmux_hdr *osmuxh; + /* TODO: This should be configurable at some point. */ + uint32_t payload_size = osmux_ft_dummy_size(AMR_FT_3, batch_factor) - + sizeof(struct osmux_hdr); + + osmuxh = (struct osmux_hdr *)state->out_msg->tail; + osmuxh->ft = OSMUX_FT_DUMMY; + osmuxh->ctr = batch_factor - 1; + osmuxh->amr_f = 0; + osmuxh->amr_q= 0; + osmuxh->seq = 0; + osmuxh->circuit_id = state->ccid; + osmuxh->amr_cmr = 0; + osmuxh->amr_ft = AMR_FT_3; + msgb_put(state->out_msg, sizeof(struct osmux_hdr)); + + memset(state->out_msg->tail, 0xff, payload_size); + msgb_put(state->out_msg, payload_size); +} + static struct msgb *osmux_build_batch(struct osmux_batch *batch, - uint32_t batch_size) + uint32_t batch_size, uint32_t batch_factor) { struct msgb *batch_msg; struct osmux_circuit *circuit; @@ -305,6 +343,15 @@ static struct msgb *osmux_build_batch(struct osmux_batch *batch, struct msgb *cur, *tmp; int ctr = 0;
+ if (circuit->dummy) { + struct osmux_input_state state = { + .out_msg = batch_msg, + .ccid = circuit->ccid, + }; + osmux_encode_dummy(batch, batch_factor, &state); + continue; + } + llist_for_each_entry_safe(cur, tmp, &circuit->msg_list, list) { struct osmux_input_state state = { .msg = cur, @@ -348,7 +395,7 @@ void osmux_xfrm_input_deliver(struct osmux_in_handle *h) #ifdef DEBUG_MSG LOGP(DLMIB, LOGL_DEBUG, "invoking delivery function\n"); #endif - batch_msg = osmux_build_batch(batch, h->batch_size); + batch_msg = osmux_build_batch(batch, h->batch_size, h->batch_factor);
h->stats.output_osmux_msgs++; h->stats.output_osmux_bytes += batch_msg->len; @@ -356,6 +403,11 @@ void osmux_xfrm_input_deliver(struct osmux_in_handle *h) h->deliver(batch_msg, h->data); osmo_timer_del(&batch->timer); batch->remaining_bytes = h->batch_size; + + if (batch->dummy) { + osmo_timer_schedule(&batch->timer, 0, + h->batch_factor * DELTA_RTP_MSG); + } }
static void osmux_batch_timer_expired(void *data) @@ -462,7 +514,8 @@ osmux_batch_find_circuit(struct osmux_batch *batch, int ccid) }
static struct osmux_circuit * -osmux_batch_add_circuit(struct osmux_batch *batch, int ccid) +osmux_batch_add_circuit(struct osmux_batch *batch, int ccid, int dummy, + int batch_factor) { struct osmux_circuit *circuit;
@@ -482,6 +535,13 @@ osmux_batch_add_circuit(struct osmux_batch *batch, int ccid) INIT_LLIST_HEAD(&circuit->msg_list); llist_add_tail(&circuit->head, &batch->circuit_list);
+ if (dummy) { + circuit->dummy = dummy; + batch->dummy++; + if (!osmo_timer_pending(&batch->timer)) + osmo_timer_schedule(&batch->timer, 0, + batch_factor * DELTA_RTP_MSG); + } return circuit; }
@@ -493,6 +553,8 @@ static void osmux_batch_del_circuit(struct osmux_batch *batch, int ccid) if (circuit == NULL) return;
+ if (circuit->dummy) + batch->dummy--; llist_del(&circuit->head); talloc_free(circuit); } @@ -503,51 +565,48 @@ osmux_batch_add(struct osmux_batch *batch, int batch_factor, struct msgb *msg, { int bytes = 0, amr_payload_len; struct osmux_circuit *circuit; + struct msgb *cur;
circuit = osmux_batch_find_circuit(batch, ccid); + if (!circuit) + return -1;
+ /* We've seen the first RTP message, disable dummy padding */ + if (circuit->dummy) { + circuit->dummy = 0; + batch->dummy--; + } amr_payload_len = osmux_rtp_amr_payload_len(msg, rtph); if (amr_payload_len < 0) return -1;
/* First check if there is room for this message in the batch */ bytes += amr_payload_len; - if (!circuit || circuit->nmsgs == 0) + if (circuit->nmsgs == 0) bytes += sizeof(struct osmux_hdr);
/* No room, sorry. You'll have to retry */ if (bytes > batch->remaining_bytes) return 1;
- if (circuit) { - struct msgb *cur; - - /* Extra validation: check if this message already exists, - * should not happen but make sure we don't propagate - * duplicated messages. - */ - llist_for_each_entry(cur, &circuit->msg_list, list) { - struct rtp_hdr *rtph2 = osmo_rtp_get_hdr(cur); - if (rtph2 == NULL) - return -1; - - /* Already exists message with this sequence, skip */ - if (rtph2->sequence == rtph->sequence) { - LOGP(DLMIB, LOGL_ERROR, "already exists " - "message with seq=%u, skip it\n", - rtph->sequence); - return -1; - } - } - /* Handle RTP packet loss scenario */ - osmux_replay_lost_packets(circuit, rtph); + /* Extra validation: check if this message already exists, should not + * happen but make sure we don't propagate duplicated messages. + */ + llist_for_each_entry(cur, &circuit->msg_list, list) { + struct rtp_hdr *rtph2 = osmo_rtp_get_hdr(cur); + if (rtph2 == NULL) + return -1;
- } else { - /* This is the first message with that ssrc we've seen */ - circuit = osmux_batch_add_circuit(batch, ccid); - if (!circuit) + /* Already exists message with this sequence, skip */ + if (rtph2->sequence == rtph->sequence) { + LOGP(DLMIB, LOGL_ERROR, "already exists " + "message with seq=%u, skip it\n", + rtph->sequence); return -1; + } } + /* Handle RTP packet loss scenario */ + osmux_replay_lost_packets(circuit, rtph);
/* This batch is full, force batch delivery */ if (osmux_batch_enqueue(msg, circuit) < 0) @@ -655,6 +714,14 @@ void osmux_xfrm_input_init(struct osmux_in_handle *h) LOGP(DLMIB, LOGL_DEBUG, "initialized osmux input converter\n"); }
+int osmux_xfrm_input_open_circuit(struct osmux_in_handle *h, int ccid, + int dummy) +{ + struct osmux_batch *batch = (struct osmux_batch *)h->internal_data; + + return osmux_batch_add_circuit(batch, ccid, dummy, h->batch_factor) ? 0 : -1; +} + void osmux_xfrm_input_close_circuit(struct osmux_in_handle *h, int ccid) { struct osmux_batch *batch = (struct osmux_batch *)h->internal_data;
On Sat, Aug 08, 2015 at 09:08:53PM +0200, Holger Freyther wrote:
On 21 Jul 2015, at 16:23, pablo@gnumonks.org wrote:
@@ -185,6 +198,7 @@ struct osmux_batch { unsigned int remaining_bytes; uint8_t seq; uint32_t nmsgs;
- int dummy;
ndummy?
OK, will rename this.
From: Pablo Neira Ayuso pablo@soleta.eu
This also introduces a spare circuit that contains no voice data to test bandwidth preallocation through the new osmux dummy frame type. --- tests/osmux/osmux_test.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/tests/osmux/osmux_test.c b/tests/osmux/osmux_test.c index dae9aa2..1b87db7 100644 --- a/tests/osmux/osmux_test.c +++ b/tests/osmux/osmux_test.c @@ -135,6 +135,13 @@ int main(void) /* If the test takes longer than 10 seconds, abort it */ alarm(10);
+ for (i = 0; i < 2; i++) + osmux_xfrm_input_open_circuit(&h_input, i, 0); + + /* Add two circuits with dummy padding */ + osmux_xfrm_input_open_circuit(&h_input, 2, 1); + osmux_xfrm_input_open_circuit(&h_input, 3, 1); + for (i=1; i<64; i++) { msg = msgb_alloc(1500, "test"); if (!msg) @@ -178,6 +185,12 @@ int main(void) k = 0; } } + + for (i = 0; i < 4; i++) + osmux_xfrm_input_close_circuit(&h_input, i); + + osmux_xfrm_input_fini(&h_input); + fprintf(stdout, "OK: Test passed\n"); return EXIT_SUCCESS; }
From: Pablo Neira Ayuso pablo@soleta.eu
Move main test loop routine to the new osmux_test_loop() function. --- tests/osmux/osmux_test.c | 64 ++++++++++++++++++++++++++-------------------- 1 file changed, 36 insertions(+), 28 deletions(-)
diff --git a/tests/osmux/osmux_test.c b/tests/osmux/osmux_test.c index 1b87db7..51f026c 100644 --- a/tests/osmux/osmux_test.c +++ b/tests/osmux/osmux_test.c @@ -112,7 +112,7 @@ static void sigalarm_handler(int foo) exit(EXIT_FAILURE); }
-int main(void) +static void osmux_test_loop(int ccid) { struct msgb *msg; char buf[1024]; @@ -120,32 +120,10 @@ int main(void) uint16_t seq; int i, j, k = 0;
- if (signal(SIGALRM, sigalarm_handler) == SIG_ERR) { - perror("signal"); - exit(EXIT_FAILURE); - } - - /* This test doesn't use it, but osmux requires it internally. */ - osmo_init_logging(&osmux_test_log_info); - log_set_log_level(osmo_stderr_target, LOGL_DEBUG); - - osmux_xfrm_input_init(&h_input); - osmux_xfrm_output_init(&h_output, 0x7000000); - - /* If the test takes longer than 10 seconds, abort it */ - alarm(10); - - for (i = 0; i < 2; i++) - osmux_xfrm_input_open_circuit(&h_input, i, 0); - - /* Add two circuits with dummy padding */ - osmux_xfrm_input_open_circuit(&h_input, 2, 1); - osmux_xfrm_input_open_circuit(&h_input, 3, 1); - - for (i=1; i<64; i++) { + for (i = 1; i < 64; i++) { msg = msgb_alloc(1500, "test"); if (!msg) - return 0; + exit(EXIT_FAILURE);
memcpy(msg->data, rtp_pkt, sizeof(rtp_pkt)); msgb_put(msg, sizeof(rtp_pkt)); @@ -155,7 +133,7 @@ int main(void) rtph->sequence = htons(seq);
osmo_rtp_snprintf(buf, sizeof(buf), msg); - fprintf(stderr, "adding to ccid=%u %s\n", i % 2, buf); + fprintf(stderr, "adding to ccid=%u %s\n", (i % 2) + ccid, buf); rtp_pkts++;
k++; @@ -164,7 +142,7 @@ int main(void) * gaps between two messages to test the osmux replaying * feature. */ - osmux_xfrm_input(&h_input, msg, i % 2); + osmux_xfrm_input(&h_input, msg, (i % 2) + ccid);
if (i % 4 == 0) { gettimeofday(&last, NULL); @@ -179,12 +157,42 @@ int main(void) * messages that are extracted from OSMUX has been * delivered. */ - for (j=0; j<k-2; j++) + for (j = 0; j < k-2; j++) osmo_select_main(0);
k = 0; } } +} + +int main(void) +{ + int i; + + if (signal(SIGALRM, sigalarm_handler) == SIG_ERR) { + perror("signal"); + exit(EXIT_FAILURE); + } + + /* This test doesn't use it, but osmux requires it internally. */ + osmo_init_logging(&osmux_test_log_info); + log_set_log_level(osmo_stderr_target, LOGL_DEBUG); + + osmux_xfrm_input_init(&h_input); + osmux_xfrm_output_init(&h_output, 0x7000000); + + /* If the test takes longer than 10 seconds, abort it */ + alarm(10); + + for (i = 0; i < 2; i++) + osmux_xfrm_input_open_circuit(&h_input, i, 0); + + /* Add two circuits with dummy padding */ + osmux_xfrm_input_open_circuit(&h_input, 2, 1); + osmux_xfrm_input_open_circuit(&h_input, 3, 1); + + /* Start pushing voice data to circuits 0 and 1 */ + osmux_test_loop(0);
for (i = 0; i < 4; i++) osmux_xfrm_input_close_circuit(&h_input, i);
From: Pablo Neira Ayuso pablo@soleta.eu
Add RTP packets to circuit with dummy padding enabled to test automatic deactivation once when start seeing real voice traffic. --- tests/osmux/osmux_test.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/tests/osmux/osmux_test.c b/tests/osmux/osmux_test.c index 51f026c..09ef9fd 100644 --- a/tests/osmux/osmux_test.c +++ b/tests/osmux/osmux_test.c @@ -193,6 +193,10 @@ int main(void)
/* Start pushing voice data to circuits 0 and 1 */ osmux_test_loop(0); + /* ... now start pushing voice data to circuits 2 and 3. This circuits + * comes with dummy padding enabled. + */ + osmux_test_loop(2);
for (i = 0; i < 4; i++) osmux_xfrm_input_close_circuit(&h_input, i);
From: Pablo Neira Ayuso pablo@soleta.eu
Make sure that early dummy bandwitch preallocation works fine in the absence of any kind of voice traffic. --- tests/osmux/osmux_test.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/tests/osmux/osmux_test.c b/tests/osmux/osmux_test.c index 09ef9fd..0ad02d3 100644 --- a/tests/osmux/osmux_test.c +++ b/tests/osmux/osmux_test.c @@ -191,6 +191,10 @@ int main(void) osmux_xfrm_input_open_circuit(&h_input, 2, 1); osmux_xfrm_input_open_circuit(&h_input, 3, 1);
+ /* Wait 10 times to make sure dummy padding timer works fine */ + for (i = 0; i < 10; i++) + osmo_select_main(0); + /* Start pushing voice data to circuits 0 and 1 */ osmux_test_loop(0); /* ... now start pushing voice data to circuits 2 and 3. This circuits
From: Pablo Neira Ayuso pablo@soleta.eu
Make sure circuit routines works correctly. --- tests/osmux/osmux_test.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/tests/osmux/osmux_test.c b/tests/osmux/osmux_test.c index 0ad02d3..7cc838d 100644 --- a/tests/osmux/osmux_test.c +++ b/tests/osmux/osmux_test.c @@ -205,6 +205,13 @@ int main(void) for (i = 0; i < 4; i++) osmux_xfrm_input_close_circuit(&h_input, i);
+ /* Reopen with two circuits and retest */ + osmux_xfrm_input_open_circuit(&h_input, 0, 0); + osmux_xfrm_input_open_circuit(&h_input, 1, 1); + osmux_test_loop(0); + osmux_xfrm_input_close_circuit(&h_input, 0); + osmux_xfrm_input_close_circuit(&h_input, 1); + osmux_xfrm_input_fini(&h_input);
fprintf(stdout, "OK: Test passed\n");
From: Pablo Neira Ayuso pablo@soleta.eu
Never used, so let's get rid of this function. We can recover it later on in case we need it. --- include/osmocom/netif/osmux.h | 2 -- src/osmux.c | 12 ------------ 2 files changed, 14 deletions(-)
diff --git a/include/osmocom/netif/osmux.h b/include/osmocom/netif/osmux.h index c1f527a..83bb2e1 100644 --- a/include/osmocom/netif/osmux.h +++ b/include/osmocom/netif/osmux.h @@ -70,8 +70,6 @@ struct osmux_out_handle { uint32_t rtp_ssrc; };
-struct osmux_hdr *osmux_get_hdr(struct msgb *msg); - static inline uint8_t *osmux_get_payload(struct osmux_hdr *osmuxh) { return (uint8_t *)osmuxh + sizeof(struct osmux_hdr); diff --git a/src/osmux.c b/src/osmux.c index 74883d9..eedae69 100644 --- a/src/osmux.c +++ b/src/osmux.c @@ -39,18 +39,6 @@
static void *osmux_ctx;
-struct osmux_hdr *osmux_get_hdr(struct msgb *msg) -{ - struct osmux_hdr *osmuxh = (struct osmux_hdr *)msg->data; - - if (msg->len < sizeof(struct osmux_hdr)) { - DEBUGPC(DLMUX, "received OSMUX frame too short (len = %d)\n", - msg->len); - return NULL; - } - return osmuxh; -} - static uint32_t osmux_get_payload_len(struct osmux_hdr *osmuxh) { return osmo_amr_bytes(osmuxh->amr_ft) * (osmuxh->ctr+1);
From: Pablo Neira Ayuso pablo@soleta.eu
--- tests/osmux/osmux_test.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/tests/osmux/osmux_test.c b/tests/osmux/osmux_test.c index 7cc838d..9ceabcd 100644 --- a/tests/osmux/osmux_test.c +++ b/tests/osmux/osmux_test.c @@ -78,6 +78,7 @@ static void tx_cb(struct msgb *msg, void *data) }
rtp_pkts--; + msgb_free(msg); }
static struct osmux_out_handle h_output; @@ -98,6 +99,7 @@ static void osmux_deliver(struct msgb *batch_msg, void *data) osmux_xfrm_output(osmuxh, &h_output, &list); osmux_tx_sched(&list, tx_cb, NULL); } + msgb_free(batch_msg); }
struct osmux_in_handle h_input = {
From: Pablo Neira Ayuso pablo@soleta.eu
Useful when debuggin via valgrind/gdb. --- tests/Makefile.am | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/Makefile.am b/tests/Makefile.am index a986940..f99e276 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1,4 +1,4 @@ -AM_CFLAGS = -Wall -I$(top_srcdir)/include $(LIBOSMOCORE_CFLAGS) +AM_CFLAGS = -Wall -I$(top_srcdir)/include $(LIBOSMOCORE_CFLAGS) -g AM_LDFLAGS = $(LIBOSMOCORE_LDFLAGS)
check_PROGRAMS = osmux/osmux_test
On 21 Jul 2015, at 16:23, pablo@gnumonks.org wrote:
-AM_CFLAGS = -Wall -I$(top_srcdir)/include $(LIBOSMOCORE_CFLAGS) +AM_CFLAGS = -Wall -I$(top_srcdir)/include $(LIBOSMOCORE_CFLAGS) -g
I like -ggdb3 :)
In terms of the tests. You run the code but I don’t see many asserts. Could you please explain how the “no voice traffic” code tests? Would a lack of data end printing a message that would cause the test to fail?
holger
On Sat, Aug 08, 2015 at 09:11:39PM +0200, Holger Freyther wrote:
On 21 Jul 2015, at 16:23, pablo@gnumonks.org wrote:
-AM_CFLAGS = -Wall -I$(top_srcdir)/include $(LIBOSMOCORE_CFLAGS) +AM_CFLAGS = -Wall -I$(top_srcdir)/include $(LIBOSMOCORE_CFLAGS) -g
I like -ggdb3 :)
OK, will change this.
In terms of the tests. You run the code but I don’t see many asserts. Could you please explain how the “no voice traffic” code tests? Would a lack of data end printing a message that would cause the test to fail?
The test most likely will timeout if it doesn't get any voice traffic.
But I think it's a good idea to add more asserts.
In summary:
1) Will rename dummy to ndummy. 2) Will mangle this patch to replace -g to -ggdb3.
And I will push out this patchset, then will send two follow up patches:
3) Will revisit the one timer running + circuit gone scenario. This will come in a follow up patch since this problem since to be there since the beginning. 4) Will add more asserts to validate osmux outputs from tests.
Fine with this procedure? Let me know if I'm missing anything.
Thanks for reviewing.
On 10 Aug 2015, at 10:33, Pablo Neira Ayuso pablo@gnumonks.org wrote:
On Sat, Aug 08, 2015 at 09:11:39PM +0200, Holger Freyther wrote:
On 21 Jul 2015, at 16:23, pablo@gnumonks.org wrote:
-AM_CFLAGS = -Wall -I$(top_srcdir)/include $(LIBOSMOCORE_CFLAGS) +AM_CFLAGS = -Wall -I$(top_srcdir)/include $(LIBOSMOCORE_CFLAGS) -g
I like -ggdb3 :)
OK, will change this.
This was just personal taste. No need to update if -g was good enough for you. I tend to compile with CFLAGS=“-Og -ggdb3” make
Fine with this procedure? Let me know if I'm missing anything.
yes, that is fine.
On Mon, Aug 10, 2015 at 10:29:21AM +0200, Holger Freyther wrote: [...]
Fine with this procedure? Let me know if I'm missing anything.
yes, that is fine.
Just pushed out this to master, including a patch to rename dummy to ndummy as you suggested. Will follow up soon by reviewing the other concerns you spot during review.
Cheers from Seattle!
On 19 Aug 2015, at 02:52, Pablo Neira Ayuso pablo@gnumonks.org wrote:
Hi!
Just pushed out this to master, including a patch to rename dummy to ndummy as you suggested. Will follow up soon by reviewing the other concerns you spot during review.
the CI fails because of missing documentation strings in the new command
Documentation error (missing docs): <command id='osmux dummy (on|off)'> <param name='off' doc='(null)' />
Documentation error (missing docs): <command id='osmux dummy (on|off)'> <param name='off' doc='(null)' />
with the VTY code every parameter needs to be documented in this case it needs to be:
osmux dummy on off
Given the error message I think everything but off is documented.
enjoy the city!
holger
From: Pablo Neira Ayuso pablo@soleta.eu
Instead of 63, this resolves major "definitely lost" remaining leak that valgrind reports regarding msgb. --- tests/osmux/osmux_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/osmux/osmux_test.c b/tests/osmux/osmux_test.c index 9ceabcd..94b95de 100644 --- a/tests/osmux/osmux_test.c +++ b/tests/osmux/osmux_test.c @@ -122,7 +122,7 @@ static void osmux_test_loop(int ccid) uint16_t seq; int i, j, k = 0;
- for (i = 1; i < 64; i++) { + for (i = 1; i < 65; i++) { msg = msgb_alloc(1500, "test"); if (!msg) exit(EXIT_FAILURE);
On 21 Jul 2015, at 16:23, pablo@gnumonks.org wrote:
Hi!,
If no major concerns, I'll push this initial batch soon into the repository.
it is a bit late for me right now. The granularity of changes, comments, commit messages all look great. Please give me until tomorrow evening to have a second look.
holger
-
Holger Freyther -
Pablo Neira Ayuso -
pablo＠gnumonks.org