[ovs-dev] [PATCH 05/12] ct-dpif: Add conntrack timeout policy support in dpif layer
Darrell Ball
dlu998 at gmail.com
Mon Jul 29 06:06:49 UTC 2019
On Fri, Jul 26, 2019 at 11:41 AM Darrell Ball <dlu998 at gmail.com> wrote:
> 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.
>
It appears the kernel is using '0' timeout as 'unlimited' timeout, hence
'0' can't be used as
an empty value with the existing DB schema range specfication
We need to document that, assuming it is not already documented and I just
missed it.
I think we should limit low timeouts, since the margin of error is going to
be high starting at 1 second;
like maybe (100 * n) percent error, so it becomes meaningless to use these
small values.
Maybe start at 10 seconds.
However, this is not that important, relatively speaking.
> 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
>
Lets not worry about the 'present' field comment - there are bigger fish to
fry....
>
>
>> +};
>> +
>> 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