On 16.12.2019 16:10, Eli Britstein wrote:
> Signed-off-by: Eli Britstein <el...@mellanox.com>
> Reviewed-by: Oz Shlomo <o...@mellanox.com>
> ---
> Documentation/howto/dpdk.rst | 1 +
> NEWS | 4 +--
> lib/netdev-offload-dpdk.c | 85
> ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 88 insertions(+), 2 deletions(-)
>
In order to reduce code size and improve readability suggesting following
incremental (based on top of a full patch-set, compile tested only).
Explanations:
1. We're allowed to modify actions here as they are not constant.
So, no need to maintain copy of the mask. tc offload does the same.
2. All build asserts moved out of function to a single place.
3. Macros for repeated code pattern.
4. mask could be obtained as just key + 1.
---
Subject: [PATCH] netdev-offload-dpdk: Refactor set actions.
Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
---
lib/netdev-offload-dpdk.c | 186 +++++++++++---------------------------
1 file changed, 51 insertions(+), 135 deletions(-)
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index d1c066992..02fe551d0 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -848,11 +848,9 @@ add_output_action(struct netdev *netdev,
}
static int
-add_set_flow_action(struct flow_actions *actions,
- const void *value,
- const void *mask,
- const size_t size,
- const int attr)
+add_set_flow_action__(struct flow_actions *actions,
+ const void *value, void *mask,
+ const size_t size, const int attr)
{
void *spec;
@@ -874,179 +872,97 @@ add_set_flow_action(struct flow_actions *actions,
memcpy(spec, value, size);
add_flow_action(actions, attr, spec);
+ /* Clear used mask for later checking. */
+ if (mask) {
+ memset(mask, 0, size);
+ }
return 0;
}
-/* Mask is at the midpoint of the data. */
-#define get_mask(a, type) ((const type *)(const void *)(a + 1) + 1)
+BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_mac) ==
+ MEMBER_SIZEOF(struct ovs_key_ethernet, eth_src));
+BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_mac) ==
+ MEMBER_SIZEOF(struct ovs_key_ethernet, eth_dst));
+BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_ipv4) ==
+ MEMBER_SIZEOF(struct ovs_key_ipv4, ipv4_src));
+BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_ipv4) ==
+ MEMBER_SIZEOF(struct ovs_key_ipv4, ipv4_dst));
+BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_ttl) ==
+ MEMBER_SIZEOF(struct ovs_key_ipv4, ipv4_ttl));
+BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_tp) ==
+ MEMBER_SIZEOF(struct ovs_key_tcp, tcp_src));
+BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_tp) ==
+ MEMBER_SIZEOF(struct ovs_key_tcp, tcp_dst));
+BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_tp) ==
+ MEMBER_SIZEOF(struct ovs_key_udp, udp_src));
+BUILD_ASSERT_DECL(sizeof(struct rte_flow_action_set_tp) ==
+ MEMBER_SIZEOF(struct ovs_key_udp, udp_dst));
static int
parse_set_actions(struct flow_actions *actions,
- const struct nlattr *set_actions,
+ struct nlattr *set_actions,
const size_t set_actions_len,
bool masked)
{
const struct nlattr *sa;
unsigned int sleft;
+#define add_set_flow_action(field, type) \
+ if (add_set_flow_action__(actions, &key->field, \
+ mask ? CONST_CAST(void *, &mask->field) : NULL, \
+ sizeof key->field, type)) { \
+ return -1; \
+ }
+
NL_ATTR_FOR_EACH_UNSAFE (sa, sleft, set_actions, set_actions_len) {
if (nl_attr_type(sa) == OVS_KEY_ATTR_ETHERNET) {
const struct ovs_key_ethernet *key = nl_attr_get(sa);
- const struct ovs_key_ethernet *mask = masked ?
- get_mask(sa, struct ovs_key_ethernet) : NULL;
- struct ovs_key_ethernet consumed_mask;
-
- if (masked) {
- memcpy(&consumed_mask, mask, sizeof consumed_mask);
- } else {
- memset(&consumed_mask, 0, sizeof consumed_mask);
- }
+ const struct ovs_key_ethernet *mask = masked ? key + 1 : NULL;
- BUILD_ASSERT(sizeof(struct rte_flow_action_set_mac) ==
- sizeof key->eth_src);
- if (add_set_flow_action(actions, &key->eth_src,
- mask ? &mask->eth_src : NULL,
- sizeof key->eth_src,
- RTE_FLOW_ACTION_TYPE_SET_MAC_SRC)) {
- return -1;
- }
- memset(&consumed_mask.eth_src, 0, sizeof consumed_mask.eth_src);
-
- BUILD_ASSERT(sizeof(struct rte_flow_action_set_mac) ==
- sizeof key->eth_dst);
- if (add_set_flow_action(actions, &key->eth_dst,
- mask ? &mask->eth_dst : NULL,
- sizeof key->eth_dst,
- RTE_FLOW_ACTION_TYPE_SET_MAC_DST)) {
- return -1;
- }
- memset(&consumed_mask.eth_dst, 0, sizeof consumed_mask.eth_dst);
+ add_set_flow_action(eth_src, RTE_FLOW_ACTION_TYPE_SET_MAC_SRC);
+ add_set_flow_action(eth_dst, RTE_FLOW_ACTION_TYPE_SET_MAC_DST);
- if (!is_all_zeros(&consumed_mask, sizeof consumed_mask)) {
+ if (mask && !is_all_zeros(mask, sizeof *mask)) {
VLOG_DBG_RL(&error_rl, "Unsupported ETHERNET set action");
return -1;
}
} else if (nl_attr_type(sa) == OVS_KEY_ATTR_IPV4) {
const struct ovs_key_ipv4 *key = nl_attr_get(sa);
- const struct ovs_key_ipv4 *mask = masked ?
- get_mask(sa, struct ovs_key_ipv4) : NULL;
- struct ovs_key_ipv4 consumed_mask;
-
- if (masked) {
- memcpy(&consumed_mask, mask, sizeof consumed_mask);
- } else {
- memset(&consumed_mask, 0, sizeof consumed_mask);
- }
+ const struct ovs_key_ipv4 *mask = masked ? key + 1 : NULL;
- BUILD_ASSERT(sizeof(struct rte_flow_action_set_ipv4) ==
- sizeof key->ipv4_src);
- if (add_set_flow_action(actions, &key->ipv4_src,
- mask ? &mask->ipv4_src : NULL,
- sizeof key->ipv4_src,
- RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC)) {
- return -1;
- }
- memset(&consumed_mask.ipv4_src, 0, sizeof consumed_mask.ipv4_src);
-
- BUILD_ASSERT(sizeof(struct rte_flow_action_set_ipv4) ==
- sizeof key->ipv4_dst);
- if (add_set_flow_action(actions, &key->ipv4_dst,
- mask ? &mask->ipv4_dst : NULL,
- sizeof key->ipv4_dst,
- RTE_FLOW_ACTION_TYPE_SET_IPV4_DST)) {
- return -1;
- }
- memset(&consumed_mask.ipv4_dst, 0, sizeof consumed_mask.ipv4_dst);
-
- BUILD_ASSERT(sizeof(struct rte_flow_action_set_ttl) ==
- sizeof key->ipv4_ttl);
- if (add_set_flow_action(actions, &key->ipv4_ttl,
- mask ? &mask->ipv4_ttl : NULL,
- sizeof key->ipv4_ttl,
- RTE_FLOW_ACTION_TYPE_SET_TTL)) {
- return -1;
- }
- memset(&consumed_mask.ipv4_ttl, 0, sizeof consumed_mask.ipv4_ttl);
+ add_set_flow_action(ipv4_src, RTE_FLOW_ACTION_TYPE_SET_IPV4_SRC);
+ add_set_flow_action(ipv4_dst, RTE_FLOW_ACTION_TYPE_SET_IPV4_DST);
+ add_set_flow_action(ipv4_ttl, RTE_FLOW_ACTION_TYPE_SET_TTL);
- if (!is_all_zeros(&consumed_mask, sizeof consumed_mask)) {
+ if (mask && !is_all_zeros(mask, sizeof *mask)) {
VLOG_DBG_RL(&error_rl, "Unsupported IPv4 set action");
return -1;
}
} else if (nl_attr_type(sa) == OVS_KEY_ATTR_TCP) {
const struct ovs_key_tcp *key = nl_attr_get(sa);
- const struct ovs_key_tcp *mask = masked ?
- get_mask(sa, struct ovs_key_tcp) : NULL;
- struct ovs_key_tcp consumed_mask;
-
- if (masked) {
- memcpy(&consumed_mask, mask, sizeof consumed_mask);
- } else {
- memset(&consumed_mask, 0, sizeof consumed_mask);
- }
+ const struct ovs_key_tcp *mask = masked ? key + 1 : NULL;
- BUILD_ASSERT(sizeof(struct rte_flow_action_set_tp) ==
- sizeof key->tcp_src);
- if (add_set_flow_action(actions, &key->tcp_src,
- mask ? &mask->tcp_src : NULL,
- sizeof key->tcp_src,
- RTE_FLOW_ACTION_TYPE_SET_TP_SRC)) {
- return -1;
- }
- memset(&consumed_mask.tcp_src, 0, sizeof consumed_mask.tcp_src);
-
- BUILD_ASSERT(sizeof(struct rte_flow_action_set_tp) ==
- sizeof key->tcp_dst);
- if (add_set_flow_action(actions, &key->tcp_dst,
- mask ? &mask->tcp_dst : NULL,
- sizeof key->tcp_dst,
- RTE_FLOW_ACTION_TYPE_SET_TP_DST)) {
- return -1;
- }
- memset(&consumed_mask.tcp_dst, 0, sizeof consumed_mask.tcp_dst);
+ add_set_flow_action(tcp_src, RTE_FLOW_ACTION_TYPE_SET_TP_SRC);
+ add_set_flow_action(tcp_dst, RTE_FLOW_ACTION_TYPE_SET_TP_DST);
- if (!is_all_zeros(&consumed_mask, sizeof consumed_mask)) {
+ if (mask && !is_all_zeros(mask, sizeof *mask)) {
VLOG_DBG_RL(&error_rl, "Unsupported TCP set action");
return -1;
}
} else if (nl_attr_type(sa) == OVS_KEY_ATTR_UDP) {
const struct ovs_key_udp *key = nl_attr_get(sa);
- const struct ovs_key_udp *mask = masked ?
- get_mask(sa, struct ovs_key_udp) : NULL;
- struct ovs_key_udp consumed_mask;
-
- if (masked) {
- memcpy(&consumed_mask, mask, sizeof consumed_mask);
- } else {
- memset(&consumed_mask, 0, sizeof consumed_mask);
- }
+ const struct ovs_key_udp *mask = masked ? key + 1 : NULL;
- BUILD_ASSERT(sizeof(struct rte_flow_action_set_tp) ==
- sizeof key->udp_src);
- if (add_set_flow_action(actions, &key->udp_src,
- mask ? &mask->udp_src : NULL,
- sizeof key->udp_src,
- RTE_FLOW_ACTION_TYPE_SET_TP_SRC)) {
- return -1;
- }
- memset(&consumed_mask.udp_src, 0, sizeof consumed_mask.udp_src);
-
- BUILD_ASSERT(sizeof(struct rte_flow_action_set_tp) ==
- sizeof key->udp_dst);
- if (add_set_flow_action(actions, &key->udp_dst,
- mask ? &mask->udp_dst : NULL,
- sizeof key->udp_dst,
- RTE_FLOW_ACTION_TYPE_SET_TP_DST)) {
- return -1;
- }
- memset(&consumed_mask.udp_dst, 0, sizeof consumed_mask.udp_dst);
+ add_set_flow_action(udp_src, RTE_FLOW_ACTION_TYPE_SET_TP_SRC);
+ add_set_flow_action(udp_dst, RTE_FLOW_ACTION_TYPE_SET_TP_DST);
- if (!is_all_zeros(&consumed_mask, sizeof consumed_mask)) {
+ if (mask && !is_all_zeros(mask, sizeof *mask)) {
VLOG_DBG_RL(&error_rl, "Unsupported UDP set action");
return -1;
}
} else {
VLOG_DBG_RL(&error_rl,
- "Unsupported set action type=%d", nl_attr_type(sa));
+ "Unsupported set action type %d", nl_attr_type(sa));
return -1;
}
}
--
2.17.1
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
