| Message ID | 1564697253-37992-9-git-send-email-yihung.wei@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support zone-based conntrack timeout policy | expand |
Thanks for the patch
The main comment I had from the V1 patch was adding the check
+ if (ofc->flags & NX_CT_F_COMMIT) {
in compose_conntrack_action()
I see that was done.
After a quick scan, I had one minor comment inline.
On Thu, Aug 1, 2019 at 3:12 PM Yi-Hung Wei <yihung.wei@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@gmail.com>
> ---
> lib/ct-dpif.c | 10 ++++++++++
> lib/ct-dpif.h | 3 +++
> lib/dpif-netdev.c | 1 +
> lib/dpif-netlink.c | 10 ++++++++++
> lib/dpif-provider.h | 5 +++++
> ofproto/ofproto-dpif-xlate.c | 29 +++++++++++++++++++++++++++++
> ofproto/ofproto-dpif.c | 24 ++++++++++++++++++++++++
> ofproto/ofproto-provider.h | 5 +++++
> ofproto/ofproto.c | 11 +++++++++++
> ofproto/ofproto.h | 2 ++
> 10 files changed, 100 insertions(+)
>
> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
> index 7f9ce0a561f7..5d2acfd7810b 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 8dacb1c7c253..0a27568880c0 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/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 b859508f718a..92da87027c58 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -3071,6 +3071,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_NL_TP_TCP_MAPPINGS \
> CT_DPIF_NL_TP_MAPPING(TCP, TCP, SYN_SENT, SYN_SENT) \
> CT_DPIF_NL_TP_MAPPING(TCP, TCP, SYN_RECV, SYN_RECV) \
> @@ -3891,6 +3900,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 79a2314500cf..57b32ccb610f 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -536,6 +536,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);
> +
> /* 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..f9b517aaa270 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -28,6 +28,7 @@
> #include "bond.h"
> #include "bundle.h"
> #include "byte-order.h"
> +#include "ct-dpif.h"
>
move further down
> #include "cfm.h"
> #include "connmgr.h"
> #include "coverage.h"
> @@ -5977,6 +5978,30 @@ put_ct_helper(struct xlate_ctx *ctx,
> }
>
> static void
> +put_ct_timeout(struct ofpbuf *odp_actions, const char *dp_type,
> + const struct ofproto_dpif *ofproto, const struct flow
> *flow,
> + struct flow_wildcards *wc, uint16_t zone_id)
> +{
> + struct dpif_backer *backer = ofproto->backer;
> + uint32_t tp_id;
> +
> + if (ofproto_ct_zone_timeout_policy_get(&ofproto->up, zone_id,
> &tp_id)) {
> + if (!strcmp(dp_type, "system")) {
> + struct ds ds = DS_EMPTY_INITIALIZER;
> + int err = ct_dpif_format_timeout_policy_name(
> + backer->dpif, tp_id, ntohs(flow->dl_type),
> + flow->nw_proto, &ds);
> + if (!err) {
> + memset(&wc->masks.nw_proto, 0xff, sizeof
> wc->masks.nw_proto);
> + 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 +6096,10 @@ 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);
> + if (ofc->flags & NX_CT_F_COMMIT) {
> + put_ct_timeout(ctx->odp_actions,
> ctx->xbridge->ofproto->backer->type,
> + ctx->xbridge->ofproto, &ctx->xin->flow, ctx->wc,
> zone);
> + }
> put_ct_nat(ctx);
> ctx->ct_nat_action = NULL;
> nl_msg_end_nested(ctx->odp_actions, ct_offset);
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 6336494e0bc8..f31162f4481d 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -5351,6 +5351,29 @@ ct_zone_timeout_policy_reconfig(const struct
> ofproto *ofproto,
> }
>
> static bool
> +ct_zone_timeout_policy_get(const struct ofproto *ofproto_,
> + uint16_t zone, uint32_t *tp_id)
> +{
> + struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
> + struct dpif_backer *backer = ofproto->backer;
> + struct ct_zone *ct_zone;
> + struct ct_timeout_policy *ct_tp;
> +
> + ct_zone = ct_zone_lookup(&backer->ct_zones, zone);
> + if (!ct_zone) {
> + return false;
> + }
> +
> + ct_tp = ct_timeout_policy_lookup(&backer->ct_tps, &ct_zone->tp_uuid);
> + if (!ct_tp) {
> + return false;
> + }
> +
> + *tp_id = ct_tp->cdtp.id;
> + return true;
> +}
> +
> +static bool
> set_frag_handling(struct ofproto *ofproto_,
> enum ofputil_frag_handling frag_handling)
> {
> @@ -6455,4 +6478,5 @@ const struct ofproto_class ofproto_dpif_class = {
> ct_flush, /* ct_flush */
> ct_zone_timeout_policy_reconfig,
> ct_zone_timeout_policy_sweep,
> + ct_zone_timeout_policy_get,
> };
> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
> index 41e07f0ee23e..1a2fc4a6a084 100644
> --- a/ofproto/ofproto-provider.h
> +++ b/ofproto/ofproto-provider.h
> @@ -1880,6 +1880,11 @@ struct ofproto_class {
> const struct ovsrec_datapath *dp_cfg, unsigned int idl_seqno);
> /* Cleans up the to be deleted timeout policy in the pending kill
> list. */
> void (*ct_zone_timeout_policy_sweep)(const struct ofproto *ofproto_);
> +
> + /* Returns true if timeout policy for 'zone' is configured and stores
> the
> + * timeout policy id in '*tp_id'. */
> + bool (*ct_zone_timeout_policy_get)(const struct ofproto *ofproto_,
> + uint16_t zone, uint32_t *tp_id);
> };
>
> extern const struct ofproto_class ofproto_dpif_class;
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 373b8a4eba0c..cef0690cf466 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -955,6 +955,17 @@ ofproto_ct_zone_timeout_policy_sweep(const struct
> ofproto *ofproto)
> }
> }
>
> +bool
> +ofproto_ct_zone_timeout_policy_get(const struct ofproto *ofproto,
> + uint16_t zone, uint32_t *tp_id)
> +{
> + if (ofproto->ofproto_class->ct_zone_timeout_policy_get) {
> + return ofproto->ofproto_class->ct_zone_timeout_policy_get(
> + ofproto, zone, tp_id);
> + }
> + return false;
> +}
> +
>
> /* Spanning Tree Protocol (STP) configuration. */
>
> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h
> index 2ae42374be36..8749cabdb7bd 100644
> --- a/ofproto/ofproto.h
> +++ b/ofproto/ofproto.h
> @@ -366,6 +366,8 @@ void ofproto_set_vlan_limit(int vlan_limit);
> void ofproto_ct_zone_timeout_policy_reconfig(const struct ofproto
> *ofproto,
> const struct ovsrec_datapath *dp_cfg, unsigned int idl_seqno);
> void ofproto_ct_zone_timeout_policy_sweep(const struct ofproto *ofproto);
> +bool ofproto_ct_zone_timeout_policy_get(const struct ofproto *ofproto,
> + uint16_t zone, uint32_t *tp_id);
>
> /* Configuration of ports. */
> void ofproto_port_unregister(struct ofproto *, ofp_port_t ofp_port);
> --
> 2.7.4
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
On Mon, Aug 5, 2019 at 8:51 PM Darrell Ball <dlu998@gmail.com> wrote: > Thanks for the patch > > The main comment I had from the V1 patch was adding the check > > + if (ofc->flags & NX_CT_F_COMMIT) { > > in compose_conntrack_action() > > I see that was done. > > After a quick scan, I had one minor comment inline. > I wanted to reiterate one more comment that I added to V1 regarding the unwildcarding (inline). > > On Thu, Aug 1, 2019 at 3:12 PM Yi-Hung Wei <yihung.wei@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@gmail.com> >> --- >> lib/ct-dpif.c | 10 ++++++++++ >> lib/ct-dpif.h | 3 +++ >> lib/dpif-netdev.c | 1 + >> lib/dpif-netlink.c | 10 ++++++++++ >> lib/dpif-provider.h | 5 +++++ >> ofproto/ofproto-dpif-xlate.c | 29 +++++++++++++++++++++++++++++ >> ofproto/ofproto-dpif.c | 24 ++++++++++++++++++++++++ >> ofproto/ofproto-provider.h | 5 +++++ >> ofproto/ofproto.c | 11 +++++++++++ >> ofproto/ofproto.h | 2 ++ >> 10 files changed, 100 insertions(+) >> >> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c >> index 7f9ce0a561f7..5d2acfd7810b 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 8dacb1c7c253..0a27568880c0 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/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 b859508f718a..92da87027c58 100644 >> --- a/lib/dpif-netlink.c >> +++ b/lib/dpif-netlink.c >> @@ -3071,6 +3071,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_NL_TP_TCP_MAPPINGS \ >> CT_DPIF_NL_TP_MAPPING(TCP, TCP, SYN_SENT, SYN_SENT) \ >> CT_DPIF_NL_TP_MAPPING(TCP, TCP, SYN_RECV, SYN_RECV) \ >> @@ -3891,6 +3900,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 79a2314500cf..57b32ccb610f 100644 >> --- a/lib/dpif-provider.h >> +++ b/lib/dpif-provider.h >> @@ -536,6 +536,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); >> + >> /* 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..f9b517aaa270 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -28,6 +28,7 @@ >> #include "bond.h" >> #include "bundle.h" >> #include "byte-order.h" >> +#include "ct-dpif.h" >> > > move further down > > >> #include "cfm.h" >> #include "connmgr.h" >> #include "coverage.h" >> @@ -5977,6 +5978,30 @@ put_ct_helper(struct xlate_ctx *ctx, >> } >> >> static void >> +put_ct_timeout(struct ofpbuf *odp_actions, const char *dp_type, >> + const struct ofproto_dpif *ofproto, const struct flow >> *flow, >> + struct flow_wildcards *wc, uint16_t zone_id) >> +{ >> + struct dpif_backer *backer = ofproto->backer; >> + uint32_t tp_id; >> + >> + if (ofproto_ct_zone_timeout_policy_get(&ofproto->up, zone_id, >> &tp_id)) { >> + if (!strcmp(dp_type, "system")) { >> + struct ds ds = DS_EMPTY_INITIALIZER; >> + int err = ct_dpif_format_timeout_policy_name( >> + backer->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 function in dpif-netlink or netlink-conntrack. This is bcoz, the userspace datapath does not need to do this unwildcarding since a timeout profile can be shared across dl_type and ip_proto. 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 +6096,10 @@ 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); >> + if (ofc->flags & NX_CT_F_COMMIT) { >> + put_ct_timeout(ctx->odp_actions, >> ctx->xbridge->ofproto->backer->type, >> + ctx->xbridge->ofproto, &ctx->xin->flow, ctx->wc, >> zone); >> + } >> put_ct_nat(ctx); >> ctx->ct_nat_action = NULL; >> nl_msg_end_nested(ctx->odp_actions, ct_offset); >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >> index 6336494e0bc8..f31162f4481d 100644 >> --- a/ofproto/ofproto-dpif.c >> +++ b/ofproto/ofproto-dpif.c >> @@ -5351,6 +5351,29 @@ ct_zone_timeout_policy_reconfig(const struct >> ofproto *ofproto, >> } >> >> static bool >> +ct_zone_timeout_policy_get(const struct ofproto *ofproto_, >> + uint16_t zone, uint32_t *tp_id) >> +{ >> + struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); >> + struct dpif_backer *backer = ofproto->backer; >> + struct ct_zone *ct_zone; >> + struct ct_timeout_policy *ct_tp; >> + >> + ct_zone = ct_zone_lookup(&backer->ct_zones, zone); >> + if (!ct_zone) { >> + return false; >> + } >> + >> + ct_tp = ct_timeout_policy_lookup(&backer->ct_tps, &ct_zone->tp_uuid); >> + if (!ct_tp) { >> + return false; >> + } >> + >> + *tp_id = ct_tp->cdtp.id; >> + return true; >> +} >> + >> +static bool >> set_frag_handling(struct ofproto *ofproto_, >> enum ofputil_frag_handling frag_handling) >> { >> @@ -6455,4 +6478,5 @@ const struct ofproto_class ofproto_dpif_class = { >> ct_flush, /* ct_flush */ >> ct_zone_timeout_policy_reconfig, >> ct_zone_timeout_policy_sweep, >> + ct_zone_timeout_policy_get, >> }; >> diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h >> index 41e07f0ee23e..1a2fc4a6a084 100644 >> --- a/ofproto/ofproto-provider.h >> +++ b/ofproto/ofproto-provider.h >> @@ -1880,6 +1880,11 @@ struct ofproto_class { >> const struct ovsrec_datapath *dp_cfg, unsigned int >> idl_seqno); >> /* Cleans up the to be deleted timeout policy in the pending kill >> list. */ >> void (*ct_zone_timeout_policy_sweep)(const struct ofproto *ofproto_); >> + >> + /* Returns true if timeout policy for 'zone' is configured and >> stores the >> + * timeout policy id in '*tp_id'. */ >> + bool (*ct_zone_timeout_policy_get)(const struct ofproto *ofproto_, >> + uint16_t zone, uint32_t *tp_id); >> }; >> >> extern const struct ofproto_class ofproto_dpif_class; >> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c >> index 373b8a4eba0c..cef0690cf466 100644 >> --- a/ofproto/ofproto.c >> +++ b/ofproto/ofproto.c >> @@ -955,6 +955,17 @@ ofproto_ct_zone_timeout_policy_sweep(const struct >> ofproto *ofproto) >> } >> } >> >> +bool >> +ofproto_ct_zone_timeout_policy_get(const struct ofproto *ofproto, >> + uint16_t zone, uint32_t *tp_id) >> +{ >> + if (ofproto->ofproto_class->ct_zone_timeout_policy_get) { >> + return ofproto->ofproto_class->ct_zone_timeout_policy_get( >> + ofproto, zone, tp_id); >> + } >> + return false; >> +} >> + >> >> /* Spanning Tree Protocol (STP) configuration. */ >> >> diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h >> index 2ae42374be36..8749cabdb7bd 100644 >> --- a/ofproto/ofproto.h >> +++ b/ofproto/ofproto.h >> @@ -366,6 +366,8 @@ void ofproto_set_vlan_limit(int vlan_limit); >> void ofproto_ct_zone_timeout_policy_reconfig(const struct ofproto >> *ofproto, >> const struct ovsrec_datapath *dp_cfg, unsigned int idl_seqno); >> void ofproto_ct_zone_timeout_policy_sweep(const struct ofproto *ofproto); >> +bool ofproto_ct_zone_timeout_policy_get(const struct ofproto *ofproto, >> + uint16_t zone, uint32_t *tp_id); >> >> /* Configuration of ports. */ >> void ofproto_port_unregister(struct ofproto *, ofp_port_t ofp_port); >> -- >> 2.7.4 >> >> _______________________________________________ >> dev mailing list >> dev@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >
> On Aug 1, 2019, at 3:07 PM, Yi-Hung Wei <yihung.wei@gmail.com> wrote: > > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h > index 79a2314500cf..57b32ccb610f 100644 > --- a/lib/dpif-provider.h > +++ b/lib/dpif-provider.h > @@ -536,6 +536,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); To Darrell's point about wanting the ability to implement timeout policies in the userspace implementation without unwildcarding the dl_type and nw_proto, I'd suggest adding a "bool *unwildcard" argument that the provider can indicate whether those bits should be unwildcarded or not. > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > index 41e07f0ee23e..1a2fc4a6a084 100644 > --- a/ofproto/ofproto-provider.h > +++ b/ofproto/ofproto-provider.h > @@ -1880,6 +1880,11 @@ struct ofproto_class { > const struct ovsrec_datapath *dp_cfg, unsigned int idl_seqno); > /* Cleans up the to be deleted timeout policy in the pending kill list. */ > void (*ct_zone_timeout_policy_sweep)(const struct ofproto *ofproto_); > + > + /* Returns true if timeout policy for 'zone' is configured and stores the > + * timeout policy id in '*tp_id'. */ > + bool (*ct_zone_timeout_policy_get)(const struct ofproto *ofproto_, > + uint16_t zone, uint32_t *tp_id); > }; As we discuss off-line, by pulling the OVSDB information out of the dpif layer, I think we'll want a more tradition get/set ofproto interface. I believe that will be done in v3. Thanks, --Justin
On Fri, Aug 9, 2019 at 1:10 PM Justin Pettit <jpettit@ovn.org> wrote: > > > On Aug 1, 2019, at 3:07 PM, Yi-Hung Wei <yihung.wei@gmail.com> wrote: > > > > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h > > index 79a2314500cf..57b32ccb610f 100644 > > --- a/lib/dpif-provider.h > > +++ b/lib/dpif-provider.h > > @@ -536,6 +536,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); > > To Darrell's point about wanting the ability to implement timeout policies > in the userspace implementation without unwildcarding the dl_type and > nw_proto, I'd suggest adding a "bool *unwildcard" argument that the > provider can indicate whether those bits should be unwildcarded or not. > Sounds good A related point is that some timeout profile expansion supporting code is presently located in common code in ct-dpif.* This is the code that helps convert a profile from the database form and expands it to 6 Netfilter profiles - (TCP4, TCP6, UDP4, UDP6, ICMP4, ICMP6). I did mention this in some other comments, but I wanted to reiterate to save some of the further churn. One other thing that concerns me is that datapath rules referencing timeout profiles can linger in the fast path after the associated timeout profiles are supposed to be gone (eg) short timers/ct-flush). Although, presently, the communication of timeout profiles via datapath rules is needed for Netfilter support, this lingering of timeout profile context is not ideal. It would be good to document it. The above being said, I do think that for userspace datapath timeout profiles could be communicated more simply to userspace conntrack. After reading ct-zone/timeout profile info from the database, the ct-zone/timeout profile delta can be sent via _type_set_config() API to the userspace datapath/conntrack. conntrack can associate timeout profiles to ct/commit related rules via ct-zone. This would also satisfy the present requirements. This would mean the timeout profile specifiers would not need to be directly associated with datapath rules. > > > diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h > > index 41e07f0ee23e..1a2fc4a6a084 100644 > > --- a/ofproto/ofproto-provider.h > > +++ b/ofproto/ofproto-provider.h > > @@ -1880,6 +1880,11 @@ struct ofproto_class { > > const struct ovsrec_datapath *dp_cfg, unsigned int > idl_seqno); > > /* Cleans up the to be deleted timeout policy in the pending kill > list. */ > > void (*ct_zone_timeout_policy_sweep)(const struct ofproto *ofproto_); > > + > > + /* Returns true if timeout policy for 'zone' is configured and > stores the > > + * timeout policy id in '*tp_id'. */ > > + bool (*ct_zone_timeout_policy_get)(const struct ofproto *ofproto_, > > + uint16_t zone, uint32_t *tp_id); > > }; > > As we discuss off-line, by pulling the OVSDB information out of the dpif > layer, I think we'll want a more tradition get/set ofproto interface. I > believe that will be done in v3. > > Thanks, > > --Justin > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c index 7f9ce0a561f7..5d2acfd7810b 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 8dacb1c7c253..0a27568880c0 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/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 b859508f718a..92da87027c58 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -3071,6 +3071,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_NL_TP_TCP_MAPPINGS \ CT_DPIF_NL_TP_MAPPING(TCP, TCP, SYN_SENT, SYN_SENT) \ CT_DPIF_NL_TP_MAPPING(TCP, TCP, SYN_RECV, SYN_RECV) \ @@ -3891,6 +3900,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 79a2314500cf..57b32ccb610f 100644 --- a/lib/dpif-provider.h +++ b/lib/dpif-provider.h @@ -536,6 +536,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); + /* 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..f9b517aaa270 100644 --- a/ofproto/ofproto-dpif-xlate.c +++ b/ofproto/ofproto-dpif-xlate.c @@ -28,6 +28,7 @@ #include "bond.h" #include "bundle.h" #include "byte-order.h" +#include "ct-dpif.h" #include "cfm.h" #include "connmgr.h" #include "coverage.h" @@ -5977,6 +5978,30 @@ put_ct_helper(struct xlate_ctx *ctx, } static void +put_ct_timeout(struct ofpbuf *odp_actions, const char *dp_type, + const struct ofproto_dpif *ofproto, const struct flow *flow, + struct flow_wildcards *wc, uint16_t zone_id) +{ + struct dpif_backer *backer = ofproto->backer; + uint32_t tp_id; + + if (ofproto_ct_zone_timeout_policy_get(&ofproto->up, zone_id, &tp_id)) { + if (!strcmp(dp_type, "system")) { + struct ds ds = DS_EMPTY_INITIALIZER; + int err = ct_dpif_format_timeout_policy_name( + backer->dpif, tp_id, ntohs(flow->dl_type), + flow->nw_proto, &ds); + if (!err) { + memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto); + 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 +6096,10 @@ 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); + if (ofc->flags & NX_CT_F_COMMIT) { + put_ct_timeout(ctx->odp_actions, ctx->xbridge->ofproto->backer->type, + ctx->xbridge->ofproto, &ctx->xin->flow, ctx->wc, zone); + } put_ct_nat(ctx); ctx->ct_nat_action = NULL; nl_msg_end_nested(ctx->odp_actions, ct_offset); diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 6336494e0bc8..f31162f4481d 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -5351,6 +5351,29 @@ ct_zone_timeout_policy_reconfig(const struct ofproto *ofproto, } static bool +ct_zone_timeout_policy_get(const struct ofproto *ofproto_, + uint16_t zone, uint32_t *tp_id) +{ + struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_); + struct dpif_backer *backer = ofproto->backer; + struct ct_zone *ct_zone; + struct ct_timeout_policy *ct_tp; + + ct_zone = ct_zone_lookup(&backer->ct_zones, zone); + if (!ct_zone) { + return false; + } + + ct_tp = ct_timeout_policy_lookup(&backer->ct_tps, &ct_zone->tp_uuid); + if (!ct_tp) { + return false; + } + + *tp_id = ct_tp->cdtp.id; + return true; +} + +static bool set_frag_handling(struct ofproto *ofproto_, enum ofputil_frag_handling frag_handling) { @@ -6455,4 +6478,5 @@ const struct ofproto_class ofproto_dpif_class = { ct_flush, /* ct_flush */ ct_zone_timeout_policy_reconfig, ct_zone_timeout_policy_sweep, + ct_zone_timeout_policy_get, }; diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h index 41e07f0ee23e..1a2fc4a6a084 100644 --- a/ofproto/ofproto-provider.h +++ b/ofproto/ofproto-provider.h @@ -1880,6 +1880,11 @@ struct ofproto_class { const struct ovsrec_datapath *dp_cfg, unsigned int idl_seqno); /* Cleans up the to be deleted timeout policy in the pending kill list. */ void (*ct_zone_timeout_policy_sweep)(const struct ofproto *ofproto_); + + /* Returns true if timeout policy for 'zone' is configured and stores the + * timeout policy id in '*tp_id'. */ + bool (*ct_zone_timeout_policy_get)(const struct ofproto *ofproto_, + uint16_t zone, uint32_t *tp_id); }; extern const struct ofproto_class ofproto_dpif_class; diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c index 373b8a4eba0c..cef0690cf466 100644 --- a/ofproto/ofproto.c +++ b/ofproto/ofproto.c @@ -955,6 +955,17 @@ ofproto_ct_zone_timeout_policy_sweep(const struct ofproto *ofproto) } } +bool +ofproto_ct_zone_timeout_policy_get(const struct ofproto *ofproto, + uint16_t zone, uint32_t *tp_id) +{ + if (ofproto->ofproto_class->ct_zone_timeout_policy_get) { + return ofproto->ofproto_class->ct_zone_timeout_policy_get( + ofproto, zone, tp_id); + } + return false; +} + /* Spanning Tree Protocol (STP) configuration. */ diff --git a/ofproto/ofproto.h b/ofproto/ofproto.h index 2ae42374be36..8749cabdb7bd 100644 --- a/ofproto/ofproto.h +++ b/ofproto/ofproto.h @@ -366,6 +366,8 @@ void ofproto_set_vlan_limit(int vlan_limit); void ofproto_ct_zone_timeout_policy_reconfig(const struct ofproto *ofproto, const struct ovsrec_datapath *dp_cfg, unsigned int idl_seqno); void ofproto_ct_zone_timeout_policy_sweep(const struct ofproto *ofproto); +bool ofproto_ct_zone_timeout_policy_get(const struct ofproto *ofproto, + uint16_t zone, uint32_t *tp_id); /* Configuration of ports. */ void ofproto_port_unregister(struct ofproto *, ofp_port_t ofp_port);
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@gmail.com> --- lib/ct-dpif.c | 10 ++++++++++ lib/ct-dpif.h | 3 +++ lib/dpif-netdev.c | 1 + lib/dpif-netlink.c | 10 ++++++++++ lib/dpif-provider.h | 5 +++++ ofproto/ofproto-dpif-xlate.c | 29 +++++++++++++++++++++++++++++ ofproto/ofproto-dpif.c | 24 ++++++++++++++++++++++++ ofproto/ofproto-provider.h | 5 +++++ ofproto/ofproto.c | 11 +++++++++++ ofproto/ofproto.h | 2 ++ 10 files changed, 100 insertions(+)