On Mon, Apr 13, 2020 at 4:32 PM Yi-Hung Wei <yihung....@gmail.com> wrote: > > On Tue, Apr 7, 2020 at 2:08 PM William Tu <u9012...@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 <u9012...@gmail.com> > > Thanks William for implementing this feature in the userspace. I have > some comments as below. > > > > diff --git a/Documentation/faq/releases.rst b/Documentation/faq/releases.rst > > index b3507bd1c7fa..ffcda37f9985 100644 > > --- a/Documentation/faq/releases.rst > > +++ b/Documentation/faq/releases.rst > > @@ -118,7 +118,7 @@ Q: Are all features available with all datapaths? > > ========================== ============== ============== ========= > > ======= > > Connection tracking 4.3 2.5 2.6 > > YES > > Conntrack Fragment Reass. 4.3 2.6 2.12 > > YES > > - Conntrack Timeout Policies 5.2 2.12 NO NO > > + Conntrack Timeout Policies 5.2 2.12 2.13 NO > > For the support of userspace timeout policies in userspace datapath, > should it be 2.14? > Yes, thanks!
> > > > diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h > > index 9a8ca3910157..852482303684 100644 > > --- a/lib/conntrack-private.h > > +++ b/lib/conntrack-private.h > > @@ -118,6 +118,8 @@ struct conn { > > /* Immutable data. */ > > bool alg_related; /* True if alg data connection. */ > > enum ct_conn_type conn_type; > > + > > + uint32_t tpid; /* Timeout policy ID. */ > > I have a minor suggestion here. I would recommend to replace tpid > with tp_id so that when we grep tp_it it would not be confused with > vlan's tpid (tunnel protocol id) field. OK, make sense. > > > > @@ -143,9 +145,9 @@ enum ct_update_res { > > CT_TIMEOUT(ICMP_FIRST, 60 * 1000) \ > > CT_TIMEOUT(ICMP_REPLY, 30 * 1000) > > > > -/* The smallest of the above values: it is used as an upper bound for the > > - * interval between two rounds of cleanup of expired entries */ > > -#define CT_TM_MIN (30 * 1000) > > +/* This is used as an upper bound for the interval between two > > + * rounds of cleanup of expired entries */ > > +#define CT_TM_MIN (1 * 1000) > > I understand that while this patch enables users to customize the > timeout policy, users can set the minimum timeout configuration to 1 > second. Thus, updates CT_TM_MIN to 1 second would make the conntrack > clean thread to check timer expiration every single second to meet > that requirement. However, this may increase the system load even > without any customized timeout policy. I would recommend to update > CT_TM_MIN dynamically to the minimum customized timeout value on the > fly. ok, will do it in next version. > > > > diff --git a/lib/conntrack-tp.c b/lib/conntrack-tp.c > > new file mode 100644 > > index 000000000000..6be4b8276340 > > --- /dev/null > > +++ b/lib/conntrack-tp.c > > @@ -0,0 +1,247 @@ > > +/* > > + * Copyright (c) 2020 VMware, 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. > > + */ > > + > > +#include <config.h> > > + > > +#include "conntrack-private.h" > > +#include "conntrack-tp.h" > > +#include "dp-packet.h" > > + > > +#include "openvswitch/vlog.h" > > +VLOG_DEFINE_THIS_MODULE(conntrack_tp); > > +static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5); > > + > > +static const char *ct_timeout_str[] = { > > +#define CT_TIMEOUT(NAME, VALUE) #NAME > > + CT_TIMEOUTS > > +#undef CT_TIMEOUT > > +}; > > + > > +static inline bool > > +check_present_and_set(struct timeout_policy *tp, uint32_t *v, > > + enum ct_dpif_tp_attr attr) > > +{ > > + if (tp && (tp->p.present & (1 << attr))) { > > + *v = tp->p.attrs[attr]; > > + return true; > > + } > > + return false; > > +} > > + > > +#define TP_HAS_FUNC(ATTR) \ > > +static bool \ > > +tp_has_##ATTR(struct timeout_policy *tp, uint32_t *v) \ > > +{ \ > > + return check_present_and_set(tp, v, CT_DPIF_TP_ATTR_##ATTR); \ > > +} > > + > > +/* These functions check whether the timeout value is set from the > > + * present bit. If true, then set the value to '*v'. For meaning > > + * of each value, see CT_Timeout_Policy table at ovs-vswitchd.conf.db(5). > > + */ > > +TP_HAS_FUNC(TCP_SYN_SENT) > > +TP_HAS_FUNC(TCP_SYN_RECV) > > +TP_HAS_FUNC(TCP_ESTABLISHED) > > +TP_HAS_FUNC(TCP_FIN_WAIT) > > +TP_HAS_FUNC(TCP_TIME_WAIT) > > +TP_HAS_FUNC(TCP_CLOSE) > > +TP_HAS_FUNC(UDP_FIRST) > > +TP_HAS_FUNC(UDP_SINGLE) > > +TP_HAS_FUNC(UDP_MULTIPLE) > > +TP_HAS_FUNC(ICMP_FIRST) > > +TP_HAS_FUNC(ICMP_REPLY) > > + > > +static bool > > +conn_update_expiration_with_policy(struct conntrack *ct, struct conn *conn, > > + enum ct_timeout tm, long long now, > > + struct timeout_policy *tp) > > +{ > > + uint32_t val; > > + > > + switch (tm) { > > + case CT_TM_TCP_FIRST_PACKET: > > + if (tp_has_TCP_SYN_SENT(tp, &val)) { > > + goto update_with_val; > > + } > > + break; > > + case CT_TM_TCP_OPENING: > > + if (tp_has_TCP_SYN_RECV(tp, &val)) { > > + goto update_with_val; > > + } > > + break; > > + case CT_TM_TCP_ESTABLISHED: > > + if (tp_has_TCP_ESTABLISHED(tp, &val)) { > > + goto update_with_val; > > + } > > + break; > > + case CT_TM_TCP_CLOSING: > > + if (tp_has_TCP_FIN_WAIT(tp, &val)) { > > + goto update_with_val; > > + } > > + break; > > + case CT_TM_TCP_FIN_WAIT: > > + if (tp_has_TCP_TIME_WAIT(tp, &val)) { > > + goto update_with_val; > > + } > > + break; > > + case CT_TM_TCP_CLOSED: > > + if (tp_has_TCP_CLOSE(tp, &val)) { > > + goto update_with_val; > > + } > > + break; > > + case CT_TM_OTHER_FIRST: > > + if (tp_has_UDP_FIRST(tp, &val)) { > > + goto update_with_val; > > + } > > + break; > > + case CT_TM_OTHER_BIDIR: > > + if (tp_has_UDP_SINGLE(tp, &val)) { > > + goto update_with_val; > > + } > > + break; > > + case CT_TM_OTHER_MULTIPLE: > > + if (tp_has_UDP_MULTIPLE(tp, &val)) { > > + goto update_with_val; > > + } > > + break; > > + case CT_TM_ICMP_FIRST: > > + if (tp_has_ICMP_FIRST(tp, &val)) { > > + goto update_with_val; > > + } > > + break; > > + case CT_TM_ICMP_REPLY: > > + if (tp_has_ICMP_REPLY(tp, &val)) { > > + goto update_with_val; > > + } > > + break; > > + case N_CT_TM: > > + default: > > + OVS_NOT_REACHED(); > > + break; > > + } > > + return false; > > + > > +update_with_val: > > + conn_update_expiration(ct, conn, tm, now, val, false); > > + return true; > > +} > > + > > +static bool > > +conn_init_expiration_with_policy(struct conntrack *ct, struct conn *conn, > > + enum ct_timeout tm, long long now, > > + struct timeout_policy *tp) > > +{ > > + uint32_t val; > > + > > + switch (tm) { > > + case CT_TM_TCP_FIRST_PACKET: > > + if (tp_has_TCP_SYN_SENT(tp, &val)) { > > + goto init_with_val; > > + } > > + break; > > + case CT_TM_TCP_OPENING: > > + if (tp_has_TCP_SYN_RECV(tp, &val)) { > > + goto init_with_val; > > + } > > + break; > > + case CT_TM_TCP_ESTABLISHED: > > + if (tp_has_TCP_ESTABLISHED(tp, &val)) { > > + goto init_with_val; > > + } > > + break; > > + case CT_TM_TCP_CLOSING: > > + if (tp_has_TCP_FIN_WAIT(tp, &val)) { > > + goto init_with_val; > > + } > > + break; > > + case CT_TM_TCP_FIN_WAIT: > > + if (tp_has_TCP_TIME_WAIT(tp, &val)) { > > + goto init_with_val; > > + } > > + break; > > + case CT_TM_TCP_CLOSED: > > + if (tp_has_TCP_CLOSE(tp, &val)) { > > + goto init_with_val; > > + } > > + break; > > + case CT_TM_OTHER_FIRST: > > + if (tp_has_UDP_FIRST(tp, &val)) { > > + goto init_with_val; > > + } > > + break; > > + case CT_TM_OTHER_BIDIR: > > + if (tp_has_UDP_SINGLE(tp, &val)) { > > + goto init_with_val; > > + } > > + break; > > + case CT_TM_OTHER_MULTIPLE: > > + if (tp_has_UDP_MULTIPLE(tp, &val)) { > > + goto init_with_val; > > + } > > + break; > > + case CT_TM_ICMP_FIRST: > > + if (tp_has_ICMP_FIRST(tp, &val)) { > > + goto init_with_val; > > + } > > + break; > > + case CT_TM_ICMP_REPLY: > > + if (tp_has_ICMP_REPLY(tp, &val)) { > > + goto init_with_val; > > + } > > + break; > > + case N_CT_TM: > > + default: > > + OVS_NOT_REACHED(); > > + break; > > + } > > + return false; > > + > > +init_with_val: > > + conn_init_expiration(ct, conn, tm, now, val, false); > > + return true; > > +} > > + > > +void > > +conn_init_expiration_with_tp(struct conntrack *ct, struct conn *conn, > > + enum ct_timeout tm, long long now) > > +{ > > + struct timeout_policy *tp; > > + > > + tp = timeout_policy_lookup(ct, conn->tpid); > > + if (tp && conn_init_expiration_with_policy(ct, conn, tm, now, tp)) { > > + VLOG_DBG_RL(&rl, "Init timeout %s with policy.", > > + ct_timeout_str[tm]); > > + } else { > > + /* Init with default. */ > > + conn_init_expiration(ct, conn, tm, now, 0, true); > > + } > > +} > > + > > +void > > +conn_update_expiration_with_tp(struct conntrack *ct, struct conn *conn, > > + enum ct_timeout tm, long long now) > > +{ > > + struct timeout_policy *tp; > > + > > + tp = timeout_policy_lookup(ct, conn->tpid); > > + if (tp && conn_update_expiration_with_policy(ct, conn, tm, now, tp)) { > > + VLOG_DBG_RL(&rl, "Update timeout %s with policy.", > > + ct_timeout_str[tm]); > > + } else { > > + /* Update with default. */ > > + conn_update_expiration(ct, conn, tm, now, 0, true); > > + } > > +} > > > > diff --git a/lib/conntrack-tp.h b/lib/conntrack-tp.h > > new file mode 100644 > > index 000000000000..34048ac2aeea > > --- /dev/null > > +++ b/lib/conntrack-tp.h > > @@ -0,0 +1,24 @@ > > +/* > > + * Copyright (c) 2020 VMware, 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_TP_H > > +#define CONNTRACK_TP_H 1 > > + > > +void conn_init_expiration_with_tp(struct conntrack *ct, struct conn *conn, > > + enum ct_timeout tm, long long now); > > +void conn_update_expiration_with_tp(struct conntrack *ct, struct conn > > *conn, > > + enum ct_timeout tm, long long now); > > I think introduce the _with_tp() version may cause confusion to choose > the *with_tp() API or the existing API. Since our goal is to use > customized timeout if it is available. I would suggest the following > two change options. > > 1) Since both conn_init_expiration_with_tp() and > conn_update_expiration_with_tp() did a lookup() on the timeout policy > before init or udpate the expiration time. Maybe we can abstrct the > lookup logic as a seperate function, and call the abstract funcion in > existing conn_init_expiration() and conn_update_expiration()? > > 2) The other options is to maybe to rename existing > conn_update_expiration() to conn_update_expiration__() and rename > conn_update_expiration_with_tp() to conn_update_expiration()? Both > option will avoid changes in the L4 specific conntrack code and have > less confustion about whether to use the _with_tp() API or the > existing API. > OK I will rename the existing one to conn_update_expiration__() > > > diff --git a/lib/conntrack.c b/lib/conntrack.c > > index 0cbc8f6d2b25..de934bdf7169 100644 > > --- a/lib/conntrack.c > > +++ b/lib/conntrack.c > > @@ -430,6 +437,139 @@ zone_limit_delete(struct conntrack *ct, uint16_t zone) > > return 0; > > } > > > > +struct timeout_policy * > > +timeout_policy_get(struct conntrack *ct, int32_t tpid) > > +{ > > + struct timeout_policy *tp; > > + > > + ovs_mutex_lock(&ct->ct_lock); > > + tp = timeout_policy_lookup(ct, tpid); > > + if (!tp) { > > + ovs_mutex_unlock(&ct->ct_lock); > > + return NULL; > > + } > > + > > + ovs_mutex_unlock(&ct->ct_lock); > > + return tp; > > +} > > + > > +struct timeout_policy * > > +timeout_policy_lookup(struct conntrack *ct, int32_t tpid) > > + OVS_REQUIRES(ct->ct_lock) > > +{ > > + struct timeout_policy *tp; > > + uint32_t hash; > > + > > + hash = zone_key_hash(tpid, ct->hash_basis); > > + HMAP_FOR_EACH_IN_BUCKET (tp, node, hash, &ct->timeout_policies) { > > + if (tp->p.id == tpid) { > > + return tp; > > + } > > + } > > + return NULL; > > +} > > + > > +static bool > > +is_valid_tpid(uint32_t tpid OVS_UNUSED) > > +{ > > + return true; > > +} > > We can get rid of this function. > > > > + > > +static int > > +timeout_policy_create(struct conntrack *ct, > > + struct timeout_policy *new_tp) > > + OVS_REQUIRES(ct->ct_lock) > > +{ > > + uint32_t tpid = new_tp->p.id; > > + uint32_t hash; > > + > > + if (is_valid_tpid(tpid)) { > > + struct timeout_policy *tp; > > + > > + tp = xzalloc(sizeof *tp); > > + memcpy(&tp->p, &new_tp->p, sizeof tp->p); > > Instead of directly copy 'new_tp' into 'tp', if we can fill in the > default values for the unpresent fields in the configuration time. > We can reduce the effort to lookup the default vaule at run time in > conntrack-tp.c. Same thing can apply to the update function. I see, looks like a better design. > > > > @@ -1540,14 +1689,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. > > + * 1 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 1000 /* 1 second */ > > Not sure if we need to change CT_CLEAN_INTERVAL from 5 to 1, given > that we set CT_TM_MIN dynamically. > > > > diff --git a/lib/conntrack.h b/lib/conntrack.h > > index b0d0fc8d9597..a55609bb2907 100644 > > --- a/lib/conntrack.h > > +++ b/lib/conntrack.h > > @@ -20,6 +20,7 @@ > > #include <stdbool.h> > > > > #include "cmap.h" > > +#include "ct-dpif.h" > > #include "latch.h" > > #include "odp-netlink.h" > > #include "openvswitch/hmap.h" > > @@ -93,7 +94,7 @@ int 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 tpid); > > void conntrack_clear(struct dp_packet *packet); > > > > struct conntrack_dump { > > @@ -111,6 +112,11 @@ struct conntrack_zone_limit { > > uint32_t zone_limit_seq; /* Used to disambiguate zone limit counts. */ > > }; > > > > +struct timeout_policy { > > + struct hmap_node node; > > + struct ct_dpif_timeout_policy p; > > It seems to be more clear to me to name it as 'tp' than 'p'. > > > > @@ -139,5 +145,11 @@ struct conntrack_zone_limit zone_limit_get(struct > > conntrack *ct, > > int32_t zone); > > int zone_limit_update(struct conntrack *ct, int32_t zone, uint32_t limit); > > int zone_limit_delete(struct conntrack *ct, uint16_t zone); > > +int timeout_policy_update(struct conntrack *ct, struct timeout_policy *tp); > > +int timeout_policy_delete(struct conntrack *ct, uint32_t tpid); > > +struct timeout_policy *timeout_policy_get(struct conntrack *ct, int32_t > > tpid); > > +struct timeout_policy *timeout_policy_lookup(struct conntrack *ct, > > + int32_t tpid); > > + > > > > #endif /* conntrack.h */ > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > index e456cc9befbe..41e805a832c8 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -7337,6 +7337,7 @@ dp_execute_cb(void *aux_, struct dp_packet_batch > > *packets_, > > bool commit = false; > > unsigned int left; > > uint16_t zone = 0; > > + uint32_t tpid = 0; > > const char *helper = NULL; > > const uint32_t *setmark = NULL; > > const struct ovs_key_ct_labels *setlabel = NULL; > > @@ -7372,8 +7373,9 @@ dp_execute_cb(void *aux_, struct dp_packet_batch > > *packets_, > > * netlink events. */ > > break; > > case OVS_CT_ATTR_TIMEOUT: > > - /* Userspace datapath does not support customized timeout > > - * policy yet. */ > > + if (!str_to_uint(nl_attr_get_string(b), 10, &tpid)) { > > + VLOG_WARN("Invalid tpid %s.", nl_attr_get_string(b)); > > + } > > Looks like if the OVS_CT_ATTR_TIMEOUT is not present, we will always > use timeout policy 0 rather than the default timeout policy, since > from ofproto-dpif.c the tiemout policy id pools allocate id number > from starting from 0. One simple fix may be to adjust the timeout > policy id pool (backer->tpids) to start from 1, so that userspace can > use 0 as the reserved default timeout policy. Kernel space is > agnostic to the id pool starting number. > thanks! William _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev