On Mon, May 06, 2019 at 09:20:53AM -0700, Darrell Ball wrote:
> For performance and code simplification reasons, add rcu support for
> conntrack. The array of hmaps is replaced by a cmap as part of this
> conversion. Using a single map also simplifies the handling of NAT
> and allows the removal of the nat_conn map and friends. Per connection
> entry locks are introduced, which are needed in a few code paths.
>
> Signed-off-by: Darrell Ball <[email protected]>
> ---
>
> v7: Document 'conn' locking better; not using OVS_GUARDED bcoz that
> would require several OVS_NO_THREAD_SAFETY_ANALYSIS to avoid
> clang false positive.
>
> Use OVS_PACKED_ENUM.
Thanks!
This style of putting the comment before the ; looks odd to me:
enum icmp_state state /* 'conn' lock protected. */;
Normally I see it as:
enum icmp_state state; /* 'conn' lock protected. */
I don't understand the following comment. It's needed because it's
used? That usually goes without saying.
/* Needed because of usage in CMAP_FOR_EACH. */
static void
conn_clean_one(struct conntrack *ct, struct conn *conn)
I am not confident about destruction ordering here. It appears that
conntrack_destroy() frees 'ct'. I don't see anything that assures that
a grace period has expired before this. In the meantime, it seems that
packet processing could access 'ct'. I think that the same is true for,
e.g., ct_lock. I guess one way it could be safe if is
conntrack_destroy() requires the caller to have already quiesced packet
processing for a grace period, but that doesn't seem to be documented
and I don't think dpif-netdev does that.
I played around with the formatting for the structs and came up with the
following, which seemed clearer to me.
diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
index 7af0f9bb0357..3c49eb489b2c 100644
--- a/lib/conntrack-private.h
+++ b/lib/conntrack-private.h
@@ -92,23 +92,25 @@ struct conn {
struct conn_key key;
struct conn_key rev_key;
struct conn_key master_key; /* Only used for orig_tuple support. */
- /* Guards fields that can be updated; this includes protocol specific
- * state. */
- struct ovs_mutex lock;
- long long expiration /* 'conn' lock protected. */;
+
struct ovs_list exp_node;
struct cmap_node cm_node;
+
+ /* Immutable data. */
ovs_u128 label;
+ uint32_t mark;
struct nat_action_info_t *nat_info;
char *alg;
struct conn *nat_conn; /* The NAT 'conn' context, if there is one. */
- int seq_skew /* 'conn' lock protected. */;
- uint32_t mark;
enum ct_conn_type conn_type;
- /* TCP sequence skew direction due to NATTing of FTP control messages;
- * true if reply direction. */
- bool seq_skew_dir /* 'conn' lock protected. */;
bool alg_related; /* True if alg data connection. */
+
+ /* Mutable data. */
+ struct ovs_mutex lock; /* Guards all of the following. */
+ long long expiration;
+ int seq_skew;
+ bool seq_skew_dir; /* TCP sequence skew direction due to NATTing of FTP
+ * control messages; true if reply direction. */
};
enum ct_update_res {
@@ -153,28 +155,20 @@ struct conntrack {
struct cmap conns OVS_GUARDED;
struct ovs_list exp_lists[N_CT_TM] OVS_GUARDED;
- /* Salt for hashing a connection key. */
- uint32_t hash_basis;
- /* The thread performing periodic cleanup of the connection
- * tracker. */
- pthread_t clean_thread;
- /* Latch to destroy the 'clean_thread' */
- struct latch clean_thread_exit;
-
- /* Number of connections currently in the connection tracker. */
- atomic_count n_conn;
- /* Connections limit. When this limit is reached, no new connection
- * will be accepted. */
- atomic_uint n_conn_limit;
-
- /* This lock is used to guard alg_expectations and alg_expectation_refs.
- */
- struct ovs_rwlock resources_lock;
- /* Hash table for alg expectations. Expectations are created
- * by control connections to help create data connections. */
- struct hmap alg_expectations OVS_GUARDED;
- /* Used to lookup alg expectations from the control context. */
- struct hindex alg_expectation_refs OVS_GUARDED;
+ uint32_t hash_basis; /* Salt for hashing a connection key. */
+ pthread_t clean_thread; /* Periodically cleans up connection tracker. */
+ struct latch clean_thread_exit; /* To destroy the 'clean_thread'. */
+
+ /* Counting connections. */
+ atomic_count n_conn; /* Number of connections currently tracked. */
+ atomic_uint n_conn_limit; /* Max connections tracked. */
+
+ /* Expectations for application level gateways (created by control
+ * connections to help create data connections, e.g. for FTP). */
+ struct ovs_rwlock resources_lock; /* Protects fields below. */
+ struct hmap alg_expectations OVS_GUARDED; /* Holds struct alg_exp_nodes. */
+ struct hindex alg_expectation_refs OVS_GUARDED; /* For lookup from
+ * control context. */
/* Fragmentation handling context. */
struct ipf *ipf;
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
