Gaëtan Rivet <[email protected]> 于2021年3月3日周三 上午1:46写道： > > On Sat, Feb 27, 2021, at 09:49, 贺鹏 wrote: > > Hi, > > > > Thanks for the detailed reviews. > > These patches are like RFC to see if the work is interesting enough. > > > > Aaron Conole <[email protected]> 于2021年2月25日周四 上午1:37写道： > > > > > > "hepeng.0320" <[email protected]> writes: > > > > > > > From: hepeng <[email protected]> > > > > > > > > Currently, when doing NAT, the userspace conntrack will use an extra > > > > conn for the two directions in a flow. However, each conn has actually > > > > the two keys for both orig and rev flow directions. This patch > > > > introduces a conn_dir member in the conn and it consists of both rev and > > > > orig cmap_node for hash lookup. This saves extra allocation for > > > > nat_conn, and makes userspace code much cleaner. > > > > > > If I'm understanding this correctly, you still re-insert the conn into > > > the conn list? > > > > > > Yes. This is to insert the rev key into the cmap also. So for the nat > > traffic, > > both orig and rev key are in the cmap. > > > > > > > > > We also introduces a conn_flags member to reduce the memory footprint > > > > of a > > > > conn. > > > > > > We should split this out to a separate patch. > > > > > > > Signed-off-by: Peng He <[email protected]> > > > > --- > > > > lib/conntrack-private.h | 20 +-- > > > > lib/conntrack.c | 264 ++++++++++++++++------------------------ > > > > 2 files changed, 121 insertions(+), 163 deletions(-) > > > > > > > > diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h > > > > index 789af82ff..6bec43d3f 100644 > > > > --- a/lib/conntrack-private.h > > > > +++ b/lib/conntrack-private.h > > > > @@ -83,21 +83,23 @@ struct alg_exp_node { > > > > bool nat_rpl_dst; > > > > }; > > > > > > > > -enum OVS_PACKED_ENUM ct_conn_type { > > > > - CT_CONN_TYPE_DEFAULT, > > > > - CT_CONN_TYPE_UN_NAT, > > > > +#define CONN_FLAG_NAT_MASK 0xf > > > > + > > > > +struct conn_dir { > > > > + struct cmap_node cm_node; > > > > + bool orig; > > > > > > This naming is confusing. We can have 'conn->orig->orig'. Consider > > > renaming one of these fields to distinguish what is going on. I would > > > prefer seeing 'fwd' used (since it's the forward direction). > > > > Yes, agree. > > > > > > > > > }; > > > > > > > > struct conn { > > > > /* Immutable data. */ > > > > struct conn_key key; > > > > + struct conn_dir orig; > > > > struct conn_key rev_key; > > > > + struct conn_dir rev; > > > > > > I think this might be better if setup like: > > > +enum ct_direction { > > > + CT_DIRECTION_FWD, > > > + CT_DIRECTION_REV, > > > + CT_DIRECTIONS > > > +} > > > + > > > +struct conn_data { > > > + struct conn_key key; > > > + struct cmap_node cm_node; > > > }; > > > > > > struct conn { > > > - struct conn_key key; > > > - struct conn_key rev_key; > > > + struct conn_data cn_data[CT_DIRECTIONS]; > > > ... > > > > > > Then in the code, we can always get orig and rev information: > > > > > > conn->cn_data[CT_DIRECTION_FWD] > > > conn->cn_data[CT_DIRECTION_REV] > > > > > > Did I miss something? > > > > Since both origin and rev are in the cmap, when you lookup the hash table, > > you get a pointer to the 'middle data structure', in the patch, it is > > the conn_dir. > > > > However, you are not sure what you get is the origin dir or the rev dir. > > The key > > is to use a variable stored in the conn_dir, like 'fwd' or 'orig'. > > Then you know the dir, > > by knowing the dir, you know the offset between your pointer and the > > pointer to > > the conn, just like the belowing code: > > > > static inline struct conn * conn_from_conndir(struct conn_dir *conndir) { > > return conndir->orig ? CONTAINER_OF(conndir, struct conn, orig) : \ > > CONTAINER_OF(conndir, struct conn, rev); > > } > > > > In your case: > > > > static inline struct conn * conn_from_conndir(struct conn_dir *conndir) { > > return conndir->orig ? CONTAINER_OF(conndir, struct conn, cn_data[FWD]) > > : \ > > CONTAINER_OF(conndir, struct conn, cn_data[REV]); > > } > > > > In the following patches I've changed the code of conn_dir as a part > > of conn_key.... > > I haven't seen it in the following patches, I see in the last commit still > struct conn_dir on its own, and not as part of conn_key. Maybe I > misunderstood what you meant? > > Also, logically I'd consider the conn_key as part of the conn_dir, not the > inverse. I would expect conn_key to contain the whole key and only the key of > the conn to compute its hash, no other elements such as a dir mark or a cmap > node. > > would it make sense to have something like > > struct conn_dir { > enum ct_direction dir; > struct conn_key key; > struct cmap_node cm_node; > }; > > struct conn { > [...] > struct conn_dir dir[CT_DIRECTIONS]; > }; > > So the same as what Aaron proposed, but with 'conn_data' renamed as > 'conn_dir' and the enum ct_directions used to mark each conn_dir. Maybe such > enum could be packed to restrict its size to one byte, making it equivalent > to your boolean-based solution?
Yes, it's the same. > > -- > Gaetan Rivet > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev -- hepeng _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
