One more comment inline On Thu, Jul 25, 2019 at 10:02 PM Darrell Ball <dlu...@gmail.com> wrote:
> Thanks for the patch > > Few comments inline > > On Thu, Jul 25, 2019 at 4:30 PM Yi-Hung Wei <yihung....@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....@gmail.com> >> --- >> lib/ct-dpif.c | 10 ++++++++++ >> lib/ct-dpif.h | 3 +++ >> lib/datapath-config.c | 30 ++++++++++++++++++++++++++++++ >> lib/datapath-config.h | 2 ++ >> lib/dpif-netdev.c | 1 + >> lib/dpif-netlink.c | 10 ++++++++++ >> lib/dpif-provider.h | 5 +++++ >> ofproto/ofproto-dpif-xlate.c | 23 +++++++++++++++++++++++ >> 8 files changed, 84 insertions(+) >> >> diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c >> index 1625754e2441..e2c96b29b17f 100644 >> --- a/lib/ct-dpif.c >> +++ b/lib/ct-dpif.c >> @@ -864,3 +864,13 @@ ct_dpif_timeout_policy_dump_done(struct dpif *dpif, >> void *state) >> ? dpif->dpif_class->ct_timeout_policy_dump_done(dpif, state) >> : EOPNOTSUPP); >> } >> + >> +int >> +ct_dpif_format_timeout_policy_name(struct dpif *dpif, uint32_t tp_id, >> + uint16_t dl_type, uint8_t nw_proto, >> + struct ds *ds) >> +{ >> + return (dpif->dpif_class->ct_format_timeout_policy_name >> + ? dpif->dpif_class->ct_format_timeout_policy_name( >> + dpif, tp_id, dl_type, nw_proto, ds) : EOPNOTSUPP); >> +} >> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h >> index de032cc416ce..a4e91f975a9b 100644 >> --- a/lib/ct-dpif.h >> +++ b/lib/ct-dpif.h >> @@ -318,5 +318,8 @@ int ct_dpif_timeout_policy_dump_start(struct dpif >> *dpif, void **statep); >> int ct_dpif_timeout_policy_dump_next(struct dpif *dpif, void *state, >> struct ct_dpif_timeout_policy **tp); >> int ct_dpif_timeout_policy_dump_done(struct dpif *dpif, void *state); >> +int ct_dpif_format_timeout_policy_name(struct dpif *dpif, uint32_t tp_id, >> + uint16_t dl_type, uint8_t >> nw_proto, >> + struct ds *ds); >> >> #endif /* CT_DPIF_H */ >> diff --git a/lib/datapath-config.c b/lib/datapath-config.c >> index cdd2128a60bc..2f3852100b78 100644 >> --- a/lib/datapath-config.c >> +++ b/lib/datapath-config.c >> @@ -377,3 +377,33 @@ destroy_all_datapaths(void) >> datapath_destroy(dp); >> } >> } >> + >> +/* If timeout policy is found in datapath '*dp_type' and in 'zone', >> + * sets timeout policy id in '*tp_id' and returns true. Otherwise, >> + * returns false. */ >> +bool >> +datapath_get_zone_timeout_policy_id(const char *dp_type, uint16_t zone, >> + uint32_t *tp_id) >> +{ >> + struct datapath *dp; >> + struct ct_zone *ct_zone; >> + struct ct_timeout_policy *ct_tp; >> + >> + dp = datapath_lookup(dp_type); >> + if (!dp) { >> + return false; >> + } >> + >> + ct_zone = ct_zone_lookup(&dp->ct_zones, zone); >> + if (!ct_zone) { >> + return false; >> + } >> + >> + ct_tp = ct_timeout_policy_lookup(&dp->ct_tps, &ct_zone->tp_uuid); >> + if (!ct_tp) { >> + return false; >> + } >> + >> + *tp_id = ct_tp->cdtp.id; >> + return true; >> +} >> diff --git a/lib/datapath-config.h b/lib/datapath-config.h >> index d9a90e4f8312..0e0cd7eaad2f 100644 >> --- a/lib/datapath-config.h >> +++ b/lib/datapath-config.h >> @@ -21,5 +21,7 @@ >> void reconfigure_datapath(const struct ovsrec_open_vswitch *, >> unsigned int idl_seqno); >> void destroy_all_datapaths(void); >> +bool datapath_get_zone_timeout_policy_id(const char *dp_type, uint16_t >> zone, >> + uint32_t *tp_id); >> >> #endif /* datapath-config.h */ >> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >> index 7240a3e6f3c8..19cf9f21ec85 100644 >> --- a/lib/dpif-netdev.c >> +++ b/lib/dpif-netdev.c >> @@ -7539,6 +7539,7 @@ const struct dpif_class dpif_netdev_class = { >> NULL, /* ct_timeout_policy_dump_start */ >> NULL, /* ct_timeout_policy_dump_next */ >> NULL, /* ct_timeout_policy_dump_done */ >> + NULL, /* ct_format_timeout_policy_name */ >> dpif_netdev_ipf_set_enabled, >> dpif_netdev_ipf_set_min_frag, >> dpif_netdev_ipf_set_max_nfrags, >> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >> index abfad9543c3b..8e6f2ebb51e1 100644 >> --- a/lib/dpif-netlink.c >> +++ b/lib/dpif-netlink.c >> @@ -3076,6 +3076,15 @@ dpif_netlink_format_tp_name(uint32_t id, uint16_t >> l3num, uint8_t l4num, >> ovs_assert(tp_name->length < CTNL_TIMEOUT_NAME_MAX); >> } >> >> +static int >> +dpif_netlink_ct_format_timeout_policy_name(struct dpif *dpif OVS_UNUSED, >> + uint32_t tp_id, uint16_t dl_type, uint8_t nw_proto, struct ds *ds) >> +{ >> + dpif_netlink_format_tp_name(tp_id, >> + dl_type == ETH_TYPE_IP ? AF_INET : AF_INET6, nw_proto, ds); >> + return 0; >> +} >> + >> #define CT_DPIF_TO_NL_TP_TCP_MAPPINGS \ >> CT_DPIF_TO_NL_TP_MAPPING(TCP, TCP, SYN_SENT, SYN_SENT) \ >> CT_DPIF_TO_NL_TP_MAPPING(TCP, TCP, SYN_RECV, SYN_RECV) \ >> @@ -3860,6 +3869,7 @@ const struct dpif_class dpif_netlink_class = { >> dpif_netlink_ct_timeout_policy_dump_start, >> dpif_netlink_ct_timeout_policy_dump_next, >> dpif_netlink_ct_timeout_policy_dump_done, >> + dpif_netlink_ct_format_timeout_policy_name, >> NULL, /* ipf_set_enabled */ >> NULL, /* ipf_set_min_frag */ >> NULL, /* ipf_set_max_nfrags */ >> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h >> index 3460ef8aa98d..f01f3abee5ab 100644 >> --- a/lib/dpif-provider.h >> +++ b/lib/dpif-provider.h >> @@ -541,6 +541,11 @@ struct dpif_class { >> struct ct_dpif_timeout_policy >> **tp); >> int (*ct_timeout_policy_dump_done)(struct dpif *, void *state); >> >> + /* Get timeout policy name (OVS_CT_ATTR_TIMEOUT) from datapath. */ >> + int (*ct_format_timeout_policy_name)(struct dpif *, uint32_t tp_id, >> + uint16_t dl_type, uint8_t >> nw_proto, >> + struct ds *ds); >> + >> > > The above generic API implies that 'dl_type' and 'nw_proto' are always > needed, which is > not true in general. > I think these parameters could have generic names in this base class. > The corresponding types could be uint32_t as well so as not to imply any > particular context. > > If this is going to delay things, it can be done later from my POV. > > > >> /* IP Fragmentation. */ >> >> /* Disables or enables conntrack fragment reassembly. The default >> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >> index 28a7fdd842a6..12fa9684d0e4 100644 >> --- a/ofproto/ofproto-dpif-xlate.c >> +++ b/ofproto/ofproto-dpif-xlate.c >> @@ -28,10 +28,12 @@ >> #include "bond.h" >> #include "bundle.h" >> #include "byte-order.h" >> +#include "ct-dpif.h" >> #include "cfm.h" >> #include "connmgr.h" >> #include "coverage.h" >> #include "csum.h" >> +#include "datapath-config.h" >> #include "dp-packet.h" >> #include "dpif.h" >> #include "in-band.h" >> @@ -5977,6 +5979,25 @@ put_ct_helper(struct xlate_ctx *ctx, >> } >> >> static void >> +put_ct_timeout(struct ofpbuf *odp_actions, const char *dp_type, >> + struct dpif *dpif, const struct flow *flow, >> + struct flow_wildcards *wc, uint16_t zone_id) >> +{ >> + uint32_t tp_id; >> + >> + if (datapath_get_zone_timeout_policy_id(dp_type, zone_id, &tp_id)) { >> + struct ds ds = DS_EMPTY_INITIALIZER; >> + int err = ct_dpif_format_timeout_policy_name( >> + dpif, tp_id, ntohs(flow->dl_type), flow->nw_proto, >> &ds); >> + if (!err) { >> > > > >> + memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto); >> > > I think the above unwildcarding should be wrapped in a datapath type > specific (i.e. 'system' in this case) > function. > > Also a note should be added to the function describing the why the > additional datapath flows are > needed (i.e. bcoz of Netfilter timeouts implementation). Note that these > datapath flows will remain if kept > minimally active, which is generally undesirable, hence needs to be > flagged. > > If this is going to delay things, it can be done later from my POV. > > > >> + nl_msg_put_string(odp_actions, OVS_CT_ATTR_TIMEOUT, >> ds_cstr(&ds)); >> + } >> + ds_destroy(&ds); >> + } >> +} >> + >> +static void >> put_ct_nat(struct xlate_ctx *ctx) >> { >> struct ofpact_nat *ofn = ctx->ct_nat_action; >> @@ -6071,6 +6092,8 @@ compose_conntrack_action(struct xlate_ctx *ctx, >> struct ofpact_conntrack *ofc, >> put_ct_mark(&ctx->xin->flow, ctx->odp_actions, ctx->wc); >> put_ct_label(&ctx->xin->flow, ctx->odp_actions, ctx->wc); >> put_ct_helper(ctx, ctx->odp_actions, ofc); >> + put_ct_timeout(ctx->odp_actions, ctx->xbridge->ofproto->backer->type, >> + ctx->xbridge->dpif, &ctx->xin->flow, ctx->wc, zone); >> > Let us change: put_ct_timeout(ctx->odp_actions, ctx->xbridge->ofproto->backer->type, ctx->xbridge->dpif, &ctx->xin->flow, ctx->wc, zone); to if (ofc->flags & NX_CT_F_COMMIT) { put_ct_timeout(ctx->odp_actions, ctx->xbridge->ofproto->backer->type, ctx->xbridge->dpif, &ctx->xin->flow, ctx->wc, zone); } since we only need the unwildcarding for 'commit' related flows This will save lots of unnecessary datapath flows. > put_ct_nat(ctx); >> ctx->ct_nat_action = NULL; >> nl_msg_end_nested(ctx->odp_actions, ct_offset); >> -- >> 2.7.4 >> >> _______________________________________________ >> dev mailing list >> d...@openvswitch.org >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev