[ovs-dev] [PATCH 11/12] ofproto-dpif-xlate: Translate timeout policy in ct action

Darrell Ball dlu998 at gmail.com
Fri Jul 26 05:02:12 UTC 2019


Thanks for the patch

Few comments inline

On Thu, Jul 25, 2019 at 4:30 PM Yi-Hung Wei <yihung.wei at gmail.com> wrote:

> This patch derives the timeout policy based on ct zone from the
> internal data structure that reads the configuration from ovsdb.
>
> Signed-off-by: Yi-Hung Wei <yihung.wei at gmail.com>
> ---
>  lib/ct-dpif.c                | 10 ++++++++++
>  lib/ct-dpif.h                |  3 +++
>  lib/datapath-config.c        | 30 ++++++++++++++++++++++++++++++
>  lib/datapath-config.h        |  2 ++
>  lib/dpif-netdev.c            |  1 +
>  lib/dpif-netlink.c           | 10 ++++++++++
>  lib/dpif-provider.h          |  5 +++++
>  ofproto/ofproto-dpif-xlate.c | 23 +++++++++++++++++++++++
>  8 files changed, 84 insertions(+)
>
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index 1625754e2441..e2c96b29b17f 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -864,3 +864,13 @@ ct_dpif_timeout_policy_dump_done(struct dpif *dpif,
> void *state)
>              ? dpif->dpif_class->ct_timeout_policy_dump_done(dpif, state)
>              : EOPNOTSUPP);
>  }
> +
> +int
> +ct_dpif_format_timeout_policy_name(struct dpif *dpif, uint32_t tp_id,
> +                                   uint16_t dl_type, uint8_t nw_proto,
> +                                   struct ds *ds)
> +{
> +    return (dpif->dpif_class->ct_format_timeout_policy_name
> +            ? dpif->dpif_class->ct_format_timeout_policy_name(
> +                dpif, tp_id, dl_type, nw_proto, ds) : EOPNOTSUPP);
> +}
> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index de032cc416ce..a4e91f975a9b 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -318,5 +318,8 @@ int ct_dpif_timeout_policy_dump_start(struct dpif
> *dpif, void **statep);
>  int ct_dpif_timeout_policy_dump_next(struct dpif *dpif, void *state,
>                                       struct ct_dpif_timeout_policy **tp);
>  int ct_dpif_timeout_policy_dump_done(struct dpif *dpif, void *state);
> +int ct_dpif_format_timeout_policy_name(struct dpif *dpif, uint32_t tp_id,
> +                                       uint16_t dl_type, uint8_t nw_proto,
> +                                       struct ds *ds);
>
>  #endif /* CT_DPIF_H */
> diff --git a/lib/datapath-config.c b/lib/datapath-config.c
> index cdd2128a60bc..2f3852100b78 100644
> --- a/lib/datapath-config.c
> +++ b/lib/datapath-config.c
> @@ -377,3 +377,33 @@ destroy_all_datapaths(void)
>          datapath_destroy(dp);
>      }
>  }
> +
> +/* If timeout policy is found in datapath '*dp_type' and in 'zone',
> + * sets timeout policy id in '*tp_id' and returns true. Otherwise,
> + * returns false. */
> +bool
> +datapath_get_zone_timeout_policy_id(const char *dp_type, uint16_t zone,
> +                                    uint32_t *tp_id)
> +{
> +    struct datapath *dp;
> +    struct ct_zone *ct_zone;
> +    struct ct_timeout_policy *ct_tp;
> +
> +    dp = datapath_lookup(dp_type);
> +    if (!dp) {
> +        return false;
> +    }
> +
> +    ct_zone = ct_zone_lookup(&dp->ct_zones, zone);
> +    if (!ct_zone) {
> +        return false;
> +    }
> +
> +    ct_tp = ct_timeout_policy_lookup(&dp->ct_tps, &ct_zone->tp_uuid);
> +    if (!ct_tp) {
> +        return false;
> +    }
> +
> +    *tp_id = ct_tp->cdtp.id;
> +    return true;
> +}
> diff --git a/lib/datapath-config.h b/lib/datapath-config.h
> index d9a90e4f8312..0e0cd7eaad2f 100644
> --- a/lib/datapath-config.h
> +++ b/lib/datapath-config.h
> @@ -21,5 +21,7 @@
>  void reconfigure_datapath(const struct ovsrec_open_vswitch *,
>                            unsigned int idl_seqno);
>  void destroy_all_datapaths(void);
> +bool datapath_get_zone_timeout_policy_id(const char *dp_type, uint16_t
> zone,
> +                                         uint32_t *tp_id);
>
>  #endif /* datapath-config.h */
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 7240a3e6f3c8..19cf9f21ec85 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -7539,6 +7539,7 @@ const struct dpif_class dpif_netdev_class = {
>      NULL,                       /* ct_timeout_policy_dump_start */
>      NULL,                       /* ct_timeout_policy_dump_next */
>      NULL,                       /* ct_timeout_policy_dump_done */
> +    NULL,                       /* ct_format_timeout_policy_name */
>      dpif_netdev_ipf_set_enabled,
>      dpif_netdev_ipf_set_min_frag,
>      dpif_netdev_ipf_set_max_nfrags,
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index abfad9543c3b..8e6f2ebb51e1 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -3076,6 +3076,15 @@ dpif_netlink_format_tp_name(uint32_t id, uint16_t
> l3num, uint8_t l4num,
>      ovs_assert(tp_name->length < CTNL_TIMEOUT_NAME_MAX);
>  }
>
> +static int
> +dpif_netlink_ct_format_timeout_policy_name(struct dpif *dpif OVS_UNUSED,
> +    uint32_t tp_id, uint16_t dl_type, uint8_t nw_proto, struct ds *ds)
> +{
> +    dpif_netlink_format_tp_name(tp_id,
> +        dl_type == ETH_TYPE_IP ? AF_INET : AF_INET6, nw_proto, ds);
> +    return 0;
> +}
> +
>  #define CT_DPIF_TO_NL_TP_TCP_MAPPINGS \
>      CT_DPIF_TO_NL_TP_MAPPING(TCP, TCP, SYN_SENT, SYN_SENT) \
>      CT_DPIF_TO_NL_TP_MAPPING(TCP, TCP, SYN_RECV, SYN_RECV) \
> @@ -3860,6 +3869,7 @@ const struct dpif_class dpif_netlink_class = {
>      dpif_netlink_ct_timeout_policy_dump_start,
>      dpif_netlink_ct_timeout_policy_dump_next,
>      dpif_netlink_ct_timeout_policy_dump_done,
> +    dpif_netlink_ct_format_timeout_policy_name,
>      NULL,                       /* ipf_set_enabled */
>      NULL,                       /* ipf_set_min_frag */
>      NULL,                       /* ipf_set_max_nfrags */
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 3460ef8aa98d..f01f3abee5ab 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -541,6 +541,11 @@ struct dpif_class {
>                                         struct ct_dpif_timeout_policy
> **tp);
>      int (*ct_timeout_policy_dump_done)(struct dpif *, void *state);
>
> +    /* Get timeout policy name (OVS_CT_ATTR_TIMEOUT) from datapath. */
> +    int (*ct_format_timeout_policy_name)(struct dpif *, uint32_t tp_id,
> +                                         uint16_t dl_type, uint8_t
> nw_proto,
> +                                         struct ds *ds);
> +
>

The above generic API implies that 'dl_type' and 'nw_proto' are always
needed, which is
not true in general.
I think these parameters could have generic names in this base class.
The corresponding types could be uint32_t as well so as not to imply any
particular context.

If this is going to delay things, it can be done later from my POV.



>      /* IP Fragmentation. */
>
>      /* Disables or enables conntrack fragment reassembly.  The default
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 28a7fdd842a6..12fa9684d0e4 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -28,10 +28,12 @@
>  #include "bond.h"
>  #include "bundle.h"
>  #include "byte-order.h"
> +#include "ct-dpif.h"
>  #include "cfm.h"
>  #include "connmgr.h"
>  #include "coverage.h"
>  #include "csum.h"
> +#include "datapath-config.h"
>  #include "dp-packet.h"
>  #include "dpif.h"
>  #include "in-band.h"
> @@ -5977,6 +5979,25 @@ put_ct_helper(struct xlate_ctx *ctx,
>  }
>
>  static void
> +put_ct_timeout(struct ofpbuf *odp_actions, const char *dp_type,
> +               struct dpif *dpif, const struct flow *flow,
> +               struct flow_wildcards *wc, uint16_t zone_id)
> +{
> +    uint32_t tp_id;
> +
> +    if (datapath_get_zone_timeout_policy_id(dp_type, zone_id, &tp_id)) {
> +        struct ds ds = DS_EMPTY_INITIALIZER;
> +        int err = ct_dpif_format_timeout_policy_name(
> +                    dpif, tp_id, ntohs(flow->dl_type), flow->nw_proto,
> &ds);
> +        if (!err) {
>



> +            memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
>

I think the above unwildcarding should be wrapped in a datapath type
specific (i.e. 'system' in this case)
function.

Also a note should be added to the function describing the why the
additional datapath flows are
needed (i.e. bcoz of Netfilter timeouts implementation). Note that these
datapath flows will remain if kept
minimally active, which is generally undesirable, hence needs to be flagged.

If this is going to delay things, it can be done later from my POV.



> +            nl_msg_put_string(odp_actions, OVS_CT_ATTR_TIMEOUT,
> ds_cstr(&ds));
> +        }
> +        ds_destroy(&ds);
> +    }
> +}
> +
> +static void
>  put_ct_nat(struct xlate_ctx *ctx)
>  {
>      struct ofpact_nat *ofn = ctx->ct_nat_action;
> @@ -6071,6 +6092,8 @@ compose_conntrack_action(struct xlate_ctx *ctx,
> struct ofpact_conntrack *ofc,
>      put_ct_mark(&ctx->xin->flow, ctx->odp_actions, ctx->wc);
>      put_ct_label(&ctx->xin->flow, ctx->odp_actions, ctx->wc);
>      put_ct_helper(ctx, ctx->odp_actions, ofc);
> +    put_ct_timeout(ctx->odp_actions, ctx->xbridge->ofproto->backer->type,
> +                   ctx->xbridge->dpif, &ctx->xin->flow, ctx->wc, zone);
>      put_ct_nat(ctx);
>      ctx->ct_nat_action = NULL;
>      nl_msg_end_nested(ctx->odp_actions, ct_offset);
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list