On 30 Jul 2026, at 19:49, Aaron Conole wrote:
> Refactored TCP module to use a new ct private storage area rather than
> an compatible extended conn struct so that future modules will have
> access to the TCP state details. This will be needed when getting the
> actual tcp state of the connection for offload.
>
> Assisted-by: Claude Sonnet 4.6 <[email protected]>
> Signed-off-by: Aaron Conole <[email protected]>
Thanks Aaron for the v2, some small (nits) below.
//Eelco
[...]
> @@ -439,15 +425,14 @@ static struct conn *
> tcp_new_conn(struct conntrack *ct, struct dp_packet *pkt, long long now,
> uint32_t tp_id)
> {
> - struct conn_tcp* newconn = NULL;
I would keep the definition here (without the NULL
assignment) rather than moving it down. But it is just
personal preference.
> struct tcp_header *tcp = dp_packet_l4(pkt);
> + struct conn_tcp_state *tcp_state;
> struct tcp_peer *src, *dst;
> uint16_t tcp_flags = TCP_FLAGS(tcp->tcp_ctl);
>
> - newconn = xzalloc(sizeof *newconn);
> -
> - src = &newconn->peer[0];
> - dst = &newconn->peer[1];
> + tcp_state = xzalloc(sizeof *tcp_state);
> + src = &tcp_state->peer[0];
> + dst = &tcp_state->peer[1];
>
> src->seqlo = ntohl(get_16aligned_be32(&tcp->tcp_seq));
> src->seqhi = src->seqlo + dp_packet_get_tcp_payload_length(pkt) + 1;
> @@ -473,11 +458,15 @@ tcp_new_conn(struct conntrack *ct, struct dp_packet
> *pkt, long long now,
> src->state = CT_DPIF_TCPS_SYN_SENT;
> dst->state = CT_DPIF_TCPS_CLOSED;
>
> - newconn->up.tp_id = tp_id;
> - conn_init(&newconn->up);
> - conn_init_expiration(ct, &newconn->up, CT_TM_TCP_FIRST_PACKET, now);
> + struct conn *newconn = xzalloc(sizeof *newconn);
> + newconn->tp_id = tp_id;
> + conn_init(newconn);
> + ovs_mutex_lock(&newconn->lock);
> + conn_private_set(newconn, conntrack_l4_private_id, tcp_state);
> + ovs_mutex_unlock(&newconn->lock);
> + conn_init_expiration(ct, newconn, CT_TM_TCP_FIRST_PACKET, now);
>
> - return &newconn->up;
> + return newconn;
> }
>
> static uint8_t
> @@ -499,8 +488,12 @@ tcp_peer_to_protoinfo_flags(const struct tcp_peer *peer)
> static void
> tcp_conn_get_protoinfo(const struct conn *conn_,
> struct ct_dpif_protoinfo *protoinfo)
> + OVS_REQUIRES(conn_->lock)
> {
> - const struct conn_tcp *conn = conn_tcp_cast(conn_);
> + const struct conn_tcp_state *conn = conn_tcp_state_get(conn_);
Blank line between definitions and code? Also in other
places.
> + if (!conn) {
> + return;
> + }
>
> protoinfo->proto = IPPROTO_TCP;
> protoinfo->tcp.state_orig = conn->peer[0].state;
> @@ -519,3 +512,11 @@ struct ct_l4_proto ct_proto_tcp = {
> .conn_update = tcp_conn_update,
> .conn_get_protoinfo = tcp_conn_get_protoinfo,
> };
> +
> +
Only a single blank line is needed.
> +void
> +conntrack_tcp_init(void)
> +{
> + conntrack_l4_private_id = conn_private_id_alloc(free);
> + ovs_assert(conntrack_l4_private_id != CT_PRIVATE_ID_INVALID);
I feel like this init should be done in the main conntrack
framework as it is a global L4 id. Are we sure this shared
ID will not cause any collisions with encapsulated packets?
> +}
> diff --git a/lib/conntrack-tcp.h b/lib/conntrack-tcp.h
> new file mode 100644
> index 0000000000..a80b762a09
> --- /dev/null
> +++ b/lib/conntrack-tcp.h
> @@ -0,0 +1,59 @@
> +/*
> + * Copyright (c) 2026 Red Hat, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef CONNTRACK_TCP_H
> +#define CONNTRACK_TCP_H
> +
> +#include "conntrack-private.h"
> +#include "ct-dpif.h"
> +
> +/* wscale field flags stored in tcp_peer.wscale. */
> +#define CT_WSCALE_FLAG 0x80 /* Negotiated window scaling is in use. */
> +#define CT_WSCALE_UNKNOWN 0x40 /* Scale factor not yet known. */
> +#define CT_WSCALE_MASK 0x0f /* Actual scale factor (0-14). */
> +
> +/* Per-direction TCP state tracked by the conntrack TCP module. */
> +struct tcp_peer {
> + uint32_t seqlo; /* Max sequence number sent. */
> + uint32_t seqhi; /* Max the other end ACKed + win. */
Guess we might need to add 'acked' to the dictionary.
[...]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
