[ovs-dev] [PATCH v3 9/9] ofproto-dpif-xlate: Translate timeout policy in ct action
Yi-Hung Wei
yihung.wei at gmail.com
Tue Aug 13 23:29:43 UTC 2019
On Tue, Aug 13, 2019 at 2:33 PM Yi-Hung Wei <yihung.wei at gmail.com> wrote:
>
> On Tue, Aug 13, 2019 at 11:43 AM Darrell Ball <dlu998 at gmail.com> wrote:
> > btw, similarly
> > make 'check-kernel' fails for the same reasons.
> >
> > Ostensibly, I would have expected 5.0 to be ok.
> > I can dig more on this part later if you wish.
>
> The ct timeout feature is introduced in 5.2 kernel, so 'make
> check-kernel' is expected to fail on 5.0 kernel. The upstream kernel
> support for ct timeout feature is documented at
> "Documentation/faq/releases.rst" in the patch 4.
>
>
> > btw, I think a timeout policy not being applied should not result in packet blackholing.
> > I think we need to make this better.
>
> Sure, we can definitely make it better. I am focusing on some other
> issue now, but I will have a follow up patch that only translate the
> ct timeout attribute when the datapath does support that.
>
With the following diff, OVS will translate the ct timeout attribute
depends on whether the datapath support it or not. This shall resolve
the 'make check-kernel' issue on 5.0 kernel.
Thanks,
-Yi-Hung
<-----------------------------------diff-------------------------------------------------------->
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 0b5c56f443e6..4b9a11da221e 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -6085,15 +6085,15 @@ compose_conntrack_action(struct xlate_ctx
*ctx, struct ofpact_conntrack *ofc,
nl_msg_put_u32(ctx->odp_actions, OVS_CT_ATTR_EVENTMASK,
OVS_CT_EVENTMASK_DEFAULT);
}
+ if (ctx->xbridge->support.ct_timeout) {
+ put_ct_timeout(ctx->odp_actions, ctx->xbridge->ofproto->backer,
+ &ctx->xin->flow, ctx->wc, zone);
+ }
}
nl_msg_put_u16(ctx->odp_actions, OVS_CT_ATTR_ZONE, zone);
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,
- &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 8bbc596e2ce0..a5617b589964 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -1319,6 +1319,67 @@ check_ct_clear(struct dpif_backer *backer)
return supported;
}
+/* Tests whether 'backer''s datapath supports the OVS_CT_ATTR_TIMEOUT
+ * attribute in OVS_ACTION_ATTR_CT. */
+static bool
+check_ct_timeout_policy(struct dpif_backer *backer)
+{
+ struct dpif_execute execute;
+ struct dp_packet packet;
+ struct ofpbuf actions;
+ struct flow flow = {
+ .dl_type = CONSTANT_HTONS(ETH_TYPE_IP),
+ .nw_proto = IPPROTO_UDP,
+ .nw_ttl = 64,
+ /* Use the broadcast address on the loopback address range 127/8 to
+ * avoid hitting any real conntrack entries. We leave the UDP ports to
+ * zeroes for the same purpose. */
+ .nw_src = CONSTANT_HTONL(0x7fffffff),
+ .nw_dst = CONSTANT_HTONL(0x7fffffff),
+ };
+ size_t ct_start;
+ int error;
+
+ /* Compose CT action with timeout policy attribute and check if datapath
+ * can decode the message. */
+ ofpbuf_init(&actions, 64);
+ ct_start = nl_msg_start_nested(&actions, OVS_ACTION_ATTR_CT);
+ /* Timeout policy has no effect without the commit flag, but currently the
+ * datapath will accept a timeout policy even without commit. This is
+ * useful as we do not want to persist the probe connection in the
+ * conntrack table. */
+ nl_msg_put_string(&actions, OVS_CT_ATTR_TIMEOUT, "ovs_test_tp");
+ nl_msg_end_nested(&actions, ct_start);
+
+ /* Compose a dummy UDP packet. */
+ dp_packet_init(&packet, 0);
+ flow_compose(&packet, &flow, NULL, 64);
+
+ /* Execute the actions. On older datapaths this fails with EINVAL, on
+ * newer datapaths it succeeds. */
+ execute.actions = actions.data;
+ execute.actions_len = actions.size;
+ execute.packet = &packet;
+ execute.flow = &flow;
+ execute.needs_help = false;
+ execute.probe = true;
+ execute.mtu = 0;
+
+ error = dpif_execute(backer->dpif, &execute);
+
+ dp_packet_uninit(&packet);
+ ofpbuf_uninit(&actions);
+
+ if (error) {
+ VLOG_INFO("%s: Datapath does not support timeout policy in conntrack "
+ "action", dpif_name(backer->dpif));
+ } else {
+ VLOG_INFO("%s: Datapath supports timeout policy in conntrack action",
+ dpif_name(backer->dpif));
+ }
+
+ return !error;
+}
/* Tests whether 'backer''s datapath supports the
* OVS_ACTION_ATTR_CHECK_PKT_LEN action. */
@@ -1469,6 +1530,7 @@ check_support(struct dpif_backer *backer)
backer->rt_support.ct_clear = check_ct_clear(backer);
backer->rt_support.max_hash_alg = check_max_dp_hash_alg(backer);
backer->rt_support.check_pkt_len = check_check_pkt_len(backer);
+ backer->rt_support.ct_timeout = check_ct_timeout_policy(backer);
/* Flow fields. */
backer->rt_support.odp.ct_state = check_ct_state(backer);
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index cce6bdbc842d..7703be9899e6 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -194,8 +194,12 @@ struct group_dpif *group_dpif_lookup(struct ofproto_dpif *,
/* Highest supported dp_hash algorithm. */ \
DPIF_SUPPORT_FIELD(size_t, max_hash_alg, "Max dp_hash algorithm") \
\
- /* True if the datapath supports OVS_ACTION_ATTR_CHECK_PKT_LEN. */ \
- DPIF_SUPPORT_FIELD(bool, check_pkt_len, "Check pkt length action")
+ /* True if the datapath supports OVS_ACTION_ATTR_CHECK_PKT_LEN. */ \
+ DPIF_SUPPORT_FIELD(bool, check_pkt_len, "Check pkt length action") \
+ \
+ /* True if the datapath supports OVS_CT_ATTR_TIMEOUT in \
+ * OVS_ACTION_ATTR_CT action. */ \
+ DPIF_SUPPORT_FIELD(bool, ct_timeout, "Conntrack timeout policy")
/* Stores the various features which the corresponding backer supports. */
struct dpif_backer_support {
<-----------------------------------end of
diff----------------------------------------------->
More information about the dev
mailing list