[ovs-dev] [PATCHv3] userspace: Add conntrack timeout policy support.
William Tu
u9012063 at gmail.com
Sun Apr 26 23:37:23 UTC 2020
Hi Yi-Hung,
Thanks for your review.
On Wed, Apr 22, 2020 at 03:52:20PM -0700, Yi-Hung Wei wrote:
> On Sun, Apr 19, 2020 at 9:05 AM William Tu <u9012063 at gmail.com> wrote:
> >
> > Commit 1f1613183733 ("ct-dpif, dpif-netlink: Add conntrack timeout
> > policy support") adds conntrack timeout policy for kernel datapath.
> > This patch enables support for the userspace datapath. I tested
> > using the 'make check-system-userspace' which checks the timeout
> > policies for ICMP and UDP cases.
> >
> > Signed-off-by: William Tu <u9012063 at gmail.com>
> > ---
> > v3:
> > - address feedback from Yi-Hung
> > - use ID 0 as default policy
> > - move conn_{init,update}_expiration to lib/conntrack-tp.c
> > - s/tpid/tp_id/
> > - add default timeout value to CT_DPIF_TP_*_ATTRs
> > - reduce the CT_CLEAN_INTERVAL from 5 to 3s, to make the tests
> > run faster.
> > - add more tests to system-traffic.at
> > - code refactoring and renaming
> > - travis: https://travis-ci.org/github/williamtu/ovs-travis/builds/676683451
> > ---
>
> Thanks for the updated patch.
>
> > diff --git a/lib/conntrack-icmp.c b/lib/conntrack-icmp.c
> > index 63246f0124d0..5c6ef37d2eb4 100644
> > --- a/lib/conntrack-icmp.c
> > +++ b/lib/conntrack-icmp.c
> > @@ -22,6 +22,7 @@
> > #include <netinet/icmp6.h>
> >
> > #include "conntrack-private.h"
> > +#include "conntrack-tp.h"
> > #include "dp-packet.h"
> >
> > enum OVS_PACKED_ENUM icmp_state {
> > @@ -50,9 +51,12 @@ icmp_conn_update(struct conntrack *ct, struct conn *conn_,
> > struct dp_packet *pkt OVS_UNUSED, bool reply, long long now)
> > {
> > struct conn_icmp *conn = conn_icmp_cast(conn_);
> > - conn->state = reply ? ICMPS_REPLY : ICMPS_FIRST;
> > - conn_update_expiration(ct, &conn->up, icmp_timeouts[conn->state], now);
> >
> > + if (reply && conn->state == ICMPS_FIRST) {
> > + conn->state = ICMPS_REPLY;
> > + }
>
> This code snippet looks like a reasonable fix on the ICMP state
> handling, should we pull it out as a separate patch?
>
> Previously, we will change the ICMP state from ICMP_REPLY back to
> ICMP_FIRST for every echo request packet after an echo reply packet.
> With this patch, the ICMP connection will stay in ICMP_REPLY state
> after the first echo reply packet.
>
yes, it's a bug in icmp conntrack, I will send separate patch.
>
>
> > diff --git a/lib/conntrack-tp.c b/lib/conntrack-tp.c
> > new file mode 100644
> > index 000000000000..5e59f56eb949
> > --- /dev/null
> > +++ b/lib/conntrack-tp.c
> > @@ -0,0 +1,319 @@
> > +#include <config.h>
> > +
> > +#include "conntrack-private.h"
> > +#include "conntrack-tp.h"
> > +#include "ct-dpif.h"
> > +#include "dp-packet.h"
>
> Is "dp-packet.h" required?
>
ok
> > +#include "openvswitch/vlog.h"
>
> May be put this line with the other #include?
ok
>
>
> > +VLOG_DEFINE_THIS_MODULE(conntrack_tp);
> > +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > +
> > +#define DEFAULT_TP_ID 0
>
> DEFAULT_TP_ID is also defined in ct-dpif.h, we should keep it in one place.
OK, I will keep it in ct-dpif.h.
>
>
> > +static const char *ct_timeout_str[] = {
> > +#define CT_TIMEOUT(NAME, VALUE) #NAME,
> > + CT_TIMEOUTS
> > +#undef CT_TIMEOUT
> > +};
> > +
> > +static const char *dpif_ct_timeout_str[] = {
> > +#define xstr(s) #s
> > +#define CT_DPIF_TP_TCP_ATTR(NAME, VAL) xstr(TCP_##NAME),
> > + CT_DPIF_TP_TCP_ATTRS
> > +#undef CT_DPIF_TP_TCP_ATTR
> > +#define CT_DPIF_TP_UDP_ATTR(NAME, VAL) xstr(UDP_##NAME),
> > + CT_DPIF_TP_UDP_ATTRS
> > +#undef CT_DPIF_TP_UDP_ATTR
> > +#define CT_DPIF_TP_ICMP_ATTR(NAME, VAL) xstr(ICMP_##NAME),
> > + CT_DPIF_TP_ICMP_ATTRS
> > +#undef CT_DPIF_TP_ICMP_ATTR
> > +#undef xstr
> > +};
> > +
> > +static unsigned int ct_dpif_timeout_value_def[] = {
> > +#define CT_DPIF_TP_TCP_ATTR(NAME, VAL) [CT_DPIF_TP_ATTR_TCP_##NAME] = VAL,
> > + CT_DPIF_TP_TCP_ATTRS
> > +#undef CT_DPIF_TP_TCP_ATTR
> > +#define CT_DPIF_TP_UDP_ATTR(NAME, VAL) [CT_DPIF_TP_ATTR_UDP_##NAME] = VAL,
> > + CT_DPIF_TP_UDP_ATTRS
> > +#undef CT_DPIF_TP_UDP_ATTR
> > +#define CT_DPIF_TP_ICMP_ATTR(NAME, VAL) [CT_DPIF_TP_ATTR_ICMP_##NAME] = VAL,
> > + CT_DPIF_TP_ICMP_ATTRS
> > +#undef CT_DPIF_TP_ICMP_ATTR
> > +};
>
> We now have two places that define the default timeout of the L4
> protocols, one in ct-dpif.h (CT_DPIF_TP_TCP_ATTRS,
> CT_DPIF_TP_UDP_ATTRS, CT_DPIF_ICMP_ATTRS), and one in
> conntrack-private.h (CT_TIMEOUTS).
>
> Since ct-dpif.h is a common conntrack dpif interface for both the
> kernel and the userspace datapath, and the default value is different
> in this two datapaths, should we put the userspace default timeout
> setting in lib/conntrack-tp.h, and clean up CT_TIMEOUTS in
> lib/conntrack-private.h ?
>
> Maybe rename ct_dpif_timeout_value_def to ct_dpif_netdev* ?
Sure, will do it in next version.
>
>
> > +
> > +static void OVS_UNUSED
> > +timeout_policy_dump(struct timeout_policy *tp)
> > +{
> > + bool present;
> > + int i;
> > +
> > + VLOG_DBG("Timeout Policy ID %u:", tp->policy.id);
> > + for (i = 0; i < ARRAY_SIZE(tp->policy.attrs); i++) {
> > + if (tp->policy.present & (1 << i)) {
> > + present = !!(tp->policy.present & (1 << i));
> > + VLOG_DBG(" Policy: %s present: %u value: %u",
> > + dpif_ct_timeout_str[i], present, tp->policy.attrs[i]);
> > + }
> > + }
> > +}
>
> Is this static function needed? It does not seem to be called within
> conntrack-tp.c. If it is not needed, we should remove
> dpif_ct_timeout_str as well.
This is used for debugging, let me think about keeping it or not.
>
>
> > +
> > +static struct timeout_policy *
> > +timeout_policy_lookup(struct conntrack *ct, int32_t tp_id)
> > + OVS_REQUIRES(ct->ct_lock)
> > +{
> > + struct timeout_policy *tp;
> > + uint32_t hash;
> > +
> > + hash = hash_int(tp_id, ct->hash_basis);
> > + HMAP_FOR_EACH_IN_BUCKET (tp, node, hash, &ct->timeout_policies) {
> > + if (tp->policy.id == tp_id) {
> > + return tp;
> > + }
> > + }
> > + return NULL;
> > +}
> > +
> > +struct timeout_policy *
> > +timeout_policy_get(struct conntrack *ct, int32_t tp_id)
> > +{
> > + struct timeout_policy *tp;
> > +
> > + ovs_mutex_lock(&ct->ct_lock);
> > + tp = timeout_policy_lookup(ct, tp_id);
> > + if (!tp) {
> > + ovs_mutex_unlock(&ct->ct_lock);
> > + return NULL;
> > + }
> > +
> > + ovs_mutex_unlock(&ct->ct_lock);
> > + return tp;
> > +}
> > +
> > +static void
> > +update_existing_tp(struct timeout_policy *tp_dst,
> > + struct timeout_policy *tp_src)
> > +{
> > + struct ct_dpif_timeout_policy *dst, *src;
> > + int i;
> > +
> > + dst = &tp_dst->policy;
> > + src = &tp_src->policy;
> > +
> > + /* Set the value and present bit to dst if present
> > + * bit in src is set.
> > + */
> > + for (i = 0; i < ARRAY_SIZE(dst->attrs); i++) {
> > + if (src->present & (1 << i)) {
> > + dst->attrs[i] = src->attrs[i];
> > + dst->present |= (1 << i);
> > + }
> > + }
> > +}
>
> This logic does not unset some timeout value if it is not present in
> tp_src. May be it is easier to update the timeout policy as we create
> a new one (restore every timeout values to default, and updates the
> present ones in tp_src).
>
OK will do it.
>
> > +
> > +static void
> > +init_default_tp(struct timeout_policy *tp, uint32_t tp_id)
> > +{
> > + tp->policy.id = tp_id;
> > + /* Initialize the timeout value to default, but not
> > + * setting the present bit.
> > + */
> > + tp->policy.present = 0;
> > + memcpy(tp->policy.attrs, ct_dpif_timeout_value_def,
> > + sizeof tp->policy.attrs);
> > +}
> > +
> > +static void
> > +timeout_policy_create(struct conntrack *ct,
> > + struct timeout_policy *new_tp)
> > + OVS_REQUIRES(ct->ct_lock)
> > +{
> > + uint32_t tp_id = new_tp->policy.id;
> > + struct timeout_policy *tp;
> > + uint32_t hash;
> > +
> > + tp = xzalloc(sizeof *tp);
> > + init_default_tp(tp, tp_id);
> > + update_existing_tp(tp, new_tp);
> > + hash = hash_int(tp_id, ct->hash_basis);
> > + hmap_insert(&ct->timeout_policies, &tp->node, hash);
> > +}
> > +
> > +int
> > +timeout_policy_update(struct conntrack *ct,
> > + struct timeout_policy *new_tp)
> > +{
> > + int err = 0;
> > + uint32_t tp_id = new_tp->policy.id;
> > +
> > + ovs_mutex_lock(&ct->ct_lock);
> > + struct timeout_policy *tp = timeout_policy_lookup(ct, tp_id);
> > + if (tp) {
> > + update_existing_tp(tp, new_tp);
> > + } else {
> > + timeout_policy_create(ct, new_tp);
> > + }
> > + ovs_mutex_unlock(&ct->ct_lock);
> > + return err;
> > +}
> > +
> > +void
> > +timeout_policy_clean(struct conntrack *ct, struct timeout_policy *tp)
> > + OVS_REQUIRES(ct->ct_lock)
> > +{
> > + hmap_remove(&ct->timeout_policies, &tp->node);
> > + free(tp);
> > +}
>
> Maybe make timeout_policy_clean() to be static ?
OK
>
>
> > +int
> > +timeout_policy_delete(struct conntrack *ct, uint32_t tp_id)
> > +{
> > + ovs_mutex_lock(&ct->ct_lock);
> > + struct timeout_policy *tp = timeout_policy_lookup(ct, tp_id);
> > + if (tp) {
> > + timeout_policy_clean(ct, tp);
> > + } else {
> > + VLOG_WARN_RL(&rl, "Attempted delete of non-existent timeout "
> > + "policy: zone %d", tp_id);
>
> I would sugguest the following minor change on the warning log
> + VLOG_WARN_RL(&rl, "Failed to delete a non-existent timeout "
> + "policy: id=%d", tp_id);
OK
>
> > + }
> > + ovs_mutex_unlock(&ct->ct_lock);
> > + return 0;
> > +}
> > +
> > +void
> > +timeout_policy_init(struct conntrack *ct)
> > + OVS_REQUIRES(ct->ct_lock)
> > +{
> > + struct timeout_policy tp;
> > +
> > + hmap_init(&ct->timeout_policies);
> > +
> > + /* Create default timeout policy. */
> > + memset(&tp, 0, sizeof tp);
> > + tp.policy.id = DEFAULT_TP_ID;
> > + timeout_policy_create(ct, &tp);
> > +}
> > +
> > +static enum ct_dpif_tp_attr
> > +tm_to_ct_dpif_tp(enum ct_timeout tm)
> > +{
> > + switch (tm) {
> > + case CT_TM_TCP_FIRST_PACKET:
> > + return CT_DPIF_TP_ATTR_TCP_SYN_SENT;
> > + case CT_TM_TCP_OPENING:
> > + return CT_DPIF_TP_ATTR_TCP_SYN_RECV;
> > + case CT_TM_TCP_ESTABLISHED:
> > + return CT_DPIF_TP_ATTR_TCP_ESTABLISHED;
> > + case CT_TM_TCP_CLOSING:
> > + return CT_DPIF_TP_ATTR_TCP_FIN_WAIT;
> > + case CT_TM_TCP_FIN_WAIT:
> > + return CT_DPIF_TP_ATTR_TCP_TIME_WAIT;
> > + case CT_TM_TCP_CLOSED:
> > + return CT_DPIF_TP_ATTR_TCP_CLOSE;
> > + case CT_TM_OTHER_FIRST:
> > + return CT_DPIF_TP_ATTR_UDP_FIRST;
> > + case CT_TM_OTHER_BIDIR:
> > + return CT_DPIF_TP_ATTR_UDP_SINGLE;
> > + case CT_TM_OTHER_MULTIPLE:
> > + return CT_DPIF_TP_ATTR_UDP_MULTIPLE;
> > + case CT_TM_ICMP_FIRST:
> > + return CT_DPIF_TP_ATTR_ICMP_FIRST;
> > + case CT_TM_ICMP_REPLY:
> > + return CT_DPIF_TP_ATTR_ICMP_REPLY;
> > + case N_CT_TM:
> > + default:
> > + OVS_NOT_REACHED();
> > + break;
> > + }
> > + OVS_NOT_REACHED();
> > + return CT_DPIF_TP_ATTR_MAX;
> > +}
> > +
> > +static void
> > +conn_update_expiration__(struct conntrack *ct, struct conn *conn,
> > + enum ct_timeout tm, long long now,
> > + uint32_t tp_value)
> > + OVS_NO_THREAD_SAFETY_ANALYSIS
> > +{
> > + ovs_mutex_unlock(&conn->lock);
> > +
> > + ovs_mutex_lock(&ct->ct_lock);
> > + ovs_mutex_lock(&conn->lock);
> > + if (!conn->cleaned) {
> > + conn->expiration = now + tp_value * 1000;
> > + ovs_list_remove(&conn->exp_node);
> > + ovs_list_push_back(&ct->exp_lists[tm], &conn->exp_node);
> > + }
> > + ovs_mutex_unlock(&conn->lock);
> > + ovs_mutex_unlock(&ct->ct_lock);
> > +
> > + ovs_mutex_lock(&conn->lock);
> > +}
> > +
> > +/* The conn entry lock must be held on entry and exit. */
> > +void
> > +conn_update_expiration(struct conntrack *ct, struct conn *conn,
> > + enum ct_timeout tm, long long now)
> > + OVS_REQUIRES(ct->ct_lock)
>
> Does ct->ct_lock require here? We lock ct->ct_lock in conn_update_expiration__
Good catch.
>
>
> > +{
> > + struct timeout_policy *tp;
> > + uint32_t val;
> > +
> > + tp = timeout_policy_lookup(ct, conn->tp_id);
> > + if (tp) {
> > + val = tp->policy.attrs[tm_to_ct_dpif_tp(tm)];
> > + } else {
> > + val = ct_dpif_timeout_value_def[tm_to_ct_dpif_tp(tm)];
> > + }
> > + VLOG_DBG_RL(&rl, "Update timeout %s with policy id=%d val=%u sec.",
> > + ct_timeout_str[tm], conn->tp_id, val);
>
> Should we include the zone id here and in the conn_init_expiration()?
OK
>
> > +
> > + conn_update_expiration__(ct, conn, tm, now, val);
> > +}
> > +
> > +static void
> > +conn_init_expiration__(struct conntrack *ct, struct conn *conn,
> > + enum ct_timeout tm, long long now,
> > + uint32_t tp_value)
> > +{
> > + conn->expiration = now + tp_value * 1000;
> > + ovs_list_push_back(&ct->exp_lists[tm], &conn->exp_node);
> > +}
> > +
> > +/* ct_lock must be held. */
> > +void
> > +conn_init_expiration(struct conntrack *ct, struct conn *conn,
> > + enum ct_timeout tm, long long now)
> > + OVS_REQUIRES(ct->ct_lock)
> > +{
> > + struct timeout_policy *tp;
> > + uint32_t val;
> > +
> > + tp = timeout_policy_lookup(ct, conn->tp_id);
> > + if (tp) {
> > + val = tp->policy.attrs[tm_to_ct_dpif_tp(tm)];
> > + } else {
> > + val = ct_dpif_timeout_value_def[tm_to_ct_dpif_tp(tm)];
> > + }
> > + VLOG_DBG_RL(&rl, "Init timeout %s with policy id=%d val=%u sec.",
> > + ct_timeout_str[tm], conn->tp_id, val);
> > +
> > + conn_init_expiration__(ct, conn, tm, now, val);
> > +}
>
>
> > --- a/lib/conntrack.c
> > +++ b/lib/conntrack.c
> > @@ -25,6 +25,7 @@
> > #include "bitmap.h"
> > #include "conntrack.h"
> > #include "conntrack-private.h"
> > +#include "conntrack-tp.h"
> > #include "coverage.h"
> > #include "csum.h"
> > #include "ct-dpif.h"
> > @@ -88,7 +89,8 @@ static uint32_t conn_key_hash(const struct conn_key *, uint32_t basis);
> > static void conn_key_reverse(struct conn_key *);
> > static bool valid_new(struct dp_packet *pkt, struct conn_key *);
> > static struct conn *new_conn(struct conntrack *ct, struct dp_packet *pkt,
> > - struct conn_key *, long long now);
> > + struct conn_key *, long long now,
> > + uint32_t tp_id);
> > static void delete_conn_cmn(struct conn *);
> > static void delete_conn(struct conn *);
> > static void delete_conn_one(struct conn *conn);
> > @@ -175,12 +177,6 @@ static alg_helper alg_helpers[] = {
> > [CT_ALG_CTL_TFTP] = handle_tftp_ctl,
> > };
> >
> > -long long ct_timeout_val[] = {
> > -#define CT_TIMEOUT(NAME, VAL) [CT_TM_##NAME] = VAL,
> > - CT_TIMEOUTS
> > -#undef CT_TIMEOUT
> > -};
> > -
> > /* The maximum TCP or UDP port number. */
> > #define CT_MAX_L4_PORT 65535
> > /* String buffer used for parsing FTP string messages.
> > @@ -312,6 +308,7 @@ conntrack_init(void)
> > }
> > hmap_init(&ct->zone_limits);
> > ct->zone_limit_seq = 0;
> > + timeout_policy_init(ct);
> > ovs_mutex_unlock(&ct->ct_lock);
> >
> > ct->hash_basis = random_uint32();
> > @@ -502,6 +499,12 @@ conntrack_destroy(struct conntrack *ct)
> > }
> > hmap_destroy(&ct->zone_limits);
> >
> > + struct timeout_policy *tp;
> > + HMAP_FOR_EACH_POP (tp, node, &ct->timeout_policies) {
> > + free(tp);
> > + }
> > + hmap_destroy(&ct->timeout_policies);
> > +
> > ovs_mutex_unlock(&ct->ct_lock);
> > ovs_mutex_destroy(&ct->ct_lock);
> >
> > @@ -956,7 +959,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
> > struct conn_lookup_ctx *ctx, bool commit, long long now,
> > const struct nat_action_info_t *nat_action_info,
> > const char *helper, const struct alg_exp_node *alg_exp,
> > - enum ct_alg_ctl_type ct_alg_ctl)
> > + enum ct_alg_ctl_type ct_alg_ctl, uint32_t tp_id)
> > OVS_REQUIRES(ct->ct_lock)
> > {
> > struct conn *nc = NULL;
> > @@ -987,7 +990,7 @@ conn_not_found(struct conntrack *ct, struct dp_packet *pkt,
> > return nc;
> > }
> >
> > - nc = new_conn(ct, pkt, &ctx->key, now);
> > + nc = new_conn(ct, pkt, &ctx->key, now, tp_id);
> > memcpy(&nc->key, &ctx->key, sizeof nc->key);
> > memcpy(&nc->rev_key, &nc->key, sizeof nc->rev_key);
> > conn_key_reverse(&nc->rev_key);
> > @@ -1275,7 +1278,8 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
> > bool force, bool commit, long long now, const uint32_t *setmark,
> > const struct ovs_key_ct_labels *setlabel,
> > const struct nat_action_info_t *nat_action_info,
> > - ovs_be16 tp_src, ovs_be16 tp_dst, const char *helper)
> > + ovs_be16 tp_src, ovs_be16 tp_dst, const char *helper,
> > + uint32_t tp_id)
> > {
> > /* Reset ct_state whenever entering a new zone. */
> > if (pkt->md.ct_state && pkt->md.ct_zone != zone) {
> > @@ -1359,7 +1363,7 @@ process_one(struct conntrack *ct, struct dp_packet *pkt,
> > ovs_mutex_lock(&ct->ct_lock);
> > if (!conn_lookup(ct, &ctx->key, now, NULL, NULL)) {
> > conn = conn_not_found(ct, pkt, ctx, commit, now, nat_action_info,
> > - helper, alg_exp, ct_alg_ctl);
> > + helper, alg_exp, ct_alg_ctl, tp_id);
> > }
> > ovs_mutex_unlock(&ct->ct_lock);
> > }
> > @@ -1395,7 +1399,7 @@ conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
> > const struct ovs_key_ct_labels *setlabel,
> > ovs_be16 tp_src, ovs_be16 tp_dst, const char *helper,
> > const struct nat_action_info_t *nat_action_info,
> > - long long now)
> > + long long now, uint32_t tp_id)
> > {
> > ipf_preprocess_conntrack(ct->ipf, pkt_batch, now, dl_type, zone,
> > ct->hash_basis);
> > @@ -1417,7 +1421,8 @@ conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
> > write_ct_md(packet, zone, NULL, NULL, NULL);
> > } else {
> > process_one(ct, packet, &ctx, zone, force, commit, now, setmark,
> > - setlabel, nat_action_info, tp_src, tp_dst, helper);
> > + setlabel, nat_action_info, tp_src, tp_dst, helper,
> > + tp_id);
> > }
> > }
> >
> > @@ -1523,7 +1528,7 @@ conntrack_clean(struct conntrack *ct, long long now)
> > atomic_read_relaxed(&ct->n_conn_limit, &n_conn_limit);
> > size_t clean_max = n_conn_limit > 10 ? n_conn_limit / 10 : 1;
> > long long min_exp = ct_sweep(ct, now, clean_max);
> > - long long next_wakeup = MIN(min_exp, now + CT_TM_MIN);
> > + long long next_wakeup = MIN(min_exp, now + CT_DPIF_TP_MIN);
> >
> > return next_wakeup;
> > }
> > @@ -1540,14 +1545,14 @@ conntrack_clean(struct conntrack *ct, long long now)
> > * - We want to reduce the number of wakeups and batch connection cleanup
> > * when the load is not very high. CT_CLEAN_INTERVAL ensures that if we
> > * are coping with the current cleanup tasks, then we wait at least
> > - * 5 seconds to do further cleanup.
> > + * 3 seconds to do further cleanup.
> > *
> > * - We don't want to keep the map locked too long, as we might prevent
> > * traffic from flowing. CT_CLEAN_MIN_INTERVAL ensures that if cleanup is
> > * behind, there is at least some 200ms blocks of time when the map will be
> > * left alone, so the datapath can operate unhindered.
> > */
> > -#define CT_CLEAN_INTERVAL 5000 /* 5 seconds */
> > +#define CT_CLEAN_INTERVAL 3000 /* 3 second */
>
> I'm not sure whether we should reduce the CLEAN_INTERVAL from 5
> seconds to 3 seconds. It definitely help to clean up the expired
> conntrack entries more freqently, but it also increase the workload.
>
Maybe keeping it 5 seconds is better. I will remove it.
>
> > #define CT_CLEAN_MIN_INTERVAL 200 /* 0.2 seconds */
> >
> > static void *
> > @@ -2347,9 +2352,9 @@ valid_new(struct dp_packet *pkt, struct conn_key *key)
> >
> > static struct conn *
> > new_conn(struct conntrack *ct, struct dp_packet *pkt, struct conn_key *key,
> > - long long now)
> > + long long now, uint32_t tp_id)
> > {
> > - return l4_protos[key->nw_proto]->new_conn(ct, pkt, now);
> > + return l4_protos[key->nw_proto]->new_conn(ct, pkt, now, tp_id);
> > }
> >
> > static void
>
>
> > diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> > index 3e227d9e3b6e..707adf70402c 100644
> > --- a/lib/ct-dpif.h
> > +++ b/lib/ct-dpif.h
> > @@ -59,6 +59,8 @@ struct ct_dpif_timestamp {
> > uint64_t stop;
> > };
> >
> > +#define DEFAULT_TP_ID 0
> > +
> > #define CT_DPIF_TCP_STATES \
> > CT_DPIF_TCP_STATE(CLOSED) \
> > CT_DPIF_TCP_STATE(LISTEN) \
> > @@ -225,36 +227,53 @@ struct ct_dpif_zone_limit {
> > struct ovs_list node;
> > };
> >
> > +/* TP attr with default timeout in seconds. */
> > #define CT_DPIF_TP_TCP_ATTRS \
> > - CT_DPIF_TP_TCP_ATTR(SYN_SENT) \
> > - CT_DPIF_TP_TCP_ATTR(SYN_RECV) \
> > - CT_DPIF_TP_TCP_ATTR(ESTABLISHED) \
> > - CT_DPIF_TP_TCP_ATTR(FIN_WAIT) \
> > - CT_DPIF_TP_TCP_ATTR(CLOSE_WAIT) \
> > - CT_DPIF_TP_TCP_ATTR(LAST_ACK) \
> > - CT_DPIF_TP_TCP_ATTR(TIME_WAIT) \
> > - CT_DPIF_TP_TCP_ATTR(CLOSE) \
> > - CT_DPIF_TP_TCP_ATTR(SYN_SENT2) \
> > - CT_DPIF_TP_TCP_ATTR(RETRANSMIT) \
> > - CT_DPIF_TP_TCP_ATTR(UNACK)
> > + CT_DPIF_TP_TCP_ATTR(SYN_SENT, 30) \
> > + CT_DPIF_TP_TCP_ATTR(SYN_RECV, 30) \
> > + CT_DPIF_TP_TCP_ATTR(ESTABLISHED, 24 * 60 * 60) \
> > + CT_DPIF_TP_TCP_ATTR(FIN_WAIT, 15 * 60) \
> > + CT_DPIF_TP_TCP_ATTR(CLOSE_WAIT, 45) \
> > + CT_DPIF_TP_TCP_ATTR(LAST_ACK, 30) \
> > + CT_DPIF_TP_TCP_ATTR(TIME_WAIT, 45) \
> > + CT_DPIF_TP_TCP_ATTR(CLOSE, 30) \
> > + CT_DPIF_TP_TCP_ATTR(SYN_SENT2, 30) \
> > + CT_DPIF_TP_TCP_ATTR(RETRANSMIT, 30) \
> > + CT_DPIF_TP_TCP_ATTR(UNACK, 30)
> >
> > #define CT_DPIF_TP_UDP_ATTRS \
> > - CT_DPIF_TP_UDP_ATTR(FIRST) \
> > - CT_DPIF_TP_UDP_ATTR(SINGLE) \
> > - CT_DPIF_TP_UDP_ATTR(MULTIPLE)
> > + CT_DPIF_TP_UDP_ATTR(FIRST, 60) \
> > + CT_DPIF_TP_UDP_ATTR(SINGLE, 60) \
> > + CT_DPIF_TP_UDP_ATTR(MULTIPLE, 30)
> >
> > #define CT_DPIF_TP_ICMP_ATTRS \
> > - CT_DPIF_TP_ICMP_ATTR(FIRST) \
> > - CT_DPIF_TP_ICMP_ATTR(REPLY)
> > + CT_DPIF_TP_ICMP_ATTR(FIRST, 60) \
> > + CT_DPIF_TP_ICMP_ATTR(REPLY, 30)
> > +
> > +/* The minimum value of the default timeout. */
> > +#define CT_DPIF_TP_MIN 30
>
> This value is similar to CT_TM_MIN, and it is for dpif-netdev only,
> should we keep it in lib/conntrack-tp.h?
>
OK, I will remove the default timeout value above.
So this CT_DPIF_TP_MIN will be removed too.
>
> > +#define CT_DPIF_TP_TCP_ATTR(ATTR, VAL) \
> > + BUILD_ASSERT_DECL(VAL >= CT_DPIF_TP_MIN);
> > + CT_DPIF_TP_TCP_ATTRS
> > +#define CT_DPIF_TP_UDP_ATTR(ATTR, VAL) \
> > + BUILD_ASSERT_DECL(VAL >= CT_DPIF_TP_MIN);
> > + CT_DPIF_TP_UDP_ATTRS
> > +#undef CT_DPIF_TP_UDP_ATTR
> > +#define CT_DPIF_TP_ICMP_ATTR(ATTR, VAL) \
> > + BUILD_ASSERT_DECL(VAL >= CT_DPIF_TP_MIN);
> > + CT_DPIF_TP_ICMP_ATTRS
> > +#undef CT_DPIF_TP_ICMP_ATTR
> > +
> > +#undef CT_DPIF_TP_TCP_ATTR
> >
> > enum OVS_PACKED_ENUM ct_dpif_tp_attr {
> > -#define CT_DPIF_TP_TCP_ATTR(ATTR) CT_DPIF_TP_ATTR_TCP_##ATTR,
> > +#define CT_DPIF_TP_TCP_ATTR(ATTR, VAL) CT_DPIF_TP_ATTR_TCP_##ATTR,
> > CT_DPIF_TP_TCP_ATTRS
> > #undef CT_DPIF_TP_TCP_ATTR
> > -#define CT_DPIF_TP_UDP_ATTR(ATTR) CT_DPIF_TP_ATTR_UDP_##ATTR,
> > +#define CT_DPIF_TP_UDP_ATTR(ATTR, VAL) CT_DPIF_TP_ATTR_UDP_##ATTR,
> > CT_DPIF_TP_UDP_ATTRS
> > #undef CT_DPIF_TP_UDP_ATTR
> > -#define CT_DPIF_TP_ICMP_ATTR(ATTR) CT_DPIF_TP_ATTR_ICMP_##ATTR,
> > +#define CT_DPIF_TP_ICMP_ATTR(ATTR, VAL) CT_DPIF_TP_ATTR_ICMP_##ATTR,
> > CT_DPIF_TP_ICMP_ATTRS
> > #undef CT_DPIF_TP_ICMP_ATTR
> > CT_DPIF_TP_ATTR_MAX
>
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
>
> > @@ -3301,8 +3301,15 @@ udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=> AT_CHECK([ovs-appctl dpctl/flush-conntrack])
> >
> > dnl Shorten the udp_single and icmp_first timeout in zone 5
> > +dnl Userspace datapath uses udp_first and icmp_reply, and
> > +dnl kernel datapath uses udp_single and icmp_first
> > VSCTL_ADD_DATAPATH_TABLE()
> > -AT_CHECK([ovs-vsctl add-zone-tp $DP_TYPE zone=5 udp_single=3 icmp_first=3])
> > +
> > +dnl Creating more timeout policies
> > +for i in `seq 1 255`; do
> > +ovs-vsctl --may-exist add-zone-tp $DP_TYPE zone=5 udp_first=$i udp_single=$i icmp_first=$i icmp_reply=$i;
>
> Should we add zone-tp to different zone?
>
OK
Thanks
William
More information about the dev
mailing list