[ovs-dev] [PATCHv3] userspace: Add conntrack timeout policy support.
Yi-Hung Wei
yihung.wei at gmail.com
Wed Apr 22 22:52:20 UTC 2020
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.
> 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?
> +#include "openvswitch/vlog.h"
May be put this line with the other #include?
> +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.
> +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* ?
> +
> +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.
> +
> +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).
> +
> +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 ?
> +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);
> + }
> + 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__
> +{
> + 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()?
> +
> + 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.
> #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?
> +#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?
> +done
> +AT_CHECK([ovs-vsctl --may-exist add-zone-tp $DP_TYPE zone=5 udp_first=1 udp_single=1 icmp_first=1 icmp_reply=1])
>
> dnl Send ICMP and UDP traffic
> NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 | FORMAT_PING], [0], [dnl
> @@ -3317,7 +3324,7 @@ udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=
>
> dnl Wait until the timeout expire.
> dnl We intend to wait a bit longer, because conntrack does not recycle the entry right after it is expired.
> -sleep 4
> +sleep 5
>
> AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
> ])
> @@ -3335,11 +3342,25 @@ udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=
>
> dnl Wait until the timeout expire.
> dnl We intend to wait a bit longer, because conntrack does not recycle the entry right after it is expired.
> -sleep 4
> +sleep 5
>
> AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2)], [0], [dnl
> ])
>
> +dnl Set the timeout policy to default again.
> +AT_CHECK([ovs-vsctl --may-exist add-zone-tp $DP_TYPE zone=5 icmp_first=30])
Maybe delete this zone-tp?
> +
> +dnl Send ICMP and UDP traffic
> +NS_CHECK_EXEC([at_ns0], [ping -q -c 3 -i 0.3 -w 2 10.1.1.2 | FORMAT_PING], [0], [dnl
> +3 packets transmitted, 3 received, 0% packet loss, time 0ms
> +])
> +AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 packet=50540000000a50540000000908004500001c000000000011a4cd0a0101010a0101020001000200080000 actions=resubmit(,0)"])
Add some delay before dump-conntrack?
> +AT_CHECK([ovs-appctl dpctl/dump-conntrack | FORMAT_CT(10.1.1.2) | sort], [0], [dnl
> +icmp,orig=(src=10.1.1.1,dst=10.1.1.2,id=<cleared>,type=8,code=0),reply=(src=10.1.1.2,dst=10.1.1.1,id=<cleared>,type=0,code=0),zone=5
> +udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=<cleared>,dport=<cleared>),reply=(src=10.1.1.2,dst=10.1.1.1,sport=<cleared>,dport=<cleared>),zone=5
> +])
> +
> OVS_TRAFFIC_VSWITCHD_STOP
> AT_CLEANUP
>
Thanks,
-Yi-Hung
More information about the dev
mailing list