[ovs-dev] [PATCH 1/4] conntrack: remove nat_conn

贺鹏 xnhp0320 at gmail.com
Sat Mar 6 02:45:37 UTC 2021


Gaëtan Rivet <grive at u256.net> 于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 <aconole at redhat.com> 于2021年2月25日周四 上午1:37写道:
> > >
> > > "hepeng.0320" <hepeng.0320 at bytedance.com> writes:
> > >
> > > > From: hepeng <hepeng.0320 at bytedance.com>
> > > >
> > > > 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 <hepeng.0320 at bytedance.com>
> > > > ---
> > > >  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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev



-- 
hepeng


More information about the dev mailing list