[ovs-dev] [PATCH OVS 2/4] ovs-tc: allow offloading of ingress mirr TC actions to datapath
John Hurley
john.hurley at netronome.com
Wed Apr 3 10:48:32 UTC 2019
On Wed, Apr 3, 2019 at 7:19 AM Roi Dayan <roid at mellanox.com> wrote:
>
>
>
> On 02/04/2019 20:14, John Hurley wrote:
> > On Tue, Apr 2, 2019 at 4:05 PM Roi Dayan <roid at 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 at netronome.com>
> >>> Reviewed-by: Simon Horman <simon.horman at 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;
> >>>
More information about the dev
mailing list