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.
> > + 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
