Feature remove no conntrack#777
Conversation
github-actions
Bot
added
documentation
Signed-off-by: Guvenc Gulce <guevenc.guelce@external.t-systems.com>
BOOTP (DHCP) packets were taking an unnecessarily long path through the graph: conntrack → dnat → ipv4_lookup → dhcp. Since BOOTP traffic does not require NAT or route lookup processing, route it directly from the conntrack node to the dhcp node, and remove the redundant BOOTP check in ipv4_lookup_node that forwarded to dhcp from there. Signed-off-by: Guvenc Gulce <guevenc.guelce@external.t-systems.com>
Connection tracking is now always enabled. The --no-conntrack parameter was originally provided to allow running without flow tracking, but it is no longer needed and carrying it adds unnecessary conditional branches throughout the packet-processing fast path. Signed-off-by: Guvenc Gulce <guevenc.guelce@external.t-systems.com>
PlagueCZ
force-pushed
the
feature-remove-no-conntrack
branch
from
351870b to
0a31eb2
Compare
PlagueCZ
left a comment
PlagueCZ
left a comment
There was a problem hiding this comment.
As I was first surprised by this change, I tried to explain how I understand the reason behing it.
|
|
||
| if (!cntrack) | ||
| goto out; | ||
|
|
There was a problem hiding this comment.
This is were BOOTP traffic would jump to out because it has no conntrack.
| if (DP_FAILED(dp_extract_l4_header(df, ipv4_hdr + 1))) | ||
| return CONNTRACK_NEXT_DROP; | ||
| if (df->l4_type == IPPROTO_UDP && df->l4_info.trans_port.dst_port == htons(DP_BOOTP_SRV_PORT)) | ||
| return CONNTRACK_NEXT_DNAT; |
There was a problem hiding this comment.
This is where BOOTP was jumping to DNAT. As you can see this is before call to dp_cntrack_handle(), therefore this flow would be missing conntrack.
|
|
||
| out: | ||
| if (df->l3_type == RTE_ETHER_TYPE_IPV4) | ||
| return DNAT_NEXT_IPV4_LOOKUP; |
There was a problem hiding this comment.
And here all BOOTP would go to IPV4_LOOKUP.
| // TODO: add broadcast routes when machine is added | ||
| if (df->l4_type == IPPROTO_UDP && df->l4_info.trans_port.dst_port == htons(DP_BOOTP_SRV_PORT)) | ||
| return IPV4_LOOKUP_NEXT_DHCP; | ||
|
|
There was a problem hiding this comment.
All BOOTP will be now redirected to DHCP.
| return CONNTRACK_NEXT_DROP; | ||
| if (df->l4_type == IPPROTO_UDP && df->l4_info.trans_port.dst_port == htons(DP_BOOTP_SRV_PORT)) | ||
| return CONNTRACK_NEXT_DNAT; | ||
| return CONNTRACK_NEXT_DHCP; |
There was a problem hiding this comment.
Contrast with the original code - simply jump directly to DHCP.
| struct dp_flow *df = dp_get_flow_ptr(m); | ||
| struct flow_value *cntrack = df->conntrack; | ||
|
|
||
| if (!cntrack) |
There was a problem hiding this comment.
the reason why it is ok not to check if cntrack is null will be traced back to the 'routing' logic inside conntrack_node.c, which is a few nodes away from the current one.
maybe still worthy of using a simple assertion like assert(cntrack) here? (same for the other two places)
There was a problem hiding this comment.
That is actually a good idea, I put it into all five places I found. Please check.
There was a problem hiding this comment.
thanks guys. looks good.
3 participants
This PR simply removes the option as it has no use anymore.
Fixes #416