Skip to content

Commit 67413eb

Browse files
pavel-kirienkopavel-kirienko
committed
simplify
1 parent 8751e8e commit 67413eb

File tree

4 files changed

+50
-70
lines changed

4 files changed

+50
-70
lines changed

‎libudpard/udpard.c‎

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,11 @@ udpard_fragment_t* udpard_fragment_seek(udpard_fragment_t* any_frag, const size_
165165
return NULL;
166166
}
167167

168+
udpard_fragment_t* udpard_fragment_next(udpard_fragment_t* const frag)
169+
{
170+
return (frag != NULL) ? ((udpard_fragment_t*)cavl2_next_greater(&frag->index_offset)) : NULL;
171+
}
172+
168173
size_t udpard_fragment_gather(const udpard_fragment_t* any_frag,
169174
const size_t destination_size_bytes,
170175
void* const destination)
@@ -174,16 +179,15 @@ size_t udpard_fragment_gather(const udpard_fragment_t* any_frag,
174179
while (any_frag->index_offset.up != NULL) { // Locate the root.
175180
any_frag = (const udpard_fragment_t*)any_frag->index_offset.up;
176181
}
177-
for (const udpard_fragment_t* frag =
178-
(const udpard_fragment_t*)cavl2_min((udpard_tree_t*)&any_frag->index_offset);
179-
(frag != NULL) && (offset < destination_size_bytes);
180-
frag = (const udpard_fragment_t*)cavl2_next_greater((udpard_tree_t*)&frag->index_offset)) {
182+
udpard_fragment_t* frag = (udpard_fragment_t*)cavl2_min((udpard_tree_t*)&any_frag->index_offset);
183+
while ((frag != NULL) && (offset < destination_size_bytes)) {
181184
UDPARD_ASSERT(frag->offset == offset);
182185
UDPARD_ASSERT(frag->view.data != NULL);
183186
const size_t to_copy = smaller(frag->view.size, destination_size_bytes - offset);
184187
// NOLINTNEXTLINE(clang-analyzer-security.insecureAPI.DeprecatedOrUnsafeBufferHandling)
185188
(void)memcpy((byte_t*)destination + offset, frag->view.data, to_copy);
186189
offset += to_copy;
190+
frag = udpard_fragment_next(frag);
187191
}
188192
}
189193
return offset;
@@ -779,7 +783,6 @@ static udpard_fragment_t* rx_fragment_new(const udpard_mem_resource_t memory,
779783
if (mew != NULL) {
780784
mem_zero(sizeof(*mew), mew);
781785
mew->index_offset = (udpard_tree_t){ NULL, { NULL, NULL }, 0 };
782-
mew->next = NULL;
783786
mew->view.data = frame.payload.data;
784787
mew->view.size = frame.payload.size;
785788
mew->origin.data = frame.origin.data;
@@ -932,9 +935,8 @@ static rx_fragment_tree_update_result_t rx_fragment_tree_update(udpard_tree_t**
932935
/// because each fragment's offset is changed within the bounds that preserve the ordering.
933936
static bool rx_fragment_tree_finalize(udpard_tree_t* const root, const uint32_t crc_expected)
934937
{
935-
uint32_t crc_computed = CRC_INITIAL;
936-
size_t offset = 0;
937-
udpard_fragment_t* prev = NULL;
938+
uint32_t crc_computed = CRC_INITIAL;
939+
size_t offset = 0;
938940
for (udpard_tree_t* p = cavl2_min(root); p != NULL; p = cavl2_next_greater(p)) {
939941
udpard_fragment_t* const frag = (udpard_fragment_t*)p;
940942
UDPARD_ASSERT(frag->offset <= offset); // The tree reassembler cannot leave gaps.
@@ -946,11 +948,6 @@ static bool rx_fragment_tree_finalize(udpard_tree_t* const root, const uint32_t
946948
frag->view.size -= trim;
947949
offset += frag->view.size;
948950
crc_computed = crc_add(crc_computed, frag->view.size, frag->view.data);
949-
if (prev != NULL) {
950-
prev->next = frag;
951-
}
952-
frag->next = NULL;
953-
prev = frag;
954951
}
955952
return (crc_computed ^ CRC_OUTPUT_XOR) == crc_expected;
956953
}
@@ -1174,8 +1171,7 @@ static void rx_session_on_message(const rx_session_t* const self, udpard_rx_t* c
11741171
.remote = self->remote,
11751172
.payload_size_stored = slot->covered_prefix,
11761173
.payload_size_wire = slot->total_size,
1177-
.payload_head = (udpard_fragment_t*)cavl2_min(slot->fragments),
1178-
.payload_root = (udpard_fragment_t*)slot->fragments,
1174+
.payload = (udpard_fragment_t*)slot->fragments,
11791175
};
11801176
slot->fragments = NULL; // Transfer ownership to the application.
11811177
UDPARD_ASSERT(rx->on_message != NULL);
@@ -1688,8 +1684,7 @@ static void rx_port_accept_stateless(udpard_rx_t* const rx,
16881684
.remote = remote,
16891685
.payload_size_stored = required_size,
16901686
.payload_size_wire = frame.meta.transfer_payload_size,
1691-
.payload_head = frag,
1692-
.payload_root = frag,
1687+
.payload = frag,
16931688
};
16941689
rx->on_message(rx, port, transfer);
16951690
} else {

‎libudpard/udpard.h‎

Lines changed: 18 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -166,36 +166,27 @@ typedef struct udpard_mem_resource_t
166166
udpard_mem_alloc_t alloc;
167167
} udpard_mem_resource_t;
168168

169-
/// This type represents payload as an ordered sequence of its fragments to eliminate data copying.
170-
/// To free a fragmented payload buffer, the application needs to traverse the list and free each fragment's payload
171-
/// as well as the payload structure itself, assuming that it is also heap-allocated.
172-
/// The model is as follows:
173-
///
174-
/// udpard_fragment_t:
175-
/// next ---> udpard_fragment_t...
176-
/// origin ---> (the free()able payload data buffer)
177-
/// view ---> (somewhere inside the payload data buffer)
178-
///
179-
/// Payloads of received transfers are represented using this type, where each fragment corresponds to a frame.
180-
/// The application can either consume them directly or to copy the data into a contiguous buffer beforehand
181-
/// at the expense of extra time and memory utilization.
169+
/// This type represents payload as a binary tree of its fragments ordered by offset to eliminate data copying.
170+
/// The fragments are guaranteed to be non-redundant and non-overlapping; therefore, they are also ordered by their
171+
/// end offsets.
172+
/// See the helper functions below for managing the fragment tree.
182173
typedef struct udpard_fragment_t
183174
{
184-
/// The index_offset BST orders fragments by their offset within the full payload buffer.
175+
/// The index_offset BST orders fragments by their offset (and also end=offset+size) within the transfer payload.
185176
/// It must be the first member.
186-
/// The linked list links all fragments by their offset in ascending order, for convenience.
187-
udpard_tree_t index_offset;
188-
struct udpard_fragment_t* next;
177+
udpard_tree_t index_offset;
189178

190179
/// Contains the actual data to be used by the application.
191-
/// The memory pointed to by this fragment shall not be freed by the application.
180+
/// The memory pointed to by this fragment shall not be freed nor mutated by the application.
192181
udpard_bytes_t view;
193182

194183
/// Points to the base buffer that contains this fragment.
195184
/// The application can use this pointer to free the outer buffer after the payload has been consumed.
185+
/// This memory must not be accessed by the application for any purpose other than freeing it.
196186
udpard_bytes_mut_t origin;
197187

198-
size_t offset; ///< Offset of this fragment's payload within the full payload buffer.
188+
/// Offset of this fragment's payload within the full payload buffer. The ordering key for the index_offset tree.
189+
size_t offset;
199190

200191
/// When the fragment is no longer needed, this deleter shall be used to free the origin buffer.
201192
/// We provide a dedicated deleter per fragment to allow NIC drivers to manage the memory directly,
@@ -222,6 +213,11 @@ void udpard_fragment_free_all(udpard_fragment_t* const frag, const udpard_mem_re
222213
/// has a logarithmic complexity in the number of fragments, which makes it very efficient.
223214
udpard_fragment_t* udpard_fragment_seek(udpard_fragment_t* any_frag, const size_t offset);
224215

216+
/// Given any fragment in a transfer, returns the next fragment in strictly ascending order of offsets.
217+
/// Returns NULL if there is no next fragment or if the given fragment is NULL.
218+
/// The complexity is amortized-constant.
219+
udpard_fragment_t* udpard_fragment_next(udpard_fragment_t* const frag);
220+
225221
/// Given any fragment in a transfer, copies the entire transfer payload into the specified contiguous buffer.
226222
/// If the total size of all fragments combined exceeds the size of the destination buffer,
227223
/// copying will stop early after the buffer is filled, thus truncating the fragmented data short.
@@ -638,14 +634,9 @@ typedef struct udpard_rx_transfer_t
638634
/// the excess payload as it has already been discarded. Cannot be less than payload_size_stored.
639635
size_t payload_size_wire;
640636

641-
/// The payload is stored in a tree, which is also linked-listed for convenience.
642-
/// Hence we have two pointers to the same payload tree: one points to the leftmost tree node aka the 1st fragment;
643-
/// the other points to the root of the tree. The head can be used if sequential access is desired,
644-
/// while the root can be used for logarithmic-time seeking of fragments by their offset within the payload
645-
/// using udpard_fragment_seek().
646-
/// Either can be used with udpard_fragment_free_all() to free the entire payload.
647-
udpard_fragment_t* payload_head;
648-
udpard_fragment_t* payload_root;
637+
/// The payload is stored in a tree of fragments ordered by their offset within the payload.
638+
/// See udpard_fragment_t and its helper functions for managing the fragment tree.
639+
udpard_fragment_t* payload;
649640
} udpard_rx_transfer_t;
650641

651642
/// Emitted when the stack detects the need to send a reception acknowledgment back to the remote node.

‎tests/src/test_intrusive_rx.c‎

Lines changed: 20 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -64,8 +64,8 @@ static bool transfer_payload_verify(udpard_rx_transfer_t* const transfer,
6464
const void* const payload,
6565
const size_t payload_size_wire)
6666
{
67-
const udpard_fragment_t* frag = transfer->payload_head;
68-
size_t offset = 0;
67+
udpard_fragment_t* frag = udpard_fragment_seek(transfer->payload, 0);
68+
size_t offset = 0;
6969
while (frag != NULL) {
7070
if (frag->offset != offset) {
7171
return false;
@@ -77,7 +77,7 @@ static bool transfer_payload_verify(udpard_rx_transfer_t* const transfer,
7777
return false;
7878
}
7979
offset += frag->view.size;
80-
frag = frag->next;
80+
frag = udpard_fragment_next(frag);
8181
}
8282
return (transfer->payload_size_wire == payload_size_wire) && (offset == payload_size_stored);
8383
}
@@ -119,7 +119,8 @@ static bool fragment_tree_verify(udpard_tree_t* const root,
119119
}
120120
// Scan the payload tree.
121121
size_t offset = 0;
122-
for (const udpard_fragment_t* it = (udpard_fragment_t*)cavl2_min(root); it != NULL; it = it->next) {
122+
for (udpard_fragment_t* it = (udpard_fragment_t*)cavl2_min(root); it != NULL;
123+
it = (udpard_fragment_t*)cavl2_next_greater(&it->index_offset)) {
123124
if (it->offset != offset) {
124125
return false;
125126
}
@@ -348,7 +349,7 @@ static void test_rx_fragment_tree_update_a(void)
348349
TEST_ASSERT_EQUAL_size_t(0, alloc_payload.count_free);
349350
// Free the tree (as in freedom).
350351
TEST_ASSERT(fragment_tree_verify(root, 12, "abc\0def\0xyz", 0x2758cbe6UL));
351-
udpard_fragment_free_all((udpard_fragment_t*)root, mem_frag);
352+
udpard_fragment_free_all(udpard_fragment_seek((udpard_fragment_t*)root, 0), mem_frag);
352353
// Check the heap.
353354
TEST_ASSERT_EQUAL_size_t(0, alloc_frag.allocated_fragments);
354355
TEST_ASSERT_EQUAL_size_t(0, alloc_payload.allocated_fragments);
@@ -2159,7 +2160,7 @@ static void test_rx_session_ordered(void)
21592160
TEST_ASSERT_EQUAL_MEMORY("01234", cb_result.ack_mandate.am.payload_head.data, 5);
21602161

21612162
// Free the transfer payload.
2162-
udpard_fragment_free_all(cb_result.message.history[0].payload_head, mem_frag);
2163+
udpard_fragment_free_all(cb_result.message.history[0].payload, mem_frag);
21632164
TEST_ASSERT_EQUAL(0, alloc_frag.allocated_fragments);
21642165
TEST_ASSERT_EQUAL(1, alloc_session.allocated_fragments);
21652166
TEST_ASSERT_EQUAL(0, alloc_payload.allocated_fragments);
@@ -2493,7 +2494,7 @@ static void test_rx_session_ordered(void)
24932494
TEST_ASSERT_EQUAL(1, alloc_session.allocated_fragments);
24942495
TEST_ASSERT_EQUAL(5, alloc_payload.allocated_fragments);
24952496
for (size_t i = 0; i < 4; i++) {
2496-
udpard_fragment_free_all(cb_result.message.history[i].payload_head, mem_frag);
2497+
udpard_fragment_free_all(cb_result.message.history[i].payload, mem_frag);
24972498
}
24982499
TEST_ASSERT_EQUAL(1, alloc_frag.allocated_fragments); // 500 is still there
24992500
TEST_ASSERT_EQUAL(1, alloc_session.allocated_fragments);
@@ -2535,7 +2536,7 @@ static void test_rx_session_ordered(void)
25352536
TEST_ASSERT_EQUAL(udpard_prio_optional, cb_result.message.history[0].priority);
25362537
TEST_ASSERT_EQUAL(500, cb_result.message.history[0].transfer_id);
25372538
TEST_ASSERT(transfer_payload_verify(&cb_result.message.history[0], 10, "9876543210", 10));
2538-
udpard_fragment_free_all(cb_result.message.history[0].payload_head, mem_frag);
2539+
udpard_fragment_free_all(cb_result.message.history[0].payload, mem_frag);
25392540
// All transfers processed, nothing is interned.
25402541
TEST_ASSERT_EQUAL(6, cb_result.message.count);
25412542
TEST_ASSERT_EQUAL(4, cb_result.ack_mandate.count);
@@ -2594,7 +2595,7 @@ static void test_rx_session_ordered(void)
25942595
TEST_ASSERT_EQUAL(udpard_prio_optional, tr->priority);
25952596
TEST_ASSERT_EQUAL(1000 + i, tr->transfer_id);
25962597
TEST_ASSERT(transfer_payload_verify(tr, 2, (char[]){ '0', (char)('0' + i) }, 2));
2597-
udpard_fragment_free_all(tr->payload_head, mem_frag);
2598+
udpard_fragment_free_all(tr->payload, mem_frag);
25982599
}
25992600
TEST_ASSERT_EQUAL(14, cb_result.message.count);
26002601
TEST_ASSERT_EQUAL(4, cb_result.ack_mandate.count);
@@ -2638,12 +2639,12 @@ static void test_rx_session_ordered(void)
26382639
TEST_ASSERT_EQUAL_INT64(ts_3000, cb_result.message.history[1].timestamp);
26392640
TEST_ASSERT_EQUAL(3000, cb_result.message.history[1].transfer_id);
26402641
TEST_ASSERT(transfer_payload_verify(&cb_result.message.history[1], 2, "30", 2));
2641-
udpard_fragment_free_all(cb_result.message.history[1].payload_head, mem_frag);
2642+
udpard_fragment_free_all(cb_result.message.history[1].payload, mem_frag);
26422643
// Now 3001.
26432644
TEST_ASSERT_EQUAL_INT64(ts_3000 + 1, cb_result.message.history[0].timestamp);
26442645
TEST_ASSERT_EQUAL(3001, cb_result.message.history[0].transfer_id);
26452646
TEST_ASSERT(transfer_payload_verify(&cb_result.message.history[0], 2, "31", 2));
2646-
udpard_fragment_free_all(cb_result.message.history[0].payload_head, mem_frag);
2647+
udpard_fragment_free_all(cb_result.message.history[0].payload, mem_frag);
26472648
// We still have 3002..3007 in progress. They will be freed once the session has expired.
26482649
TEST_ASSERT_EQUAL(16, cb_result.message.count);
26492650
TEST_ASSERT_EQUAL(4, cb_result.ack_mandate.count);
@@ -2741,7 +2742,7 @@ static void test_rx_session_unordered(void)
27412742
TEST_ASSERT_EQUAL_MEMORY("hello", cb_result.ack_mandate.am.payload_head.data, 5);
27422743

27432744
// Free the transfer payload.
2744-
udpard_fragment_free_all(cb_result.message.history[0].payload_head, mem_frag);
2745+
udpard_fragment_free_all(cb_result.message.history[0].payload, mem_frag);
27452746
TEST_ASSERT_EQUAL(0, alloc_frag.allocated_fragments);
27462747
TEST_ASSERT_EQUAL(0, alloc_payload.allocated_fragments);
27472748

@@ -2760,7 +2761,7 @@ static void test_rx_session_unordered(void)
27602761
TEST_ASSERT_EQUAL(2, cb_result.message.count);
27612762
TEST_ASSERT_EQUAL(103, cb_result.message.history[0].transfer_id);
27622763
TEST_ASSERT(transfer_payload_verify(&cb_result.message.history[0], 6, "tid103", 6));
2763-
udpard_fragment_free_all(cb_result.message.history[0].payload_head, mem_frag);
2764+
udpard_fragment_free_all(cb_result.message.history[0].payload, mem_frag);
27642765

27652766
meta.transfer_id = 102;
27662767
meta.priority = udpard_prio_nominal;
@@ -2776,7 +2777,7 @@ static void test_rx_session_unordered(void)
27762777
TEST_ASSERT_EQUAL(3, cb_result.message.count);
27772778
TEST_ASSERT_EQUAL(102, cb_result.message.history[0].transfer_id);
27782779
TEST_ASSERT(transfer_payload_verify(&cb_result.message.history[0], 6, "tid102", 6));
2779-
udpard_fragment_free_all(cb_result.message.history[0].payload_head, mem_frag);
2780+
udpard_fragment_free_all(cb_result.message.history[0].payload, mem_frag);
27802781

27812782
// Verify that duplicates are still rejected.
27822783
meta.transfer_id = 103; // repeat of a received transfer
@@ -2834,7 +2835,7 @@ static void test_rx_session_unordered(void)
28342835
TEST_ASSERT_EQUAL(0x0A000002, cb_result.message.history[0].remote.endpoints[1].ip);
28352836
TEST_ASSERT_EQUAL(0x1234, cb_result.message.history[0].remote.endpoints[0].port);
28362837
TEST_ASSERT_EQUAL(0x5678, cb_result.message.history[0].remote.endpoints[1].port);
2837-
udpard_fragment_free_all(cb_result.message.history[0].payload_head, mem_frag);
2838+
udpard_fragment_free_all(cb_result.message.history[0].payload, mem_frag);
28382839

28392840
// ACK mandate generated upon completion.
28402841
TEST_ASSERT_EQUAL(5, cb_result.ack_mandate.count);
@@ -2997,7 +2998,7 @@ static void test_rx_port(void)
29972998
TEST_ASSERT_EQUAL(transfer_id, cb_result.ack_mandate.am.transfer_id);
29982999

29993000
// Clean up.
3000-
udpard_fragment_free_all(cb_result.message.history[0].payload_head, mem_frag);
3001+
udpard_fragment_free_all(cb_result.message.history[0].payload, mem_frag);
30013002
cb_result.message.count = 0;
30023003
cb_result.ack_mandate.count = 0;
30033004
}
@@ -3042,7 +3043,7 @@ static void test_rx_port(void)
30423043
// No ACK for stateless mode without flag_ack.
30433044
TEST_ASSERT_EQUAL(0, cb_result.ack_mandate.count);
30443045

3045-
udpard_fragment_free_all(cb_result.message.history[0].payload_head, mem_frag);
3046+
udpard_fragment_free_all(cb_result.message.history[0].payload, mem_frag);
30463047
cb_result.message.count = 0;
30473048
}
30483049

@@ -3109,7 +3110,7 @@ static void test_rx_port(void)
31093110

31103111
TEST_ASSERT_EQUAL(1, cb_result.ack_mandate.count);
31113112

3112-
udpard_fragment_free_all(cb_result.message.history[0].payload_head, mem_frag);
3113+
udpard_fragment_free_all(cb_result.message.history[0].payload, mem_frag);
31133114
cb_result.message.count = 0;
31143115
cb_result.ack_mandate.count = 0;
31153116
}
@@ -3437,7 +3438,7 @@ static void test_rx_port_timeouts(void)
34373438
// The late arrival should have ejected the earlier completed transfers.
34383439
TEST_ASSERT(cb_result.message.count >= 1);
34393440
for (size_t i = 0; i < cb_result.message.count; i++) {
3440-
udpard_fragment_free_all(cb_result.message.history[i].payload_head, mem_frag);
3441+
udpard_fragment_free_all(cb_result.message.history[i].payload, mem_frag);
34413442
}
34423443
cb_result.message.count = 0;
34433444

‎tests/src/test_public.cpp‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@ udpard_fragment_t* make_test_fragment(const udpard_mem_resource_t& fragment_memo
3232
std::memcpy(payload_data, data, size);
3333
}
3434
std::memset(frag, 0, sizeof(*frag));
35-
frag->next = nullptr;
3635
frag->view.data = payload_data;
3736
frag->view.size = size;
3837
frag->origin.data = payload_data;
@@ -59,7 +58,6 @@ void test_udpard_fragment_seek()
5958
// we can only test simple cases with manually constructed tree structures.
6059
udpard_fragment_t* single = make_test_fragment(mem_frag, mem_payload, del_payload, 0, 5, "hello");
6160
TEST_ASSERT_NOT_NULL(single);
62-
single->next = nullptr;
6361
// Initialize the tree node to null (no parent, no children) - this makes it a standalone root
6462
single->index_offset.up = nullptr;
6563
single->index_offset.lr[0] = nullptr;
@@ -113,11 +111,6 @@ void test_udpard_fragment_seek()
113111
right->index_offset.lr[1] = nullptr;
114112
right->index_offset.bf = 0;
115113

116-
// Link fragments in order using next pointer
117-
left->next = root;
118-
root->next = right;
119-
right->next = nullptr;
120-
121114
// Test seeking from the left child (non-root) - should traverse up to root first.
122115
// Seeking to offset 0 should find the left fragment.
123116
TEST_ASSERT_EQUAL_PTR(left, udpard_fragment_seek(left, 0));

0 commit comments

Comments
 (0)