On Wed, Apr 3, 2019 at 7:19 AM Roi Dayan <r...@mellanox.com> wrote: > > > > On 02/04/2019 20:14, John Hurley wrote: > > On Tue, Apr 2, 2019 at 4:05 PM Roi Dayan <r...@mellanox.com> wrote: > >> > >> > >> > >> On 02/04/2019 12:27, John Hurley wrote: > >>> The TC datapath only permits the offload of mirr actions if they are > >>> egress. To offload TC actions that output to OvS internal ports, ingress > >>> mirr actions are required. At the TC layer, an ingress mirr action passes > >>> the packet back into the network stack as if it came in the action port > >>> rather than attempting to egress the port. > >>> > >>> Update OvS-TC offloads to support ingress mirr actions. To ensure packets > >>> that match these rules are properly passed into the network stack, add a > >>> TC skbedit action along with ingress mirr that sets the pkt_type to > >>> PACKET_HOST. This mirrors the functionality of the OvS internal port > >>> kernel module. > >>> > >>> Signed-off-by: John Hurley <john.hur...@netronome.com> > >>> Reviewed-by: Simon Horman <simon.hor...@netronome.com> > >>> --- > >>> lib/netdev-tc-offloads.c | 15 +++++++++--- > >>> lib/tc.c | 62 > >>> ++++++++++++++++++++++++++++++++++++++++++------ > >>> lib/tc.h | 5 +++- > >>> 3 files changed, 71 insertions(+), 11 deletions(-) > >>> > >>> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c > >>> index f5555e4..78ad023 100644 > >>> --- a/lib/netdev-tc-offloads.c > >>> +++ b/lib/netdev-tc-offloads.c > >>> @@ -1,3 +1,4 @@ > >>> + > >>> /* > >>> * Copyright (c) 2016 Mellanox Technologies, Ltd. > >>> * > >>> @@ -52,6 +53,12 @@ struct netlink_field { > >>> int size; > >>> }; > >>> > >>> +static bool > >>> +is_internal_port(const char *type) > >>> +{ > >>> + return !strcmp(type, "internal"); > >>> +} > >>> + > >>> static struct netlink_field set_flower_map[][4] = { > >>> [OVS_KEY_ATTR_IPV4] = { > >>> { offsetof(struct ovs_key_ipv4, ipv4_src), > >>> @@ -682,8 +689,9 @@ parse_tc_flower_to_match(struct tc_flower *flower, > >>> } > >>> break; > >>> case TC_ACT_OUTPUT: { > >>> - if (action->ifindex_out) { > >>> - outport = > >>> netdev_ifindex_to_odp_port(action->ifindex_out); > >>> + if (action->out.ifindex_out) { > >>> + outport = > >>> + > >>> netdev_ifindex_to_odp_port(action->out.ifindex_out); > >>> if (!outport) { > >>> return ENOENT; > >>> } > >>> @@ -1286,7 +1294,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct > >>> match *match, > >>> odp_port_t port = nl_attr_get_odp_port(nla); > >>> struct netdev *outdev = netdev_ports_get(port, > >>> info->dpif_class); > >>> > >>> - action->ifindex_out = netdev_get_ifindex(outdev); > >>> + action->out.ifindex_out = netdev_get_ifindex(outdev); > >>> + action->out.ingress = > >>> is_internal_port(netdev_get_type(outdev)); > >>> action->type = TC_ACT_OUTPUT; > >>> flower.action_count++; > >>> netdev_close(outdev); > >>> diff --git a/lib/tc.c b/lib/tc.c > >>> index 07b50b2..3548760 100644 > >>> --- a/lib/tc.c > >>> +++ b/lib/tc.c > >>> @@ -25,6 +25,7 @@ > >>> #include <linux/tc_act/tc_gact.h> > >>> #include <linux/tc_act/tc_mirred.h> > >>> #include <linux/tc_act/tc_pedit.h> > >>> +#include <linux/tc_act/tc_skbedit.h> > >>> #include <linux/tc_act/tc_tunnel_key.h> > >>> #include <linux/tc_act/tc_vlan.h> > >>> #include <linux/gen_stats.h> > >>> @@ -1151,14 +1152,20 @@ nl_parse_act_mirred(struct nlattr *options, > >>> struct tc_flower *flower) > >>> mirred_parms = mirred_attrs[TCA_MIRRED_PARMS]; > >>> m = nl_attr_get_unspec(mirred_parms, sizeof *m); > >>> > >>> - if (m->eaction != TCA_EGRESS_REDIR && m->eaction != > >>> TCA_EGRESS_MIRROR) { > >>> + if (m->eaction != TCA_EGRESS_REDIR && m->eaction != > >>> TCA_EGRESS_MIRROR && > >>> + m->eaction != TCA_INGRESS_REDIR && m->eaction != > >>> TCA_INGRESS_MIRROR) { > >>> VLOG_ERR_RL(&error_rl, "unknown mirred action: %d, %d, %d", > >>> m->action, m->eaction, m->ifindex); > >>> return EINVAL; > >>> } > >>> > >>> action = &flower->actions[flower->action_count++]; > >>> - action->ifindex_out = m->ifindex; > >>> + action->out.ifindex_out = m->ifindex; > >>> + if (m->eaction == TCA_INGRESS_REDIR || m->eaction == > >>> TCA_INGRESS_MIRROR) { > >>> + action->out.ingress = true; > >>> + } else { > >>> + action->out.ingress = false; > >>> + } > >>> action->type = TC_ACT_OUTPUT; > >>> > >>> mirred_tm = mirred_attrs[TCA_MIRRED_TM]; > >>> @@ -1301,6 +1308,8 @@ nl_parse_single_action(struct nlattr *action, > >>> struct tc_flower *flower) > >>> err = nl_parse_act_pedit(act_options, flower); > >>> } else if (!strcmp(act_kind, "csum")) { > >>> nl_parse_act_csum(act_options, flower); > >>> + } else if (!strcmp(act_kind, "skbedit")) { > >>> + /* Added for TC rule only (not in OvS rule) so ignore. */ > >>> } else { > >>> VLOG_ERR_RL(&error_rl, "unknown tc action kind: %s", act_kind); > >>> err = EINVAL; > >>> @@ -1729,6 +1738,24 @@ nl_msg_put_act_drop(struct ofpbuf *request) > >>> } > >>> > >>> static void > >>> +nl_msg_put_act_skbedit_to_host(struct ofpbuf *request) > >>> +{ > >>> + ovs_be16 packet_host = 0; > >> > >> can you use the PACKET_HOST definition here instead of 0 ? > >> > > > > I think that would require more compat code but let me look again. > > > > > > you can include if_packet.h which exists as uapi since kernel 3.7. > if we don't require older support than no need for compat. > other option is to define it here which is also better than > specifying 0. >
Hi Roi, yes I see that. I also see in Documentation/faq/releases.rst that recent versions of OvS are only supported on >3.10. Therefore I think we are ok to add the if_packet.h without need for compat. Thanks for the review > >>> + size_t offset; > >>> + > >>> + nl_msg_put_string(request, TCA_ACT_KIND, "skbedit"); > >>> + offset = nl_msg_start_nested(request, TCA_ACT_OPTIONS); > >>> + { > >>> + struct tc_skbedit s = { .action = TC_ACT_PIPE }; > >>> + > >>> + nl_msg_put_unspec(request, TCA_SKBEDIT_PARMS, &s, sizeof s); > >>> + nl_msg_put_unspec(request, TCA_SKBEDIT_PTYPE, &packet_host, > >>> + sizeof packet_host); > >>> + } > >>> + nl_msg_end_nested(request, offset); > >>> +} > >>> + > >>> +static void > >>> nl_msg_put_act_mirred(struct ofpbuf *request, int ifindex, int action, > >>> int eaction) > >>> { > >>> @@ -1916,6 +1943,7 @@ nl_msg_put_flower_acts(struct ofpbuf *request, > >>> struct tc_flower *flower) > >>> uint16_t act_index = 1; > >>> struct tc_action *action; > >>> int i, ifindex = 0; > >>> + bool ingress; > >>> > >>> offset = nl_msg_start_nested(request, TCA_FLOWER_ACT); > >>> { > >>> @@ -1977,19 +2005,39 @@ nl_msg_put_flower_acts(struct ofpbuf *request, > >>> struct tc_flower *flower) > >>> } > >>> break; > >>> case TC_ACT_OUTPUT: { > >>> - ifindex = action->ifindex_out; > >>> + ingress = action->out.ingress; > >>> + ifindex = action->out.ifindex_out; > >>> if (ifindex < 1) { > >>> VLOG_ERR_RL(&error_rl, "%s: invalid ifindex: %d, > >>> type: %d", > >>> __func__, ifindex, action->type); > >>> return EINVAL; > >>> } > >>> + > >>> + if (ingress) { > >>> + /* If redirecting to ingress (internal port) ensure > >>> + * pkt_type on skb is set to PACKET_HOST. */ > >>> + act_offset = nl_msg_start_nested(request, > >>> act_index++); > >>> + nl_msg_put_act_skbedit_to_host(request); > >>> + nl_msg_end_nested(request, act_offset); > >>> + } > >>> + > >>> act_offset = nl_msg_start_nested(request, act_index++); > >>> if (i == flower->action_count - 1) { > >>> - nl_msg_put_act_mirred(request, ifindex, > >>> TC_ACT_STOLEN, > >>> - TCA_EGRESS_REDIR); > >>> + if (ingress) { > >>> + nl_msg_put_act_mirred(request, ifindex, > >>> TC_ACT_STOLEN, > >>> + TCA_INGRESS_REDIR); > >>> + } else { > >>> + nl_msg_put_act_mirred(request, ifindex, > >>> TC_ACT_STOLEN, > >>> + TCA_EGRESS_REDIR); > >>> + } > >>> } else { > >>> - nl_msg_put_act_mirred(request, ifindex, TC_ACT_PIPE, > >>> - TCA_EGRESS_MIRROR); > >>> + if (ingress) { > >>> + nl_msg_put_act_mirred(request, ifindex, > >>> TC_ACT_PIPE, > >>> + TCA_INGRESS_MIRROR); > >>> + } else { > >>> + nl_msg_put_act_mirred(request, ifindex, > >>> TC_ACT_PIPE, > >>> + TCA_EGRESS_MIRROR); > >>> + } > >>> } > >>> nl_msg_put_act_cookie(request, &flower->act_cookie); > >>> nl_msg_end_nested(request, act_offset); > >>> diff --git a/lib/tc.h b/lib/tc.h > >>> index d374711..154e120 100644 > >>> --- a/lib/tc.h > >>> +++ b/lib/tc.h > >>> @@ -147,7 +147,10 @@ enum tc_action_type { > >>> > >>> struct tc_action { > >>> union { > >>> - int ifindex_out; > >>> + struct { > >>> + int ifindex_out; > >>> + bool ingress; > >>> + } out; > >>> > >>> struct { > >>> ovs_be16 vlan_push_tpid; > >>> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev