[ovs-dev] [PATCH 05/12] ct-dpif: Add conntrack timeout policy support in dpif layer

Darrell Ball dlu998 at gmail.com
Fri Jul 26 18:41:13 UTC 2019


Thanks for the patch

I found this patch hard to review since it does not contain implementations
The same comment applies to Patch 6
I think Patches 5-7 can be combined into one patch, which will make review
easier.

some other comments inline

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

> This patch defines the dpif interface for a datapath to support
> adding, deleting, getting and dumping conntrack timeout policy.
> The timeout policy is identified by a 4 bytes unsigned integer in
> datapath, and it currently support timeout for TCP, UDP, and ICMP
> protocols.
>
> Signed-off-by: Yi-Hung Wei <yihung.wei at gmail.com>
> ---
>  lib/ct-dpif.c       | 51
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/ct-dpif.h       | 53
> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/dpif-netdev.c   |  6 ++++++
>  lib/dpif-netlink.c  |  6 ++++++
>  lib/dpif-provider.h | 43 +++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 159 insertions(+)
>
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index 6ea7feb0ee35..ae347a9bb46d 100644
> --- a/lib/ct-dpif.c
> +++ b/lib/ct-dpif.c
> @@ -760,3 +760,54 @@ ct_dpif_format_zone_limits(uint32_t default_limit,
>          ds_put_format(ds, ",count=%"PRIu32, zone_limit->count);
>      }
>  }
> +
> +int
> +ct_dpif_add_timeout_policy(struct dpif *dpif, bool is_default,
> +                           const struct ct_dpif_timeout_policy *tp)
> +{
> +    return (dpif->dpif_class->ct_add_timeout_policy
> +            ? dpif->dpif_class->ct_add_timeout_policy(dpif, is_default,
> tp)
> +            : EOPNOTSUPP);
> +}
> +
> +int
> +ct_dpif_del_timeout_policy(struct dpif *dpif, uint32_t tp_id)
> +{
> +    return (dpif->dpif_class->ct_del_timeout_policy
> +            ? dpif->dpif_class->ct_del_timeout_policy(dpif, tp_id)
> +            : EOPNOTSUPP);
> +}
> +
> +int
> +ct_dpif_get_timeout_policy(struct dpif *dpif, bool is_default, uint32_t
> tp_id,
> +                           struct ct_dpif_timeout_policy *tp)
> +{
> +    return (dpif->dpif_class->ct_get_timeout_policy
> +            ? dpif->dpif_class->ct_get_timeout_policy(
> +                dpif, is_default, tp_id, tp) : EOPNOTSUPP);
> +}
> +
> +int
> +ct_dpif_timeout_policy_dump_start(struct dpif *dpif, void **statep)
> +{
> +    return (dpif->dpif_class->ct_timeout_policy_dump_start
> +            ? dpif->dpif_class->ct_timeout_policy_dump_start(dpif, statep)
> +            : EOPNOTSUPP);
> +}
> +
> +int
> +ct_dpif_timeout_policy_dump_next(struct dpif *dpif, void *state,
> +                                 struct ct_dpif_timeout_policy **tp)
> +{
> +    return (dpif->dpif_class->ct_timeout_policy_dump_next
> +            ? dpif->dpif_class->ct_timeout_policy_dump_next(dpif, state,
> tp)
> +            : EOPNOTSUPP);
> +}
> +
> +int
> +ct_dpif_timeout_policy_dump_done(struct dpif *dpif, void *state)
> +{
> +    return (dpif->dpif_class->ct_timeout_policy_dump_done
> +            ? dpif->dpif_class->ct_timeout_policy_dump_done(dpif, state)
> +            : EOPNOTSUPP);
> +}
> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index 2f4906817946..9dc33bede527 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -225,6 +225,49 @@ struct ct_dpif_zone_limit {
>      struct ovs_list node;
>  };
>
> +#define CT_DPIF_TP_TCP_ATTRS \
> +    CT_DPIF_TP_TCP_ATTR(SYN_SENT) \
> +    CT_DPIF_TP_TCP_ATTR(SYN_RECV) \
> +    CT_DPIF_TP_TCP_ATTR(ESTABLISHED) \
> +    CT_DPIF_TP_TCP_ATTR(FIN_WAIT) \
> +    CT_DPIF_TP_TCP_ATTR(CLOSE_WAIT) \
> +    CT_DPIF_TP_TCP_ATTR(LAST_ACK) \
> +    CT_DPIF_TP_TCP_ATTR(TIME_WAIT) \
> +    CT_DPIF_TP_TCP_ATTR(CLOSE) \
> +    CT_DPIF_TP_TCP_ATTR(SYN_SENT2) \
> +    CT_DPIF_TP_TCP_ATTR(RETRANSMIT) \
> +    CT_DPIF_TP_TCP_ATTR(UNACK)
> +
> +#define CT_DPIF_TP_UDP_ATTRS \
> +    CT_DPIF_TP_UDP_ATTR(FIRST) \
> +    CT_DPIF_TP_UDP_ATTR(SINGLE) \
> +    CT_DPIF_TP_UDP_ATTR(MULTIPLE)
> +
> +#define CT_DPIF_TP_ICMP_ATTRS \
> +    CT_DPIF_TP_ICMP_ATTR(FIRST) \
> +    CT_DPIF_TP_ICMP_ATTR(REPLY)
> +
> +enum OVS_PACKED_ENUM ct_dpif_tp_attr {
> +#define CT_DPIF_TP_TCP_ATTR(ATTR) CT_DPIF_TP_ATTR_TCP_##ATTR,
> +    CT_DPIF_TP_TCP_ATTRS
> +#undef CT_DPIF_TP_TCP_ATTR
> +#define CT_DPIF_TP_UDP_ATTR(ATTR) CT_DPIF_TP_ATTR_UDP_##ATTR,
> +    CT_DPIF_TP_UDP_ATTRS
> +#undef CT_DPIF_TP_UDP_ATTR
> +#define CT_DPIF_TP_ICMP_ATTR(ATTR) CT_DPIF_TP_ATTR_ICMP_##ATTR,
> +    CT_DPIF_TP_ICMP_ATTRS
> +#undef CT_DPIF_TP_ICMP_ATTR
> +    CT_DPIF_TP_ATTR_MAX
> +};
> +
> +struct ct_dpif_timeout_policy {
> +    uint32_t    id;         /* id that uniquely identify a timeout
> policy. */
> +    uint32_t    present;    /* If a timeout attribute is present set the
> +                             * corresponding bit. */

+    uint32_t    attrs[CT_DPIF_TP_ATTR_MAX];     /* An array that specifies
> +                                                 * timeout attribute
> values */
>

I think you can make attrs of type 'int32_t' and use '-1' timeout for 'not
present' and then
remove the 'present' field

+struct ct_dpif_timeout_policy {
> +    uint32_t    id;         /* id that uniquely identify a timeout
> policy. */

+    int32_t    attrs[CT_DPIF_TP_ATTR_MAX];     /* An array that specifies
>
+                                                 * timeout attribute
> values; '-1' for not present. */


Could even use 0 as not present because 0 is is not a sane timeout.
in which case

+struct ct_dpif_timeout_policy {
> +    uint32_t    id;         /* id that uniquely identify a timeout
> policy. */

+    uint32_t    attrs[CT_DPIF_TP_ATTR_MAX];     /* An array that specifies
>
+                                                 * timeout attribute
> values; '0' for not present. */


There are other possibilities as well



> +};
> +
>  int ct_dpif_dump_start(struct dpif *, struct ct_dpif_dump_state **,
>                         const uint16_t *zone, int *);
>  int ct_dpif_dump_next(struct ct_dpif_dump_state *, struct ct_dpif_entry
> *);
> @@ -262,5 +305,15 @@ bool ct_dpif_parse_zone_limit_tuple(const char *s,
> uint16_t *pzone,
>                                      uint32_t *plimit, struct ds *);
>  void ct_dpif_format_zone_limits(uint32_t default_limit,
>                                  const struct ovs_list *, struct ds *);
> +int ct_dpif_add_timeout_policy(struct dpif *dpif, bool is_default,
> +                               const struct ct_dpif_timeout_policy *tp);
> +int ct_dpif_get_timeout_policy(struct dpif *dpif, bool is_default,
> +                               uint32_t tp_id,
> +                               struct ct_dpif_timeout_policy *tp);
> +int ct_dpif_del_timeout_policy(struct dpif *dpif, uint32_t tp_id);
> +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);
>
>  #endif /* CT_DPIF_H */
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index d0a1c58adace..2079e368fb52 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -7529,6 +7529,12 @@ const struct dpif_class dpif_netdev_class = {
>      NULL,                       /* ct_set_limits */
>      NULL,                       /* ct_get_limits */
>      NULL,                       /* ct_del_limits */
> +    NULL,                       /* ct_set_timeout_policy */
> +    NULL,                       /* ct_get_timeout_policy */
> +    NULL,                       /* ct_del_timeout_policy */
> +    NULL,                       /* ct_timeout_policy_dump_start */
> +    NULL,                       /* ct_timeout_policy_dump_next */
> +    NULL,                       /* ct_timeout_policy_dump_done */
>      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 985a284267f5..9825ce46a7f5 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -3434,6 +3434,12 @@ const struct dpif_class dpif_netlink_class = {
>      dpif_netlink_ct_set_limits,
>      dpif_netlink_ct_get_limits,
>      dpif_netlink_ct_del_limits,
> +    NULL,                       /* ct_set_timeout_policy */
> +    NULL,                       /* ct_get_timeout_policy */
> +    NULL,                       /* ct_del_timeout_policy */
> +    NULL,                       /* ct_timeout_policy_dump_start */
> +    NULL,                       /* ct_timeout_policy_dump_next */
> +    NULL,                       /* ct_timeout_policy_dump_done */
>

I found this patch hard to review since it does not contain implementations
The same comment applies to Patch 6
I think Patches 5-7 can be combined into one patch, which will make review
easier.



>      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 12898b9e3c6d..3460ef8aa98d 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -80,6 +80,7 @@ dpif_flow_dump_thread_init(struct dpif_flow_dump_thread
> *thread,
>  struct ct_dpif_dump_state;
>  struct ct_dpif_entry;
>  struct ct_dpif_tuple;
> +struct ct_dpif_timeout_policy;
>
>  /* 'dpif_ipf_proto_status' and 'dpif_ipf_status' are presently in
>   * sync with 'ipf_proto_status' and 'ipf_status', but more
> @@ -498,6 +499,48 @@ struct dpif_class {
>       * list of 'struct ct_dpif_zone_limit' entries. */
>      int (*ct_del_limits)(struct dpif *, const struct ovs_list
> *zone_limits);
>
> +    /* Connection tracking timeout policy */
> +
> +    /* A connection tracking timeout policy contains a list of timeout
> +     * attributes that specifies timeout values on various connection
> states.
> +     * In a datapath, the timeout policy is identified by a 4 bytes
> unsigned
> +     * integer, and the unsupported timeout attributes are ignored.
> +     * When a connection is committed it can be associated with a timeout
> +     * policy, or it defaults to the default timeout policy. */
> +
> +    /* Add timeout policy '*tp' into the datapath.  If 'is_default' is
> true
>

"is_default" - can you explain this one ?



> +     * make the timeout policy to be the default timeout policy. */
> +    int (*ct_add_timeout_policy)(struct dpif *, bool is_default,
> +                                 const struct ct_dpif_timeout_policy *tp);
> +    /* Gets a timeout policy and stores that into '*tp'.




>   If 'is_default' is
> +     * true, sets '*tp' to the default timeout policy.  Otherwise, gets
> the
>

The above text does not make sense:
 "sets" ?

"is_default" - can you explain this one ?



> +     * timeout policy by 'tp_id'. */
> +    int (*ct_get_timeout_policy)(struct dpif *, bool is_default,
> +                                 uint32_t tp_id,
> +                                 struct ct_dpif_timeout_policy *tp);
> +    /* Deletes a timeout policy identified by 'tp_id'. */
> +    int (*ct_del_timeout_policy)(struct dpif *, uint32_t tp_id);
> +
> +    /* Conntrack timeout policy dumping interface.
> +     *
> +     * These functions provide a datapath-agnostic dumping interface
> +     * to the conntrack timeout policy provided by the datapaths.
> +     *
> +     * ct_timeout_policy_dump_start() should put in '*statep' a pointer to
> +     * a newly allocated structure that will be passed by the caller to
> +     * ct_timeout_policy_dump_next() and ct_timeout_policy_dump_done().
> +     *
> +     * ct_timeout_policy_dump_next() fills a timeout policy pointed by
> +     * '*tp' and prepares to dump the next one on a subsequent invocation.
> +     * The caller is responsible to free '*tp'.
> +     *
> +     * ct_timeout_policy_dump_done() should perform any cleanup necessary
> +     * (including deallocating the 'state' structure, if applicable). */
> +    int (*ct_timeout_policy_dump_start)(struct dpif *, void **statep);
> +    int (*ct_timeout_policy_dump_next)(struct dpif *, void *state,
> +                                       struct ct_dpif_timeout_policy
> **tp);
> +    int (*ct_timeout_policy_dump_done)(struct dpif *, void *state);
> +
>      /* IP Fragmentation. */
>
>      /* Disables or enables conntrack fragment reassembly.  The default
> --
> 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