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

Gaëtan Rivet grive at u256.net
Tue Mar 2 17:45:29 UTC 2021


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?

-- 
Gaetan Rivet


More information about the dev mailing list