| Message ID | 1554197264-15502-3-git-send-email-john.hurley@netronome.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | ovs-tc: support OvS internal port offload | expand |
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.hurley@netronome.com> > Reviewed-by: Simon Horman <simon.horman@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 ? > + 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; >
On Tue, Apr 2, 2019 at 4:05 PM Roi Dayan <roid@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.hurley@netronome.com> > > Reviewed-by: Simon Horman <simon.horman@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. > > + 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; > >
On 02/04/2019 20:14, John Hurley wrote: > On Tue, Apr 2, 2019 at 4:05 PM Roi Dayan <roid@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.hurley@netronome.com> >>> Reviewed-by: Simon Horman <simon.horman@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. >>> + 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; >>>
On Wed, Apr 3, 2019 at 7:19 AM Roi Dayan <roid@mellanox.com> wrote: > > > > On 02/04/2019 20:14, John Hurley wrote: > > On Tue, Apr 2, 2019 at 4:05 PM Roi Dayan <roid@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.hurley@netronome.com> > >>> Reviewed-by: Simon Horman <simon.horman@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; > >>>
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; + 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;