| Message ID | 1564097054-72663-6-git-send-email-yihung.wei@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Support zone-based conntrack timeout policy | expand |
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@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@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Fri, Jul 26, 2019 at 11:41 AM Darrell Ball <dlu998@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@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@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@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >
On Fri, Jul 26, 2019 at 11:41 AM Darrell Ball <dlu998@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. Thanks Darrell for the review. Sure, I can squash patch 5-7 altogether in v2. >> +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 The timeout value is uint32_t in the kernel, so I will keep it as uint32_t. I find the present flag to be handy when doing conversion from ct-dpif layer to dpif-netlink layer, and as you mentioned in the following e-mail, I will keep it as is. >> --- 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 ? This flag is used to configure the default timeout policy in the datapath. If 'is_default' is true, it will set the provided timeout policy to be the default timeout policy. The default timeout policy is documented in vswitchd/vswitch.xml >> >> + * 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 ? I re-wreite the comment as following. /* Gets a timeout policy and stores that into '*tp'. If 'is_default' is * true, gets default timeout policy. Otherwise, gets the timeout * policy identified by 'tp_id'. */ int (*ct_get_timeout_policy)(struct dpif *, bool is_default, uint32_t tp_id, struct ct_dpif_timeout_policy *tp); Thanks, -Yi-Hung
On Mon, Jul 29, 2019 at 12:37 PM Yi-Hung Wei <yihung.wei@gmail.com> wrote: > On Fri, Jul 26, 2019 at 11:41 AM Darrell Ball <dlu998@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. > > Thanks Darrell for the review. Sure, I can squash patch 5-7 altogether in > v2. > > > >> +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 > > The timeout value is uint32_t in the kernel, so I will keep it as > uint32_t. I find the present flag to be handy when doing conversion > from ct-dpif layer to dpif-netlink layer, and as you mentioned in the > following e-mail, I will keep it as is. > > > >> --- 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 ? > > This flag is used to configure the default timeout policy in the > datapath. If 'is_default' is true, it will set the provided timeout > policy to be the default timeout policy. The default timeout policy is > documented in vswitchd/vswitch.xml > > Below is what I see the the schema xml file under timeout policy Please add description about the 'default' timeout policy under "CT_Timeout_Policy" - let me know if you need help, as I can submit a patch. <table name="CT_Timeout_Policy"> Connection tracking timeout policy configuration <group title="Timeouts"> <column name="timeouts"> The <code>timeouts</code> column contains key-value pairs used to configure connection tracking timeouts in a datapath. Key-value pairs that are not supported by a datapath are ignored. </column> <group title="TCP Timeouts"> <column name="timeouts" key="tcp_syn_sent"> TCP SYN sent timeout. </column> <column name="timeouts" key="tcp_syn_recv"> TCP SYN receive timeout. </column> <column name="timeouts" key="tcp_established"> TCP established timeout. </column> <column name="timeouts" key="tcp_fin_wait"> TCP FIN wait timeout. </column> <column name="timeouts" key="tcp_close_wait"> TCP close wait timeout. </column> <column name="timeouts" key="tcp_last_ack"> TCP last ACK timeout. </column> <column name="timeouts" key="tcp_time_wait"> TCP time wait timeout. </column> <column name="timeouts" key="tcp_close"> TCP close timeout. </column> <column name="timeouts" key="tcp_syn_sent2"> TCP syn sent2 timeout. </column> <column name="timeouts" key="tcp_retransmit"> TCP retransmit timeout. </column> <column name="timeouts" key="tcp_unack"> TCP unacknowledgment timeout. </column> </group> <group title="UDP Timeouts"> <column name="timeouts" key="udp_first"> First UDP packet timeout. </column> <column name="timeouts" key="udp_single"> The timeout in the state that source host sends more than one packet but the destination host has never sent one backs. </column> <column name="timeouts" key="udp_multiple"> UDP packets seen in both directions timeout. </column> </group> <group title="ICMP Timeouts"> <column name="timeouts" key="icmp_first"> First ICMP timeout. </column> <column name="timeouts" key="icmp_reply"> ICMP reply timeout. </column> </group> </group> > >> > >> + * 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 ? > > I re-wreite the comment as following. > /* Gets a timeout policy and stores that into '*tp'. If 'is_default' > is > * true, gets default timeout policy. Otherwise, gets the timeout > * policy identified by 'tp_id'. */ > int (*ct_get_timeout_policy)(struct dpif *, bool is_default, > uint32_t tp_id, > struct ct_dpif_timeout_policy *tp); > > Thanks, > > -Yi-Hung >
On Mon, Jul 29, 2019 at 1:12 PM Darrell Ball <dlu998@gmail.com> wrote: >> > "is_default" - can you explain this one ? >> >> This flag is used to configure the default timeout policy in the >> datapath. If 'is_default' is true, it will set the provided timeout >> policy to be the default timeout policy. The default timeout policy is >> documented in vswitchd/vswitch.xml >> > > Below is what I see the the schema xml file under timeout policy > Please add description about the 'default' timeout policy under > "CT_Timeout_Policy" - let me know if you need help, as I can submit a patch. It is a few lines before what you pasted below. <table name="CT_Zone"> Connection tracking zone configuration <column name="timeout_policy"> Connection tracking timeout policy for this zone. If timeout policy is not specified, defaults to the timeout policy in the default zone. If the timeout policy in default zone is not specified, defaults to the default timeouts in the system. </column> <group title="Common Columns"> The overall purpose of these columns is described under <code>Common Columns</code> at the beginning of this document. <column name="external_ids"/> </group> </table> -Yi-Hung > > <table name="CT_Timeout_Policy"> > Connection tracking timeout policy configuration > > <group title="Timeouts"> > <column name="timeouts"> > The <code>timeouts</code> column contains key-value pairs used > to configure connection tracking timeouts in a datapath. > Key-value pairs that are not supported by a datapath are > ignored. > </column> > > <group title="TCP Timeouts"> > <column name="timeouts" key="tcp_syn_sent"> > TCP SYN sent timeout. > </column> > > <column name="timeouts" key="tcp_syn_recv"> > TCP SYN receive timeout. > </column> > > <column name="timeouts" key="tcp_established"> > TCP established timeout. > </column> > > <column name="timeouts" key="tcp_fin_wait"> > TCP FIN wait timeout. > </column> > > <column name="timeouts" key="tcp_close_wait"> > TCP close wait timeout. > </column> > > <column name="timeouts" key="tcp_last_ack"> > TCP last ACK timeout. > </column> > > <column name="timeouts" key="tcp_time_wait"> > TCP time wait timeout. > </column> > > <column name="timeouts" key="tcp_close"> > TCP close timeout. > </column> > > <column name="timeouts" key="tcp_syn_sent2"> > TCP syn sent2 timeout. > </column> > > <column name="timeouts" key="tcp_retransmit"> > TCP retransmit timeout. > </column> > > <column name="timeouts" key="tcp_unack"> > TCP unacknowledgment timeout. > </column> > </group> > > <group title="UDP Timeouts"> > <column name="timeouts" key="udp_first"> > First UDP packet timeout. > </column> > > <column name="timeouts" key="udp_single"> > The timeout in the state that source host sends more than one packet > but the destination host has never sent one backs. > </column> > > <column name="timeouts" key="udp_multiple"> > UDP packets seen in both directions timeout. > </column> > </group> > > <group title="ICMP Timeouts"> > <column name="timeouts" key="icmp_first"> > First ICMP timeout. > </column> > > <column name="timeouts" key="icmp_reply"> > ICMP reply timeout. > </column> > </group> > </group>
On Mon, Jul 29, 2019 at 3:51 PM Yi-Hung Wei <yihung.wei@gmail.com> wrote: > On Mon, Jul 29, 2019 at 1:12 PM Darrell Ball <dlu998@gmail.com> wrote: > >> > "is_default" - can you explain this one ? > >> > >> This flag is used to configure the default timeout policy in the > >> datapath. If 'is_default' is true, it will set the provided timeout > >> policy to be the default timeout policy. The default timeout policy is > >> documented in vswitchd/vswitch.xml > >> > > > > Below is what I see the the schema xml file under timeout policy > > Please add description about the 'default' timeout policy under > > "CT_Timeout_Policy" - let me know if you need help, as I can submit a > patch. > > It is a few lines before what you pasted below. > > <table name="CT_Zone"> > Connection tracking zone configuration > > <column name="timeout_policy"> > Connection tracking timeout policy for this zone. If timeout policy is > not specified, defaults to the timeout policy in the default zone. If > the timeout policy in default zone is not specified, defaults to the > default timeouts in the system. > </column> > > <group title="Common Columns"> > The overall purpose of these columns is described under <code>Common > Columns</code> at the beginning of this document. > > <column name="external_ids"/> > </group> > </table> > Since we decided to remove the default zone timeout policy defaulting, I think this all becomes moot. > > > -Yi-Hung > > > > <table name="CT_Timeout_Policy"> > > Connection tracking timeout policy configuration > > > > <group title="Timeouts"> > > <column name="timeouts"> > > The <code>timeouts</code> column contains key-value pairs used > > to configure connection tracking timeouts in a datapath. > > Key-value pairs that are not supported by a datapath are > > ignored. > > </column> > > > > <group title="TCP Timeouts"> > > <column name="timeouts" key="tcp_syn_sent"> > > TCP SYN sent timeout. > > </column> > > > > <column name="timeouts" key="tcp_syn_recv"> > > TCP SYN receive timeout. > > </column> > > > > <column name="timeouts" key="tcp_established"> > > TCP established timeout. > > </column> > > > > <column name="timeouts" key="tcp_fin_wait"> > > TCP FIN wait timeout. > > </column> > > > > <column name="timeouts" key="tcp_close_wait"> > > TCP close wait timeout. > > </column> > > > > <column name="timeouts" key="tcp_last_ack"> > > TCP last ACK timeout. > > </column> > > > > <column name="timeouts" key="tcp_time_wait"> > > TCP time wait timeout. > > </column> > > > > <column name="timeouts" key="tcp_close"> > > TCP close timeout. > > </column> > > > > <column name="timeouts" key="tcp_syn_sent2"> > > TCP syn sent2 timeout. > > </column> > > > > <column name="timeouts" key="tcp_retransmit"> > > TCP retransmit timeout. > > </column> > > > > <column name="timeouts" key="tcp_unack"> > > TCP unacknowledgment timeout. > > </column> > > </group> > > > > <group title="UDP Timeouts"> > > <column name="timeouts" key="udp_first"> > > First UDP packet timeout. > > </column> > > > > <column name="timeouts" key="udp_single"> > > The timeout in the state that source host sends more than one > packet > > but the destination host has never sent one backs. > > </column> > > > > <column name="timeouts" key="udp_multiple"> > > UDP packets seen in both directions timeout. > > </column> > > </group> > > > > <group title="ICMP Timeouts"> > > <column name="timeouts" key="icmp_first"> > > First ICMP timeout. > > </column> > > > > <column name="timeouts" key="icmp_reply"> > > ICMP reply timeout. > > </column> > > </group> > > </group> >
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 */ +}; + 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 */ 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 + * 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 + * 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
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@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(+)