[ovs-dev] [PATCH v5 9/9] ofproto-dpif-xlate: Translate timeout policy in ct action
Justin Pettit
jpettit at ovn.org
Wed Sep 25 22:59:20 UTC 2019
> On Aug 28, 2019, at 3:14 PM, Yi-Hung Wei <yihung.wei at gmail.com> wrote:
>
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index e988626ea05b..4a599c8eb866 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -542,6 +542,16 @@ struct dpif_class {
> struct ct_dpif_timeout_policy *tp);
> int (*ct_timeout_policy_dump_done)(struct dpif *, void *state);
>
> + /* Gets timeout policy based on 'tp_id', 'dl_type' and 'nw_proto'.
> + * On success, returns 0, stores the timeout policy name in 'tp_name',
> + * and sets 'is_generic'. 'is_generic' is false if the returned timeout
> + * policy in the 'dpif' is specific to 'dl_type' and 'nw_proto' in the
> + * datapath, i.e. in kernel datapath. Sets 'is_generic' to true, if
> + * the timeout policy supports all OVS supported L3/L4 protocols. */
> + int (*ct_get_timeout_policy_name)(struct dpif *, uint32_t tp_id,
> + uint16_t dl_type, uint8_t nw_proto,
> + struct ds *tp_name, bool *is_generic);
I don't have a great justification, but we've generally used "char **" instead of "struct ds" in these interfaces. Let me know what you think of the attached incremental. It ended up being a more invasive change than I expected, but I think it will be more consistent in our API.
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 17800f3c8a3f..c8e508bca2c8 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -5983,6 +5983,25 @@ put_ct_helper(struct xlate_ctx *ctx,
> }
>
> static void
> +put_ct_timeout(struct ofpbuf *odp_actions, const struct dpif_backer *backer,
> + const struct flow *flow, struct flow_wildcards *wc,
> + uint16_t zone_id)
> +{
> + struct ds tp_name = DS_EMPTY_INITIALIZER;
> + bool unwildcard;
> +
> + if (ofproto_dpif_ct_zone_timeout_policy_get_name(backer, zone_id,
> + ntohs(flow->dl_type), flow->nw_proto, &tp_name, &unwildcard)) {
> + nl_msg_put_string(odp_actions, OVS_CT_ATTR_TIMEOUT, ds_cstr(&tp_name));
> +
> + if (unwildcard) {
> + memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
I think it's worth mentioning again why we're unwildcarding "nw_proto" and why it wasn't necessary to also unwildcard "dl_type". How about something like the following:
/* The underlying datapath requires separate timeout
* policies for different Ethertypes and IP protocols. We
* don't need to unwildcard 'wc->masks.dl_type' since that
* field is always unwildcarded in megaflows. */
memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
Sorry for the long delay in finishing this review. Assuming you're happy with the changes, I'll merge the series into master.
Thanks,
--Justin
-=-=-=-=-=-=-=-=-=-=-
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c
index 2181c83157ad..23db72dbbbb1 100644
--- a/lib/ct-dpif.c
+++ b/lib/ct-dpif.c
@@ -866,7 +866,7 @@ ct_dpif_timeout_policy_dump_done(struct dpif *dpif, void *state)
int
ct_dpif_get_timeout_policy_name(struct dpif *dpif, uint32_t tp_id,
uint16_t dl_type, uint8_t nw_proto,
- struct ds *tp_name, bool *is_generic)
+ char **tp_name, bool *is_generic)
{
return (dpif->dpif_class->ct_get_timeout_policy_name
? dpif->dpif_class->ct_get_timeout_policy_name(
diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
index b95e6ac762ab..bbcb537ba46d 100644
--- a/lib/ct-dpif.h
+++ b/lib/ct-dpif.h
@@ -320,6 +320,6 @@ int ct_dpif_timeout_policy_dump_next(struct dpif *dpif, void *state,
int ct_dpif_timeout_policy_dump_done(struct dpif *dpif, void *state);
int ct_dpif_get_timeout_policy_name(struct dpif *dpif, uint32_t tp_id,
uint16_t dl_type, uint8_t nw_proto,
- struct ds *tp_name, bool *is_generic);
+ char **tp_name, bool *is_generic);
#endif /* CT_DPIF_H */
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
index 8f1c6d1ffde7..71d9cdbabc5b 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -3056,28 +3056,32 @@ static struct dpif_netlink_timeout_policy_protocol tp_protos[] = {
static void
dpif_netlink_format_tp_name(uint32_t id, uint16_t l3num, uint8_t l4num,
- struct ds *tp_name)
+ char **tp_name)
{
- ds_clear(tp_name);
- ds_put_format(tp_name, "%s%"PRIu32"_", NL_TP_NAME_PREFIX, id);
- ct_dpif_format_ipproto(tp_name, l4num);
+ struct ds ds = DS_EMPTY_INITIALIZER;
+ ds_put_format(&ds, "%s%"PRIu32"_", NL_TP_NAME_PREFIX, id);
+ ct_dpif_format_ipproto(&ds, l4num);
if (l3num == AF_INET) {
- ds_put_cstr(tp_name, "4");
+ ds_put_cstr(&ds, "4");
} else if (l3num == AF_INET6 && l4num != IPPROTO_ICMPV6) {
- ds_put_cstr(tp_name, "6");
+ ds_put_cstr(&ds, "6");
}
- ovs_assert(tp_name->length < CTNL_TIMEOUT_NAME_MAX);
+ ovs_assert(ds.length < CTNL_TIMEOUT_NAME_MAX);
+
+ *tp_name = ds_steal_cstr(&ds);
}
static int
dpif_netlink_ct_get_timeout_policy_name(struct dpif *dpif OVS_UNUSED,
- uint32_t tp_id, uint16_t dl_type, uint8_t nw_proto, struct ds *tp_name,
- bool *is_generic)
+ uint32_t tp_id, uint16_t dl_type,
+ uint8_t nw_proto, char **tp_name,
+ bool *is_generic)
{
dpif_netlink_format_tp_name(tp_id,
- dl_type == ETH_TYPE_IP ? AF_INET : AF_INET6, nw_proto, tp_name);
+ dl_type == ETH_TYPE_IP ? AF_INET : AF_INET6,
+ nw_proto, tp_name);
*is_generic = false;
return 0;
}
@@ -3275,14 +3279,17 @@ static int
dpif_netlink_ct_set_timeout_policy(struct dpif *dpif OVS_UNUSED,
const struct ct_dpif_timeout_policy *tp)
{
- struct nl_ct_timeout_policy nl_tp;
- struct ds nl_tp_name = DS_EMPTY_INITIALIZER;
int err = 0;
for (int i = 0; i < ARRAY_SIZE(tp_protos); ++i) {
+ struct nl_ct_timeout_policy nl_tp;
+ char *nl_tp_name;
+
dpif_netlink_format_tp_name(tp->id, tp_protos[i].l3num,
tp_protos[i].l4num, &nl_tp_name);
- ovs_strlcpy(nl_tp.name, ds_cstr(&nl_tp_name), sizeof nl_tp.name);
+ ovs_strlcpy(nl_tp.name, nl_tp_name, sizeof nl_tp.name);
+ free(nl_tp_name);
+
nl_tp.l3num = tp_protos[i].l3num;
nl_tp.l4num = tp_protos[i].l4num;
dpif_netlink_get_nl_tp_attrs(tp, tp_protos[i].l4num, &nl_tp);
@@ -3295,7 +3302,6 @@ dpif_netlink_ct_set_timeout_policy(struct dpif *dpif OVS_UNUSED,
}
out:
- ds_destroy(&nl_tp_name);
return err;
}
@@ -3304,27 +3310,29 @@ dpif_netlink_ct_get_timeout_policy(struct dpif *dpif OVS_UNUSED,
uint32_t tp_id,
struct ct_dpif_timeout_policy *tp)
{
- struct nl_ct_timeout_policy nl_tp;
- struct ds nl_tp_name = DS_EMPTY_INITIALIZER;
int err = 0;
tp->id = tp_id;
tp->present = 0;
for (int i = 0; i < ARRAY_SIZE(tp_protos); ++i) {
+ struct nl_ct_timeout_policy nl_tp;
+ char *nl_tp_name;
+
dpif_netlink_format_tp_name(tp_id, tp_protos[i].l3num,
tp_protos[i].l4num, &nl_tp_name);
- err = nl_ct_get_timeout_policy(ds_cstr(&nl_tp_name), &nl_tp);
+ err = nl_ct_get_timeout_policy(nl_tp_name, &nl_tp);
if (err) {
VLOG_WARN_RL(&error_rl, "failed to get timeout policy %s (%s)",
- nl_tp.name, ovs_strerror(err));
+ nl_tp_name, ovs_strerror(err));
+ free(nl_tp_name);
goto out;
}
+ free(nl_tp_name);
dpif_netlink_set_ct_dpif_tp_attrs(&nl_tp, tp);
}
out:
- ds_destroy(&nl_tp_name);
return err;
}
@@ -3334,25 +3342,25 @@ static int
dpif_netlink_ct_del_timeout_policy(struct dpif *dpif OVS_UNUSED,
uint32_t tp_id)
{
- struct ds nl_tp_name = DS_EMPTY_INITIALIZER;
int ret = 0;
for (int i = 0; i < ARRAY_SIZE(tp_protos); ++i) {
+ char *nl_tp_name;
dpif_netlink_format_tp_name(tp_id, tp_protos[i].l3num,
tp_protos[i].l4num, &nl_tp_name);
- int err = nl_ct_del_timeout_policy(ds_cstr(&nl_tp_name));
+ int err = nl_ct_del_timeout_policy(nl_tp_name);
if (err == ENOENT) {
err = 0;
}
if (err) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(6, 6);
VLOG_INFO_RL(&rl, "failed to delete timeout policy %s (%s)",
- ds_cstr(&nl_tp_name), ovs_strerror(err));
+ nl_tp_name, ovs_strerror(err));
ret = 1;
}
+ free(nl_tp_name);
}
- ds_destroy(&nl_tp_name);
return ret;
}
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 4a599c8eb866..61feb94e8fca 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -546,11 +546,14 @@ struct dpif_class {
* On success, returns 0, stores the timeout policy name in 'tp_name',
* and sets 'is_generic'. 'is_generic' is false if the returned timeout
* policy in the 'dpif' is specific to 'dl_type' and 'nw_proto' in the
- * datapath, i.e. in kernel datapath. Sets 'is_generic' to true, if
- * the timeout policy supports all OVS supported L3/L4 protocols. */
+ * datapath (e.g., the Linux kernel datapath). Sets 'is_generic' to
+ * true, if the timeout policy supports all OVS supported L3/L4
+ * protocols.
+ *
+ * The caller is responsible for freeing 'tp_name'. */
int (*ct_get_timeout_policy_name)(struct dpif *, uint32_t tp_id,
uint16_t dl_type, uint8_t nw_proto,
- struct ds *tp_name, bool *is_generic);
+ char **tp_name, bool *is_generic);
/* IP Fragmentation. */
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index c8e508bca2c8..a9d5aa754ebc 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5987,18 +5987,22 @@ put_ct_timeout(struct ofpbuf *odp_actions, const struct dpif_backer *backer,
const struct flow *flow, struct flow_wildcards *wc,
uint16_t zone_id)
{
- struct ds tp_name = DS_EMPTY_INITIALIZER;
bool unwildcard;
+ char *tp_name = NULL;
if (ofproto_dpif_ct_zone_timeout_policy_get_name(backer, zone_id,
ntohs(flow->dl_type), flow->nw_proto, &tp_name, &unwildcard)) {
- nl_msg_put_string(odp_actions, OVS_CT_ATTR_TIMEOUT, ds_cstr(&tp_name));
+ nl_msg_put_string(odp_actions, OVS_CT_ATTR_TIMEOUT, tp_name);
if (unwildcard) {
- memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
+ /* The underlying datapath requires separate timeout
+ * policies for different Ethertypes and IP protocols. We
+ * don't need to unwildcard 'wc->masks.dl_type' since that
+ * field is always unwildcarded in megaflows. */
+ memset(&wc->masks.nw_proto, 0xff, sizeof wc->masks.nw_proto);
}
}
- ds_destroy(&tp_name);
+ free(tp_name);
}
static void
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 53c3515d15db..496a16c8a4af 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5441,19 +5441,19 @@ ct_del_zone_timeout_policy(const char *datapath_type, uint16_t zone_id)
}
/* Gets timeout policy name in 'backer' based on 'zone', 'dl_type' and
- * 'nw_proto'. Returns true if the zoned-based timeout policy is configured.
+ * 'nw_proto'. Returns true if the zone-based timeout policy is configured.
* On success, stores the timeout policy name in 'tp_name', and sets
* 'unwildcard' based on the dpif implementation. If 'unwildcard' is true,
* the returned timeout policy is 'dl_type' and 'nw_proto' specific, and OVS
* needs to unwildcard the datapath flow for this timeout policy in flow
- * translation. */
+ * translation.
+ *
+ * The caller is responsible for freeing 'tp_name'. */
bool
ofproto_dpif_ct_zone_timeout_policy_get_name(
const struct dpif_backer *backer, uint16_t zone, uint16_t dl_type,
- uint8_t nw_proto, struct ds *tp_name, bool *unwildcard)
+ uint8_t nw_proto, char **tp_name, bool *unwildcard)
{
- bool is_generic_tp;
-
if (!ct_dpif_timeout_policy_support_ipproto(nw_proto)) {
return false;
}
@@ -5463,14 +5463,15 @@ ofproto_dpif_ct_zone_timeout_policy_get_name(
return false;
}
+ bool is_generic;
if (ct_dpif_get_timeout_policy_name(backer->dpif,
- ct_zone->ct_tp->tp_id, dl_type,
- nw_proto, tp_name, &is_generic_tp)) {
+ ct_zone->ct_tp->tp_id, dl_type,
+ nw_proto, tp_name, &is_generic)) {
return false;
}
/* Unwildcard datapath flow if it is not a generic timeout policy. */
- *unwildcard = !is_generic_tp;
+ *unwildcard = !is_generic;
return true;
}
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index 7703be9899e6..cd453636c920 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -380,6 +380,6 @@ bool ovs_native_tunneling_is_on(struct ofproto_dpif *);
bool ofproto_dpif_ct_zone_timeout_policy_get_name(
const struct dpif_backer *backer, uint16_t zone, uint16_t dl_type,
- uint8_t nw_proto, struct ds *tp_name, bool *unwildcard);
+ uint8_t nw_proto, char **tp_name, bool *unwildcard);
#endif /* ofproto-dpif.h */
More information about the dev
mailing list