correction below for 'struct conn' inline comments
On Wed, May 8, 2019 at 12:14 AM Darrell Ball <[email protected]> wrote: > 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; > > }; > correction: 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; int seq_skew; 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
