Conversation
| struct filter_port_range *port_ranges; | ||
| uint32_t port_ranges_count; | ||
|
|
||
| char tag[balancer_vs_acl_max_tag_len + 1]; |
There was a problem hiding this comment.
I do not understand the tag meaning
There was a problem hiding this comment.
If it is about counter name length - use counters please - any counters should be accessible through standard counters API
| } | ||
|
|
||
| vs_stats->incoming_packets += 1; | ||
| vs_stats->incoming_bytes += group_pkts[i]->mbuf->pkt_len; |
There was a problem hiding this comment.
DPDK internals intrusion
|
|
||
| for (size_t pkt_idx = 0; pkt_idx < pkt_ctx_count; ++pkt_idx) { | ||
| if (pkt_idx + prefetch_distance < pkt_ctx_count) { | ||
| rte_prefetch0(rte_pktmbuf_mtod(pkt_ctxs[pkt_idx + prefetch_distance].packet->mbuf, void *)); |
There was a problem hiding this comment.
Justification of prefetch and prefetch value is required at least in form of a commentary
|
|
||
| pkt_ctxs[passed++] = (struct l4_packet_context){ | ||
| .packet = group_pkts[i], | ||
| .matched_vs = vs, |
There was a problem hiding this comment.
The only use of structure I found is obtaining stable_idx - why not to store this value?
| * deterministically. extract_network_metadata overwrites | ||
| * the relevant prefix (4 bytes for IPv4, 16 for IPv6). | ||
| */ | ||
| __builtin_memset( |
There was a problem hiding this comment.
Please use some wrapper or DPDK API function
| * deterministically. extract_network_metadata overwrites | ||
| * the relevant prefix (4 bytes for IPv4, 16 for IPv6). | ||
| */ | ||
| __builtin_memset( |
There was a problem hiding this comment.
Implicit dependency on the structure internal layout so I would suggest to zero-initialize the whole structure - following is a subject of a compiler to optimize
| struct rte_ipv4_hdr *, | ||
| pkt_ctx->packet->network_header.offset | ||
| ); | ||
| __builtin_memcpy( |
| uint8_t flags = hdr->tcp_flags; | ||
|
|
||
| pkt_ctx->session_id.client_port = hdr->src_port; | ||
| pkt_ctx->session_timeout = tcp_session_timeout(timeouts, flags); |
There was a problem hiding this comment.
Code logic breakage - extract transport metadata should not update timeout and reschedule items.
|
|
||
| pkt_ctx->session_id.client_port = hdr->src_port; | ||
| pkt_ctx->session_timeout = timeouts->udp; | ||
| pkt_ctx->can_reschedule = true; |
There was a problem hiding this comment.
In the current implementation UDP flag is an attribute of a service - you could have UDP client packet only if the service is UDP too
| struct l4_packet_context *pkt_ctx, | ||
| struct balancer_session_timeouts *timeouts | ||
| ) { | ||
| if (pkt_ctx->packet->transport_header.type == IPPROTO_TCP) { |
There was a problem hiding this comment.
This should be resolved directly from service attributes.
| ADDR_OF(&context->packet_handler->session_table); | ||
|
|
||
| rcu_t *reals_selector_guard = &context->packet_handler->rcu; | ||
| rcu_read_begin(reals_selector_guard, context->worker_idx); |
There was a problem hiding this comment.
I don't like the idea of replacing any configuration data under the feet of data processing workflows.
There was a problem hiding this comment.
The point is to form a new reals ring and then atomically swap the pointers. An then the old one could be freed after configuration generation change
| struct balancer_session_table *session_table, | ||
| uint64_t current_table_gen | ||
| ) { | ||
| const size_t prefetch_distance = 4; |
There was a problem hiding this comment.
Justification is required
There was a problem hiding this comment.
The overall comment - there are many items encapsulated inside packet context what complicates internal dependency tracking - what and where is used or not. I would prefer to pass all such data explicitly.
There was a problem hiding this comment.
The file name is confusing - it is not easy to understand what is resolved here - vs, rs, session?
| pkt_ctx->packet->hash | ||
| ); | ||
| if (unlikely(real_idx == INVALID_REAL_IDX)) { | ||
| pkt_ctx->matched_vs_stats->no_reals += 1; |
There was a problem hiding this comment.
I think the counter should be increased at upper level - when real lookup fails - without any dependency on a lookup algorithm used
| * So, we dont use ternary operator here. | ||
| */ | ||
| uint64_t idx; | ||
| if (selector->use_rr) { |
There was a problem hiding this comment.
Confusing logic - in the one hand the routine is called from the _ops scheduler but hash is passed - in the other hand there is is_rr flag used.
| uint32_t real_idx = selector_select( | ||
| ADDR_OF(&vs->selector), | ||
| context->worker_idx, | ||
| pkt_ctx->packet->hash |
There was a problem hiding this comment.
I assume you should use increasing sequence instead of hash to use OPS.
| */ | ||
| uint64_t idx; | ||
| if (selector->use_rr) { | ||
| idx = selector->workers[worker].value++; |
There was a problem hiding this comment.
What about size of RCU_WORKERS and worker_idx - we could have a huge amount of them
| * sessions. Non-initiating packets (TCP non-SYN) are dropped | ||
| * and the session slot is freed. | ||
| */ | ||
| if (unlikely(!pkt_ctx->can_reschedule)) { |
There was a problem hiding this comment.
Why do you want to pessimise UDP services
| */ | ||
| if (unlikely(!pkt_ctx->can_reschedule)) { | ||
| vs_stats->not_rescheduled_packets += 1; | ||
| st_remove_session(session_state); |
There was a problem hiding this comment.
Why the session should be removed? Packet retransmission will change the logic.
| } | ||
|
|
||
| if (unlikely(!real_is_enabled(real))) { | ||
| struct balancer_real_stats *real_stats = real_get_stats( |
There was a problem hiding this comment.
Some services allow to reuse disabled reals for established session
| * sessions. Non-initiating packets (TCP non-SYN) are dropped | ||
| * and the session slot is freed. | ||
| */ | ||
| if (unlikely(!pkt_ctx->can_reschedule)) { |
There was a problem hiding this comment.
What if a service prohibits real reschedule but has no session?
There was a problem hiding this comment.
I understand that SYN packet enables reschedule somewhere in the code but the logic here looks confusing
2 participants
No description provided.