Attention is currently required from: plaisthos.
Hello plaisthos,
I'd like you to do a code review.
Please visit
http://gerrit.openvpn.net/c/openvpn/+/1961?usp=email
to review the following change.
Change subject: mbuf: don't count dereferenced items in the queue length
......................................................................
mbuf: don't count dereferenced items in the queue length
Items of a closed instance are cleared in place, because head + len is
the ring's insertion point. mbuf_extract_item() reclaims such holes as
it walks past them, but nothing does once no live item is left behind
them, so ms->len keeps counting slots that can never be sent.
mbuf_defined() and mbuf_peek() then disagree about whether the queue
holds anything, and a ring made of nothing but holes still looks full
to mbuf_add_item(), which tries to make room by dropping the oldest
packet and finds nothing it can drop.
A UDP socket hides this, as the IOW_MBUF write event reclaims the holes
on its way out; a TCP-only server has nothing that does.
Reclaim the holes once they reach the head, so that ms->len only counts
what can still be sent.
Change-Id: I9579ab9a3f588a3841bb90c9a4b930b7690e0b71
Signed-off-by: Gianmarco De Gregori <[email protected]>
---
M src/openvpn/mbuf.c
M tests/unit_tests/openvpn/test_mbuf.c
2 files changed, 112 insertions(+), 2 deletions(-)
git pull ssh://gerrit.openvpn.net:29418/openvpn refs/changes/61/1961/1
diff --git a/src/openvpn/mbuf.c b/src/openvpn/mbuf.c
index 7b790ed..6678a5b 100644
--- a/src/openvpn/mbuf.c
+++ b/src/openvpn/mbuf.c
@@ -85,6 +85,21 @@
}
}
+/*
+ * Dereferenced items cannot be removed from the middle of the ring, so
+ * reclaim them once they reach the head: ms->len must only count items
+ * which can still be sent.
+ */
+static void
+mbuf_reclaim_head(struct mbuf_set *ms)
+{
+ while (ms->len && !ms->array[ms->head].instance)
+ {
+ ms->head = MBUF_INDEX(ms->head, 1, ms->capacity);
+ --ms->len;
+ }
+}
+
void
mbuf_add_item(struct mbuf_set *ms, const struct mbuf_item *item)
{
@@ -125,6 +140,7 @@
break;
}
}
+ mbuf_reclaim_head(ms);
}
return ret;
}
@@ -164,5 +180,6 @@
msg(D_MBUF, "MBUF: dereferenced queued packet");
}
}
+ mbuf_reclaim_head(ms);
}
}
diff --git a/tests/unit_tests/openvpn/test_mbuf.c
b/tests/unit_tests/openvpn/test_mbuf.c
index cba4da7..e20e941 100644
--- a/tests/unit_tests/openvpn/test_mbuf.c
+++ b/tests/unit_tests/openvpn/test_mbuf.c
@@ -136,9 +136,9 @@
mbuf_dereference_instance(ms, &mi2);
assert_int_equal(mbuf_buf->refcount, 2);
assert_int_equal(mbuf_buf2->refcount, 1);
- assert_int_equal(mbuf_len(ms), 3);
+ assert_int_equal(mbuf_len(ms), 1);
assert_int_equal(mbuf_maximum_queued(ms), 4);
- assert_int_equal(ms->head, 3);
+ assert_int_equal(ms->head, 1);
assert_ptr_equal(mbuf_peek(ms), &mi);
mbuf_free(ms);
@@ -148,12 +148,105 @@
mbuf_free_buf(mbuf_buf2);
}
+/* a queue holding nothing but dereferenced items is an empty queue */
+static void
+test_mbuf_dereference_reclaims_queue(void **state)
+{
+ struct mbuf_set *ms = mbuf_init(4);
+ struct multi_instance mi = { 0 };
+ struct buffer buf = alloc_buf(16);
+ struct mbuf_buffer *mbuf_buf = mbuf_alloc_buf(&buf);
+ struct mbuf_item item = { .buffer = mbuf_buf, .instance = &mi };
+ free_buf(&buf);
+
+ for (int i = 0; i < 4; ++i)
+ {
+ mbuf_add_item(ms, &item);
+ }
+ assert_int_equal(mbuf_len(ms), 4);
+ assert_int_equal(mbuf_buf->refcount, 5);
+
+ mbuf_dereference_instance(ms, &mi);
+
+ assert_int_equal(mbuf_len(ms), 0);
+ assert_false(mbuf_defined(ms));
+ assert_null(mbuf_peek(ms));
+ assert_int_equal(mbuf_buf->refcount, 1);
+
+ /* the queue is empty, so this must be queued and not dropped */
+ mbuf_add_item(ms, &item);
+ assert_int_equal(mbuf_len(ms), 1);
+ assert_ptr_equal(mbuf_peek(ms), &mi);
+
+ mbuf_free(ms);
+ mbuf_free_buf(mbuf_buf);
+}
+
+/* extracting the last live item must not leave a trailing hole behind */
+static void
+test_mbuf_extract_reclaims_tail(void **state)
+{
+ struct mbuf_set *ms = mbuf_init(4);
+ struct multi_instance mi = { 0 };
+ struct multi_instance mi2 = { 0 };
+ struct buffer buf = alloc_buf(16);
+ struct mbuf_buffer *mbuf_buf = mbuf_alloc_buf(&buf);
+ struct mbuf_item item = { .buffer = mbuf_buf, .instance = &mi };
+ struct mbuf_item item2 = { .buffer = mbuf_buf, .instance = &mi2 };
+ free_buf(&buf);
+
+ mbuf_add_item(ms, &item);
+ mbuf_add_item(ms, &item2);
+ mbuf_dereference_instance(ms, &mi2);
+ assert_int_equal(mbuf_len(ms), 2); /* head is still live, nothing to
reclaim */
+
+ struct mbuf_item out;
+ assert_true(mbuf_extract_item(ms, &out));
+ assert_ptr_equal(out.instance, &mi);
+ mbuf_free_buf(out.buffer);
+
+ assert_int_equal(mbuf_len(ms), 0);
+ assert_false(mbuf_defined(ms));
+ assert_null(mbuf_peek(ms));
+
+ mbuf_free(ms);
+ mbuf_free_buf(mbuf_buf);
+}
+
+/* a ring "full" of dereferenced items must not look full to mbuf_add_item() */
+static void
+test_mbuf_add_on_stale_full_queue(void **state)
+{
+ struct mbuf_set *ms = mbuf_init(1);
+ struct multi_instance mi = { 0 };
+ struct multi_instance mi2 = { 0 };
+ struct buffer buf = alloc_buf(16);
+ struct mbuf_buffer *mbuf_buf = mbuf_alloc_buf(&buf);
+ struct mbuf_item item = { .buffer = mbuf_buf, .instance = &mi };
+ struct mbuf_item item2 = { .buffer = mbuf_buf, .instance = &mi2 };
+ free_buf(&buf);
+
+ assert_int_equal(ms->capacity, 1);
+ mbuf_add_item(ms, &item);
+ mbuf_dereference_instance(ms, &mi);
+
+ mbuf_add_item(ms, &item2);
+ assert_int_equal(mbuf_len(ms), 1);
+ assert_ptr_equal(mbuf_peek(ms), &mi2);
+
+ mbuf_free(ms);
+ mbuf_free_buf(mbuf_buf);
+}
+
int
main(void)
{
const struct CMUnitTest tests[] = {
cmocka_unit_test(test_mbuf_init),
cmocka_unit_test(test_mbuf_add_remove),
+ cmocka_unit_test(test_mbuf_dereference_reclaims_queue),
+ cmocka_unit_test(test_mbuf_extract_reclaims_tail),
+ cmocka_unit_test(test_mbuf_add_on_stale_full_queue),
};
return cmocka_run_group_tests_name("mbuf", tests, NULL, NULL);
--
To view, visit http://gerrit.openvpn.net/c/openvpn/+/1961?usp=email
To unsubscribe, or for help writing mail filters, visit
http://gerrit.openvpn.net/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: openvpn
Gerrit-Branch: master
Gerrit-Change-Id: I9579ab9a3f588a3841bb90c9a4b930b7690e0b71
Gerrit-Change-Number: 1961
Gerrit-PatchSet: 1
Gerrit-Owner: its_Giaan <[email protected]>
Gerrit-Reviewer: plaisthos <[email protected]>
Gerrit-CC: openvpn-devel <[email protected]>
Gerrit-Attention: plaisthos <[email protected]>
_______________________________________________
Openvpn-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openvpn-devel
