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

Darrell Ball dlu998 at gmail.com
Sat Jul 27 18:33:18 UTC 2019


One more comment inline

On Thu, Jul 25, 2019 at 10:02 PM Darrell Ball <dlu998 at gmail.com> wrote:

> 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);
>>
>
Let us change:

put_ct_timeout(ctx->odp_actions, ctx->xbridge->ofproto->backer->type,
                        ctx->xbridge->dpif, &ctx->xin->flow, ctx->wc, zone);

to

if (ofc->flags & NX_CT_F_COMMIT) {
    put_ct_timeout(ctx->odp_actions, ctx->xbridge->ofproto->backer->type,
                             ctx->xbridge->dpif, &ctx->xin->flow, ctx->wc,
zone);
}

since we only need the unwildcarding for 'commit' related flows

This will save lots of unnecessary datapath flows.



>      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