Thanks for the review On Tue, May 7, 2019 at 2:56 PM Ben Pfaff <[email protected]> wrote:
> 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. */ > oops -fixed > > 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. */ > yeah, I agree. I added it when I got a question about it, but I am going to remove it now. > 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. > 'dp_netdev_free' destroys all the ports and then pmds before calling conntrack_destroy(). I think memory barriers are already implied; meaning, I do NOT think something like the following is needed, although it does no harm. --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -1638,6 +1638,7 @@ dp_netdev_free(struct dp_netdev *dp) ovs_mutex_destroy(&dp->non_pmd_mutex); ovsthread_key_delete(dp->per_pmd_key); + atomic_thread_fence(memory_order_release); conntrack_destroy(dp->conntrack); ovsrcu_synchronize() is also not needed here. > > 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. */ > }; > > Thanks I ended up with a slight variation (to avoid the extra padding): *struct* conn { /* Immutable data. */ *struct* conn_key key; *struct* conn_key rev_key; *struct* conn_key master_key; /* Only used for orig_tuple support. */ *struct* ovs_list exp_node; *struct* cmap_node cm_node; ovs_u128 label; *struct* nat_action_info_t *nat_info; *char* *alg; *struct* conn *nat_conn; /* The NAT 'conn' context, if there is one. */ uint32_t mark; /* Mutable data. */ *struct* ovs_mutex lock; /* Guards all of the following. */ *long* *long* expiration /* 'conn' lock protected. */; *int* seq_skew /* 'conn' lock protected. */; *bool* seq_skew_dir; /* TCP sequence skew direction due to NATTing of FTP + * control messages; true if reply direction. */ /* Immutable data. */ *bool* alg_related; /* True if alg data connection. */ *enum* ct_conn_type conn_type; }; > 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. > */ > Thanks yes, field descriptions are better on the same line as the field. I ended up with: *struct conntrack {* * struct ovs_mutex ct_lock; /* Protects 2 following fields. */* * struct cmap conns OVS_GUARDED;* * struct ovs_list exp_lists[N_CT_TM] 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;* *};* > > /* Fragmentation handling context. */ > struct ipf *ipf; > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
