[ovs-dev] [PATCH 3/4] conntrack: refactoring alg handle code path

Gaëtan Rivet grive at u256.net
Tue Mar 2 19:48:27 UTC 2021


On Sun, Nov 29, 2020, at 03:32, hepeng.0320 wrote:
> From: hepeng <hepeng.0320 at bytedance.com>
> 
> Just like the previous patch, split the handle_ftp_ctl into
> handle_ftp_interest and handle_ftp_other, since we can infer
> which one to call from the called positions.
> 
> We also introduce a simple framework which unifies the normal
> conn handle path and alg conn handle path by using two hooks:
> *before_conn_update_hook* and *after_conn_update_hook*.
> 

Reading this 'also', I thought initially the commit should be split and the two changes, dividing the ALG CTL in two and introducing the framework were unrelated. Reading further, it's clearer now that this is one logical change.
I think the link between the two should be expanded in the commit log.

The motivation I get for this commit is to "unify the normal and ALG conn handle path". However, only the FTP ALG benefits from the hooks introduced. The code is streamlined and a single-purpose function is removed, but it seems flimsy as a motivation to introduce a new callback system.

Can you expand on the goal of the framework, and which ALG it could help to implement?
It is hard to judge whether the framework is useful and properly defined without having a full picture.

> Signed-off-by: Peng He <hepeng.0320 at bytedance.com>
> ---
>  lib/conntrack-private.h |  16 +-
>  lib/conntrack.c         | 477 ++++++++++++++++++++++++----------------
>  2 files changed, 307 insertions(+), 186 deletions(-)
> 
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index 6bec43d3f..2aa828674 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -83,7 +83,22 @@ struct alg_exp_node {
>      bool nat_rpl_dst;
>  };
>  
> +enum ct_alg_ctl_type {
> +    CT_ALG_CTL_NONE,
> +    CT_ALG_CTL_FTP,
> +    CT_ALG_CTL_TFTP,
> +    /* SIP is not enabled through Openflow and presently only used as
> +     * an example of an alg that allows a wildcard src ip. */
> +    CT_ALG_CTL_SIP,
> +    CT_ALG_CTL_MAX,

I don't see this CT_ALG_CTL_MAX used afterward in this commit?
It is introduced by this change.

> +};
> +
>  #define CONN_FLAG_NAT_MASK 0xf
> +#define CONN_FLAG_CTL_FTP  (CT_ALG_CTL_FTP  << 4)
> +#define CONN_FLAG_CTL_TFTP (CT_ALG_CTL_TFTP << 4)
> +#define CONN_FLAG_CTL_SIP  (CT_ALG_CTL_SIP  << 4)
> +/* currently only 3 algs supported */
> +#define CONN_FLAG_ALG_MASK 0x70

The enum values are not flags.
I guess they should instead be defined as

#define CONN_FLAG_CTL_FTP (UINT64_C(1) << (4 + CT_ALG_CTL_FTP))

In the code this should have no effect as SIP is not implemented, so only ALG were 1 and 2, which are valid flags.

>  
>  struct conn_dir {
>      struct cmap_node cm_node;
> @@ -99,7 +114,6 @@ struct conn {
>      struct conn_key parent_key; /* Only used for orig_tuple support. */
>      struct ovs_list exp_node;
>      uint64_t conn_flags;
> -    char *alg;
>  
>      /* Mutable data. */
>      struct ovs_mutex lock; /* Guards all mutable fields. */
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index a22252a63..fffc617fb 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -69,15 +69,6 @@ enum ct_alg_mode {
>      CT_TFTP_MODE,
>  };
>  
> -enum ct_alg_ctl_type {
> -    CT_ALG_CTL_NONE,
> -    CT_ALG_CTL_FTP,
> -    CT_ALG_CTL_TFTP,
> -    /* SIP is not enabled through Openflow and presently only used as
> -     * an example of an alg that allows a wildcard src ip. */
> -    CT_ALG_CTL_SIP,
> -};
> -
>  struct zone_limit {
>      struct hmap_node node;
>      struct conntrack_zone_limit czl;
> @@ -153,30 +144,145 @@ static struct ct_l4_proto *l4_protos[] = {
>  };
>  
>  static void
> -handle_ftp_ctl(struct conntrack *ct, const struct conn_lookup_ctx *ctx,
> -               struct dp_packet *pkt, struct conn *ec, long long now,
> -               enum ftp_ctl_pkt ftp_ctl, bool nat);
> -
> +handle_ftp_ctl_other(struct conntrack *ct, const struct 
> conn_lookup_ctx *ctx,
> +                     struct dp_packet *pkt, struct conn *ec, long long 
> now,
> +                     bool nat);
> +static void
> +handle_ftp_ctl_interest(struct conntrack *ct, const struct 
> conn_lookup_ctx *ctx,
> +                        struct dp_packet *pkt, struct conn *ec, long 
> long now,
> +                        bool nat);
>  static void
>  handle_tftp_ctl(struct conntrack *ct,
> -                const struct conn_lookup_ctx *ctx OVS_UNUSED,
> -                struct dp_packet *pkt, struct conn 

Parameters are misaligned in this prototype.

> *conn_for_expectation,
> -                long long now OVS_UNUSED, enum ftp_ctl_pkt ftp_ctl 
> OVS_UNUSED,
> -                bool nat OVS_UNUSED);
> +                 const struct conn_lookup_ctx *ctx OVS_UNUSED,
> +                 struct dp_packet *pkt, struct conn 
> *conn_for_expectation,
> +                 long long now OVS_UNUSED, bool nat OVS_UNUSED);
> +
>  
>  typedef void (*alg_helper)(struct conntrack *ct,
>                             const struct conn_lookup_ctx *ctx,
>                             struct dp_packet *pkt,
>                             struct conn *conn_for_expectation,
> -                           long long now, enum ftp_ctl_pkt ftp_ctl,
> -                           bool nat);
> +                           long long now, bool nat);
> +
> +typedef void (*before_helper)(struct conntrack *ct,
> +                              const struct conn_lookup_ctx *ctx,
> +                              struct dp_packet *pkt,
> +                              struct conn *conn_for_expectation,
> +                              long long now, bool nat);
> +
> +
> +typedef void (*after_helper)(struct conntrack *ct,
> +                             const struct conn_lookup_ctx *ctx,
> +                             struct dp_packet *pkt,
> +                             struct conn *conn_for_expectation,
> +                             long long now, bool nat, bool create_new_conn);
> +

Can you describe more the potential use-cases for the before and after hooks?
It seems only applied for FTP ALG.

What should be the API? How should one implement those helpers in the future, if more ALGs are added?
Should we wait instead to support more ALG before adding them, to better judge their relevance?

>  
> -static alg_helper alg_helpers[] = {
> -    [CT_ALG_CTL_NONE] = NULL,
> -    [CT_ALG_CTL_FTP] = handle_ftp_ctl,
> -    [CT_ALG_CTL_TFTP] = handle_tftp_ctl,
> +struct alg_helper_hook {
> +    before_helper before_conn_update_hook;
> +    after_helper  after_conn_update_hook;
> +    alg_helper alg_helper_hook;
>  };
>  
> +static void ftp_ctl_before_hook(struct conntrack *ct, const struct 
> conn_lookup_ctx *ctx,
> +                                struct dp_packet *pkt, struct conn 
> *ec, long long now,
> +                                bool nat)
> +{
> +    enum ftp_ctl_pkt pkt_type = detect_ftp_ctl_type(ctx, pkt);
> +    if (pkt_type == CT_FTP_CTL_INTEREST)
> +        return;
> +
> +    ovs_mutex_lock(&ec->lock);
> +    if (ctx->reply != ec->seq_skew_dir) {
> +        handle_ftp_ctl_other(ct, ctx, pkt, ec, now, nat);
> +    }
> +    ovs_mutex_unlock(&ec->lock);
> +}
> +
> +static void ftp_ctl_after_hook(struct conntrack *ct, const struct 
> conn_lookup_ctx *ctx,
> +                               struct dp_packet *pkt, struct conn *ec, 
> long long now,
> +                               bool nat, bool create_new_conn)
> +{
> +    enum ftp_ctl_pkt pkt_type = detect_ftp_ctl_type(ctx, pkt);
> +    if (pkt_type == CT_FTP_CTL_INTEREST)
> +        return;
> +
> +    if (create_new_conn == false) {
> +        ovs_mutex_lock(&ec->lock);
> +        if (ctx->reply == ec->seq_skew_dir) {
> +            handle_ftp_ctl_other(ct, ctx, pkt, ec, now, nat);
> +        }
> +        ovs_mutex_unlock(&ec->lock);
> +    }
> +}
> +
> +static void handle_ftp_ctl(struct conntrack *ct, const struct 
> conn_lookup_ctx *ctx,
> +                           struct dp_packet *pkt, struct conn *ec, 
> long long now,
> +                           bool nat)
> +{
> +    if (detect_ftp_ctl_type(ctx, pkt) != CT_FTP_CTL_INTEREST)
> +        return;
> +
> +    ovs_mutex_lock(&ec->lock);
> +    handle_ftp_ctl_interest(ct, ctx, pkt, ec, now, nat);
> +    ovs_mutex_unlock(&ec->lock);
> +}
> +
> +static struct alg_helper_hook alg_helper_hooks[] = {
> +    [CT_ALG_CTL_NONE] = { .before_conn_update_hook = NULL, \
> +                          .after_conn_update_hook = NULL, \
> +                          .alg_helper_hook = NULL },
> +    [CT_ALG_CTL_FTP]  = { .before_conn_update_hook = 
> ftp_ctl_before_hook, \
> +                          .after_conn_update_hook = 
> ftp_ctl_after_hook, \
> +                          .alg_helper_hook = handle_ftp_ctl },
> +    [CT_ALG_CTL_TFTP] = { .before_conn_update_hook = NULL, \
> +                          .after_conn_update_hook = NULL, \
> +                          .alg_helper_hook = handle_tftp_ctl },
> +};
> +
> +static void
> +conn_update_before_hook(enum ct_alg_ctl_type type, struct conntrack 
> *ct, const struct conn_lookup_ctx *ctx,
> +                        struct dp_packet *pkt, struct conn *ec, long 
> long now,
> +                        bool nat)
> +{
> +    if (OVS_LIKELY(type == CT_ALG_CTL_NONE)) {
> +        return;
> +    }
> +

The above check is unnecessary with the following one, as CTL_NONE will have the before hook NULL.
Is it only used to have the OVS_LIKELY? If so, I'm not convinced it is helpful, usually those markers will help with specific CPUs, in specific use-cases (benchmarks and synthetic tests).

> +
> +    if (alg_helper_hooks[type].before_conn_update_hook)
> +        alg_helper_hooks[type].before_conn_update_hook(ct, ctx, pkt, ec, now, nat);
> +}

-- 
Gaetan Rivet


More information about the dev mailing list