diff mbox series

[ovs-dev,OVS,v2,4/4] ovs-tc: offload datapath rules matching on internal ports

Message ID 1554393913-9671-5-git-send-email-john.hurley@netronome.com
State Superseded
Delegated to: Simon Horman
Headers show
Series ovs-tc: support OvS internal port offload | expand

Commit Message

John Hurley April 4, 2019, 4:05 p.m. UTC
Rules applied to OvS internal ports are not represented in TC datapaths.
However, it is possible to support rules matching on internal ports in TC.
The start_xmit ndo of OvS internal ports directs packets back into the OvS
kernel datapath where they are rematched with the ingress port now being
that of the internal port. Due to this, rules matching on an internal port
can be added as TC filters to an egress qdisc for these ports.

Allow rules applied to internal ports to be offloaded to TC as egress
filters. Rules redirecting to an internal port are also offloaded. These
are supported by the redirect ingress functionality applied in an earlier
patch.

Signed-off-by: John Hurley <john.hurley@netronome.com>
---
 lib/dpif.c               | 13 +++++-------
 lib/netdev-linux.c       |  1 +
 lib/netdev-tc-offloads.c | 55 +++++++++++++++++++++++++++++++++++++-----------
 3 files changed, 49 insertions(+), 20 deletions(-)

Comments

Roi Dayan April 8, 2019, 10:38 a.m. UTC | #1
On 04/04/2019 19:05, John Hurley wrote:
> Rules applied to OvS internal ports are not represented in TC datapaths.
> However, it is possible to support rules matching on internal ports in TC.
> The start_xmit ndo of OvS internal ports directs packets back into the OvS
> kernel datapath where they are rematched with the ingress port now being
> that of the internal port. Due to this, rules matching on an internal port
> can be added as TC filters to an egress qdisc for these ports.
> 
> Allow rules applied to internal ports to be offloaded to TC as egress
> filters. Rules redirecting to an internal port are also offloaded. These
> are supported by the redirect ingress functionality applied in an earlier
> patch.
> 
> Signed-off-by: John Hurley <john.hurley@netronome.com>
> ---
>  lib/dpif.c               | 13 +++++-------
>  lib/netdev-linux.c       |  1 +
>  lib/netdev-tc-offloads.c | 55 +++++++++++++++++++++++++++++++++++++-----------
>  3 files changed, 49 insertions(+), 20 deletions(-)
> 
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 457c9bf..063ba20 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -101,12 +101,9 @@ static bool should_log_flow_message(const struct vlog_module *module,
>  struct seq *tnl_conf_seq;
>  
>  static bool
> -dpif_is_internal_port(const char *type)
> +dpif_is_tap_port(const char *type)
>  {
> -    /* For userspace datapath, tap devices are the equivalent
> -     * of internal devices in the kernel datapath, so both
> -     * these types are 'internal' devices. */
> -    return !strcmp(type, "internal") || !strcmp(type, "tap");
> +    return !strcmp(type, "tap");
>  }
>  
>  static void
> @@ -359,7 +356,7 @@ do_open(const char *name, const char *type, bool create, struct dpif **dpifp)
>              struct netdev *netdev;
>              int err;
>  
> -            if (dpif_is_internal_port(dpif_port.type)) {
> +            if (dpif_is_tap_port(dpif_port.type)) {
>                  continue;
>              }
>  
> @@ -434,7 +431,7 @@ dpif_remove_netdev_ports(struct dpif *dpif) {
>          struct dpif_port dpif_port;
>  
>          DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, dpif) {
> -            if (!dpif_is_internal_port(dpif_port.type)) {
> +            if (!dpif_is_tap_port(dpif_port.type)) {
>                  netdev_ports_remove(dpif_port.port_no, dpif->dpif_class);
>              }
>          }
> @@ -582,7 +579,7 @@ dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop)
>          VLOG_DBG_RL(&dpmsg_rl, "%s: added %s as port %"PRIu32,
>                      dpif_name(dpif), netdev_name, port_no);
>  
> -        if (!dpif_is_internal_port(netdev_get_type(netdev))) {
> +        if (!dpif_is_tap_port(netdev_get_type(netdev))) {
>  
>              struct dpif_port dpif_port;
>  
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index 87337e0..776d938 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -3340,6 +3340,7 @@ const struct netdev_class netdev_tap_class = {
>  
>  const struct netdev_class netdev_internal_class = {
>      NETDEV_LINUX_CLASS_COMMON,
> +    LINUX_FLOW_OFFLOAD_API,
>      .type = "internal",
>      .construct = netdev_linux_construct,
>      .get_stats = netdev_internal_get_stats,
> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> index 64a9a9e..7c2ebe0 100644
> --- a/lib/netdev-tc-offloads.c
> +++ b/lib/netdev-tc-offloads.c
> @@ -185,11 +185,12 @@ del_ufid_tc_mapping(const ovs_u128 *ufid)
>  /* Wrapper function to delete filter and ufid tc mapping */
>  static int
>  del_filter_and_ufid_mapping(int ifindex, int prio, int handle,
> -                            uint32_t block_id, const ovs_u128 *ufid)
> +                            uint32_t block_id, const ovs_u128 *ufid,
> +                            enum tc_qdisc_hook hook)
>  {
>      int err;
>  
> -    err = tc_del_filter(ifindex, prio, handle, block_id, TC_INGRESS);
> +    err = tc_del_filter(ifindex, prio, handle, block_id, hook);
>      del_ufid_tc_mapping(ufid);
>  
>      return err;
> @@ -346,9 +347,14 @@ get_block_id_from_netdev(struct netdev *netdev)
>  int
>  netdev_tc_flow_flush(struct netdev *netdev)
>  {
> +    enum tc_qdisc_hook hook = TC_INGRESS;
>      int ifindex = netdev_get_ifindex(netdev);
>      uint32_t block_id = 0;
>  
> +    if (is_internal_port(netdev_get_type(netdev))) {
> +        hook = TC_EGRESS;
> +    }
> +
>      if (ifindex < 0) {
>          VLOG_ERR_RL(&error_rl, "flow_flush: failed to get ifindex for %s: %s",
>                      netdev_get_name(netdev), ovs_strerror(-ifindex));
> @@ -357,17 +363,22 @@ netdev_tc_flow_flush(struct netdev *netdev)
>  
>      block_id = get_block_id_from_netdev(netdev);
>  
> -    return tc_flush(ifindex, block_id, TC_INGRESS);
> +    return tc_flush(ifindex, block_id, hook);
>  }
>  
>  int
>  netdev_tc_flow_dump_create(struct netdev *netdev,
>                             struct netdev_flow_dump **dump_out)
>  {
> +    enum tc_qdisc_hook hook = TC_INGRESS;
>      struct netdev_flow_dump *dump;
>      uint32_t block_id = 0;
>      int ifindex;
>  
> +    if (is_internal_port(netdev_get_type(netdev))) {
> +        hook = TC_EGRESS;
> +    }
> +
>      ifindex = netdev_get_ifindex(netdev);
>      if (ifindex < 0) {
>          VLOG_ERR_RL(&error_rl, "dump_create: failed to get ifindex for %s: %s",
> @@ -379,7 +390,7 @@ netdev_tc_flow_dump_create(struct netdev *netdev,
>      dump = xzalloc(sizeof *dump);
>      dump->nl_dump = xzalloc(sizeof *dump->nl_dump);
>      dump->netdev = netdev_ref(netdev);
> -    tc_dump_flower_start(ifindex, dump->nl_dump, block_id, TC_INGRESS);
> +    tc_dump_flower_start(ifindex, dump->nl_dump, block_id, hook);
>  
>      *dump_out = dump;
>  
> @@ -1085,6 +1096,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>      struct flow *mask = &match->wc.masks;
>      const struct flow_tnl *tnl = &match->flow.tunnel;
>      const struct flow_tnl *tnl_mask = &mask->tunnel;
> +    enum tc_qdisc_hook hook = TC_INGRESS;
>      struct tc_action *action;
>      uint32_t block_id = 0;
>      struct nlattr *nla;
> @@ -1094,6 +1106,10 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>      int ifindex;
>      int err;
>  
> +    if (is_internal_port(netdev_get_type(netdev))) {
> +        hook = TC_EGRESS;
> +    }
> +
>      ifindex = netdev_get_ifindex(netdev);
>      if (ifindex < 0) {
>          VLOG_ERR_RL(&error_rl, "flow_put: failed to get ifindex for %s: %s",
> @@ -1342,7 +1358,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>      handle = get_ufid_tc_mapping(ufid, &prio, NULL);
>      if (handle && prio) {
>          VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d", handle, prio);
> -        del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid);
> +        del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid,
> +                                    hook);
>      }
>  
>      if (!prio) {
> @@ -1356,8 +1373,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>      flower.act_cookie.data = ufid;
>      flower.act_cookie.len = sizeof *ufid;
>  
> -    err = tc_replace_flower(ifindex, prio, handle, &flower, block_id,
> -                            TC_INGRESS);
> +    err = tc_replace_flower(ifindex, prio, handle, &flower, block_id, hook);
>      if (!err) {
>          add_ufid_tc_mapping(ufid, flower.prio, flower.handle, netdev, ifindex);
>      }
> @@ -1375,6 +1391,7 @@ netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
>                     struct ofpbuf *buf)
>  {
>      static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> +    enum tc_qdisc_hook hook = TC_INGRESS;
>      struct netdev *dev;
>      struct tc_flower flower;
>      uint32_t block_id = 0;
> @@ -1389,6 +1406,10 @@ netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
>          return ENOENT;
>      }
>  
> +    if (is_internal_port(netdev_get_type(netdev))) {
> +        hook = TC_EGRESS;
> +    }
> +
>      ifindex = netdev_get_ifindex(dev);
>      if (ifindex < 0) {
>          VLOG_ERR_RL(&error_rl, "flow_get: failed to get ifindex for %s: %s",
> @@ -1400,7 +1421,7 @@ netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
>      block_id = get_block_id_from_netdev(dev);
>      VLOG_DBG_RL(&rl, "flow get (dev %s prio %d handle %d block_id %d)",
>                  netdev_get_name(dev), prio, handle, block_id);
> -    err = tc_get_flower(ifindex, prio, handle, &flower, block_id, TC_INGRESS);
> +    err = tc_get_flower(ifindex, prio, handle, &flower, block_id, hook);
>      netdev_close(dev);
>      if (err) {
>          VLOG_ERR_RL(&error_rl, "flow get failed (dev %s prio %d handle %d): %s",
> @@ -1422,6 +1443,7 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
>                     const ovs_u128 *ufid,
>                     struct dpif_flow_stats *stats)
>  {
> +    enum tc_qdisc_hook hook = TC_INGRESS;
>      struct tc_flower flower;
>      uint32_t block_id = 0;
>      struct netdev *dev;
> @@ -1435,6 +1457,10 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
>          return ENOENT;
>      }
>  
> +    if (is_internal_port(netdev_get_type(netdev))) {
> +        hook = TC_EGRESS;
> +    }
> +
>      ifindex = netdev_get_ifindex(dev);
>      if (ifindex < 0) {
>          VLOG_ERR_RL(&error_rl, "flow_del: failed to get ifindex for %s: %s",
> @@ -1447,15 +1473,15 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
>  
>      if (stats) {
>          memset(stats, 0, sizeof *stats);
> -        if (!tc_get_flower(ifindex, prio, handle, &flower, block_id,
> -                           TC_INGRESS)) {
> +        if (!tc_get_flower(ifindex, prio, handle, &flower, block_id, hook)) {
>              stats->n_packets = get_32aligned_u64(&flower.stats.n_packets);
>              stats->n_bytes = get_32aligned_u64(&flower.stats.n_bytes);
>              stats->used = flower.lastused;
>          }
>      }
>  
> -    error = del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid);
> +    error = del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid,
> +                                        hook);
>  
>      netdev_close(dev);
>  
> @@ -1527,10 +1553,15 @@ netdev_tc_init_flow_api(struct netdev *netdev)
>  {
>      static struct ovsthread_once multi_mask_once = OVSTHREAD_ONCE_INITIALIZER;
>      static struct ovsthread_once block_once = OVSTHREAD_ONCE_INITIALIZER;
> +    enum tc_qdisc_hook hook = TC_INGRESS;
>      uint32_t block_id = 0;
>      int ifindex;
>      int error;
>  
> +    if (is_internal_port(netdev_get_type(netdev))) {
> +        hook = TC_EGRESS;
> +    }
> +
>      ifindex = netdev_get_ifindex(netdev);
>      if (ifindex < 0) {
>          VLOG_ERR_RL(&error_rl, "init: failed to get ifindex for %s: %s",
> @@ -1552,7 +1583,7 @@ netdev_tc_init_flow_api(struct netdev *netdev)
>      }
>  
>      block_id = get_block_id_from_netdev(netdev);
> -    error = tc_add_del_qdisc(ifindex, true, block_id, TC_INGRESS);
> +    error = tc_add_del_qdisc(ifindex, true, block_id, hook);
>  
>      if (error && error != EEXIST) {
>          VLOG_ERR("failed adding ingress qdisc required for offloading: %s",
> 

Reviewed-by: Roi Dayan <roid@mellanox.com>
Roi Dayan April 8, 2019, 10:49 a.m. UTC | #2
On 08/04/2019 13:38, Roi Dayan wrote:
> 
> 
> On 04/04/2019 19:05, John Hurley wrote:
>> Rules applied to OvS internal ports are not represented in TC datapaths.
>> However, it is possible to support rules matching on internal ports in TC.
>> The start_xmit ndo of OvS internal ports directs packets back into the OvS
>> kernel datapath where they are rematched with the ingress port now being
>> that of the internal port. Due to this, rules matching on an internal port
>> can be added as TC filters to an egress qdisc for these ports.
>>
>> Allow rules applied to internal ports to be offloaded to TC as egress
>> filters. Rules redirecting to an internal port are also offloaded. These
>> are supported by the redirect ingress functionality applied in an earlier
>> patch.
>>
>> Signed-off-by: John Hurley <john.hurley@netronome.com>
>> ---
>>  lib/dpif.c               | 13 +++++-------
>>  lib/netdev-linux.c       |  1 +
>>  lib/netdev-tc-offloads.c | 55 +++++++++++++++++++++++++++++++++++++-----------
>>  3 files changed, 49 insertions(+), 20 deletions(-)
>>
>> diff --git a/lib/dpif.c b/lib/dpif.c
>> index 457c9bf..063ba20 100644
>> --- a/lib/dpif.c
>> +++ b/lib/dpif.c
>> @@ -101,12 +101,9 @@ static bool should_log_flow_message(const struct vlog_module *module,
>>  struct seq *tnl_conf_seq;
>>  
>>  static bool
>> -dpif_is_internal_port(const char *type)
>> +dpif_is_tap_port(const char *type)
>>  {
>> -    /* For userspace datapath, tap devices are the equivalent
>> -     * of internal devices in the kernel datapath, so both
>> -     * these types are 'internal' devices. */
>> -    return !strcmp(type, "internal") || !strcmp(type, "tap");
>> +    return !strcmp(type, "tap");
>>  }
>>  
>>  static void
>> @@ -359,7 +356,7 @@ do_open(const char *name, const char *type, bool create, struct dpif **dpifp)
>>              struct netdev *netdev;
>>              int err;
>>  
>> -            if (dpif_is_internal_port(dpif_port.type)) {
>> +            if (dpif_is_tap_port(dpif_port.type)) {
>>                  continue;
>>              }
>>  
>> @@ -434,7 +431,7 @@ dpif_remove_netdev_ports(struct dpif *dpif) {
>>          struct dpif_port dpif_port;
>>  
>>          DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, dpif) {
>> -            if (!dpif_is_internal_port(dpif_port.type)) {
>> +            if (!dpif_is_tap_port(dpif_port.type)) {
>>                  netdev_ports_remove(dpif_port.port_no, dpif->dpif_class);
>>              }
>>          }
>> @@ -582,7 +579,7 @@ dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop)
>>          VLOG_DBG_RL(&dpmsg_rl, "%s: added %s as port %"PRIu32,
>>                      dpif_name(dpif), netdev_name, port_no);
>>  
>> -        if (!dpif_is_internal_port(netdev_get_type(netdev))) {
>> +        if (!dpif_is_tap_port(netdev_get_type(netdev))) {
>>  
>>              struct dpif_port dpif_port;
>>  
>> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
>> index 87337e0..776d938 100644
>> --- a/lib/netdev-linux.c
>> +++ b/lib/netdev-linux.c
>> @@ -3340,6 +3340,7 @@ const struct netdev_class netdev_tap_class = {
>>  
>>  const struct netdev_class netdev_internal_class = {
>>      NETDEV_LINUX_CLASS_COMMON,
>> +    LINUX_FLOW_OFFLOAD_API,
>>      .type = "internal",
>>      .construct = netdev_linux_construct,
>>      .get_stats = netdev_internal_get_stats,
>> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
>> index 64a9a9e..7c2ebe0 100644
>> --- a/lib/netdev-tc-offloads.c
>> +++ b/lib/netdev-tc-offloads.c
>> @@ -185,11 +185,12 @@ del_ufid_tc_mapping(const ovs_u128 *ufid)
>>  /* Wrapper function to delete filter and ufid tc mapping */
>>  static int
>>  del_filter_and_ufid_mapping(int ifindex, int prio, int handle,
>> -                            uint32_t block_id, const ovs_u128 *ufid)
>> +                            uint32_t block_id, const ovs_u128 *ufid,
>> +                            enum tc_qdisc_hook hook)
>>  {
>>      int err;
>>  
>> -    err = tc_del_filter(ifindex, prio, handle, block_id, TC_INGRESS);
>> +    err = tc_del_filter(ifindex, prio, handle, block_id, hook);
>>      del_ufid_tc_mapping(ufid);
>>  
>>      return err;
>> @@ -346,9 +347,14 @@ get_block_id_from_netdev(struct netdev *netdev)
>>  int
>>  netdev_tc_flow_flush(struct netdev *netdev)
>>  {
>> +    enum tc_qdisc_hook hook = TC_INGRESS;
>>      int ifindex = netdev_get_ifindex(netdev);
>>      uint32_t block_id = 0;
>>  
>> +    if (is_internal_port(netdev_get_type(netdev))) {
>> +        hook = TC_EGRESS;
>> +    }
>> +
>>      if (ifindex < 0) {
>>          VLOG_ERR_RL(&error_rl, "flow_flush: failed to get ifindex for %s: %s",
>>                      netdev_get_name(netdev), ovs_strerror(-ifindex));
>> @@ -357,17 +363,22 @@ netdev_tc_flow_flush(struct netdev *netdev)
>>  
>>      block_id = get_block_id_from_netdev(netdev);
>>  
>> -    return tc_flush(ifindex, block_id, TC_INGRESS);
>> +    return tc_flush(ifindex, block_id, hook);
>>  }
>>  
>>  int
>>  netdev_tc_flow_dump_create(struct netdev *netdev,
>>                             struct netdev_flow_dump **dump_out)
>>  {
>> +    enum tc_qdisc_hook hook = TC_INGRESS;
>>      struct netdev_flow_dump *dump;
>>      uint32_t block_id = 0;
>>      int ifindex;
>>  
>> +    if (is_internal_port(netdev_get_type(netdev))) {
>> +        hook = TC_EGRESS;
>> +    }
>> +
>>      ifindex = netdev_get_ifindex(netdev);
>>      if (ifindex < 0) {
>>          VLOG_ERR_RL(&error_rl, "dump_create: failed to get ifindex for %s: %s",
>> @@ -379,7 +390,7 @@ netdev_tc_flow_dump_create(struct netdev *netdev,
>>      dump = xzalloc(sizeof *dump);
>>      dump->nl_dump = xzalloc(sizeof *dump->nl_dump);
>>      dump->netdev = netdev_ref(netdev);
>> -    tc_dump_flower_start(ifindex, dump->nl_dump, block_id, TC_INGRESS);
>> +    tc_dump_flower_start(ifindex, dump->nl_dump, block_id, hook);
>>  
>>      *dump_out = dump;
>>  
>> @@ -1085,6 +1096,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>>      struct flow *mask = &match->wc.masks;
>>      const struct flow_tnl *tnl = &match->flow.tunnel;
>>      const struct flow_tnl *tnl_mask = &mask->tunnel;
>> +    enum tc_qdisc_hook hook = TC_INGRESS;
>>      struct tc_action *action;
>>      uint32_t block_id = 0;
>>      struct nlattr *nla;
>> @@ -1094,6 +1106,10 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>>      int ifindex;
>>      int err;
>>  
>> +    if (is_internal_port(netdev_get_type(netdev))) {
>> +        hook = TC_EGRESS;
>> +    }
>> +
>>      ifindex = netdev_get_ifindex(netdev);
>>      if (ifindex < 0) {
>>          VLOG_ERR_RL(&error_rl, "flow_put: failed to get ifindex for %s: %s",
>> @@ -1342,7 +1358,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>>      handle = get_ufid_tc_mapping(ufid, &prio, NULL);
>>      if (handle && prio) {
>>          VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d", handle, prio);
>> -        del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid);
>> +        del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid,
>> +                                    hook);
>>      }
>>  
>>      if (!prio) {
>> @@ -1356,8 +1373,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>>      flower.act_cookie.data = ufid;
>>      flower.act_cookie.len = sizeof *ufid;
>>  
>> -    err = tc_replace_flower(ifindex, prio, handle, &flower, block_id,
>> -                            TC_INGRESS);
>> +    err = tc_replace_flower(ifindex, prio, handle, &flower, block_id, hook);
>>      if (!err) {
>>          add_ufid_tc_mapping(ufid, flower.prio, flower.handle, netdev, ifindex);
>>      }
>> @@ -1375,6 +1391,7 @@ netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
>>                     struct ofpbuf *buf)
>>  {
>>      static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
>> +    enum tc_qdisc_hook hook = TC_INGRESS;
>>      struct netdev *dev;
>>      struct tc_flower flower;
>>      uint32_t block_id = 0;
>> @@ -1389,6 +1406,10 @@ netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
>>          return ENOENT;
>>      }
>>  
>> +    if (is_internal_port(netdev_get_type(netdev))) {
>> +        hook = TC_EGRESS;
>> +    }
>> +
>>      ifindex = netdev_get_ifindex(dev);
>>      if (ifindex < 0) {
>>          VLOG_ERR_RL(&error_rl, "flow_get: failed to get ifindex for %s: %s",
>> @@ -1400,7 +1421,7 @@ netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
>>      block_id = get_block_id_from_netdev(dev);
>>      VLOG_DBG_RL(&rl, "flow get (dev %s prio %d handle %d block_id %d)",
>>                  netdev_get_name(dev), prio, handle, block_id);
>> -    err = tc_get_flower(ifindex, prio, handle, &flower, block_id, TC_INGRESS);
>> +    err = tc_get_flower(ifindex, prio, handle, &flower, block_id, hook);
>>      netdev_close(dev);
>>      if (err) {
>>          VLOG_ERR_RL(&error_rl, "flow get failed (dev %s prio %d handle %d): %s",
>> @@ -1422,6 +1443,7 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
>>                     const ovs_u128 *ufid,
>>                     struct dpif_flow_stats *stats)
>>  {
>> +    enum tc_qdisc_hook hook = TC_INGRESS;
>>      struct tc_flower flower;
>>      uint32_t block_id = 0;
>>      struct netdev *dev;
>> @@ -1435,6 +1457,10 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
>>          return ENOENT;
>>      }
>>  
>> +    if (is_internal_port(netdev_get_type(netdev))) {
>> +        hook = TC_EGRESS;
>> +    }
>> +
>>      ifindex = netdev_get_ifindex(dev);
>>      if (ifindex < 0) {
>>          VLOG_ERR_RL(&error_rl, "flow_del: failed to get ifindex for %s: %s",
>> @@ -1447,15 +1473,15 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
>>  
>>      if (stats) {
>>          memset(stats, 0, sizeof *stats);
>> -        if (!tc_get_flower(ifindex, prio, handle, &flower, block_id,
>> -                           TC_INGRESS)) {
>> +        if (!tc_get_flower(ifindex, prio, handle, &flower, block_id, hook)) {
>>              stats->n_packets = get_32aligned_u64(&flower.stats.n_packets);
>>              stats->n_bytes = get_32aligned_u64(&flower.stats.n_bytes);
>>              stats->used = flower.lastused;
>>          }
>>      }
>>  
>> -    error = del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid);
>> +    error = del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid,
>> +                                        hook);
>>  
>>      netdev_close(dev);
>>  
>> @@ -1527,10 +1553,15 @@ netdev_tc_init_flow_api(struct netdev *netdev)
>>  {
>>      static struct ovsthread_once multi_mask_once = OVSTHREAD_ONCE_INITIALIZER;
>>      static struct ovsthread_once block_once = OVSTHREAD_ONCE_INITIALIZER;
>> +    enum tc_qdisc_hook hook = TC_INGRESS;
>>      uint32_t block_id = 0;
>>      int ifindex;
>>      int error;
>>  
>> +    if (is_internal_port(netdev_get_type(netdev))) {
>> +        hook = TC_EGRESS;
>> +    }
>> +

a small refactor suggestion could be a small function for getting
qdisc hook as we have multiple functions declaring hook as ingress and
then change to egress if internal port.

static get_qdisc_hook(netdev)
{
  return is_internal_port(netdev_get_type(netdev) ? TC_EGRESS : TC_INGRESS;
}

then all functions will just have this line

enum tc_qdisc_hook hook = get_qdisc_hook(netdev);


>>      ifindex = netdev_get_ifindex(netdev);
>>      if (ifindex < 0) {
>>          VLOG_ERR_RL(&error_rl, "init: failed to get ifindex for %s: %s",
>> @@ -1552,7 +1583,7 @@ netdev_tc_init_flow_api(struct netdev *netdev)
>>      }
>>  
>>      block_id = get_block_id_from_netdev(netdev);
>> -    error = tc_add_del_qdisc(ifindex, true, block_id, TC_INGRESS);
>> +    error = tc_add_del_qdisc(ifindex, true, block_id, hook);
>>  
>>      if (error && error != EEXIST) {
>>          VLOG_ERR("failed adding ingress qdisc required for offloading: %s",
>>
> 
> Reviewed-by: Roi Dayan <roid@mellanox.com>
>
John Hurley April 8, 2019, 1:01 p.m. UTC | #3
On Mon, Apr 8, 2019 at 11:49 AM Roi Dayan <roid@mellanox.com> wrote:
>
>
>
> On 08/04/2019 13:38, Roi Dayan wrote:
> >
> >
> > On 04/04/2019 19:05, John Hurley wrote:
> >> Rules applied to OvS internal ports are not represented in TC datapaths.
> >> However, it is possible to support rules matching on internal ports in TC.
> >> The start_xmit ndo of OvS internal ports directs packets back into the OvS
> >> kernel datapath where they are rematched with the ingress port now being
> >> that of the internal port. Due to this, rules matching on an internal port
> >> can be added as TC filters to an egress qdisc for these ports.
> >>
> >> Allow rules applied to internal ports to be offloaded to TC as egress
> >> filters. Rules redirecting to an internal port are also offloaded. These
> >> are supported by the redirect ingress functionality applied in an earlier
> >> patch.
> >>
> >> Signed-off-by: John Hurley <john.hurley@netronome.com>
> >> ---
> >>  lib/dpif.c               | 13 +++++-------
> >>  lib/netdev-linux.c       |  1 +
> >>  lib/netdev-tc-offloads.c | 55 +++++++++++++++++++++++++++++++++++++-----------
> >>  3 files changed, 49 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/lib/dpif.c b/lib/dpif.c
> >> index 457c9bf..063ba20 100644
> >> --- a/lib/dpif.c
> >> +++ b/lib/dpif.c
> >> @@ -101,12 +101,9 @@ static bool should_log_flow_message(const struct vlog_module *module,
> >>  struct seq *tnl_conf_seq;
> >>
> >>  static bool
> >> -dpif_is_internal_port(const char *type)
> >> +dpif_is_tap_port(const char *type)
> >>  {
> >> -    /* For userspace datapath, tap devices are the equivalent
> >> -     * of internal devices in the kernel datapath, so both
> >> -     * these types are 'internal' devices. */
> >> -    return !strcmp(type, "internal") || !strcmp(type, "tap");
> >> +    return !strcmp(type, "tap");
> >>  }
> >>
> >>  static void
> >> @@ -359,7 +356,7 @@ do_open(const char *name, const char *type, bool create, struct dpif **dpifp)
> >>              struct netdev *netdev;
> >>              int err;
> >>
> >> -            if (dpif_is_internal_port(dpif_port.type)) {
> >> +            if (dpif_is_tap_port(dpif_port.type)) {
> >>                  continue;
> >>              }
> >>
> >> @@ -434,7 +431,7 @@ dpif_remove_netdev_ports(struct dpif *dpif) {
> >>          struct dpif_port dpif_port;
> >>
> >>          DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, dpif) {
> >> -            if (!dpif_is_internal_port(dpif_port.type)) {
> >> +            if (!dpif_is_tap_port(dpif_port.type)) {
> >>                  netdev_ports_remove(dpif_port.port_no, dpif->dpif_class);
> >>              }
> >>          }
> >> @@ -582,7 +579,7 @@ dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop)
> >>          VLOG_DBG_RL(&dpmsg_rl, "%s: added %s as port %"PRIu32,
> >>                      dpif_name(dpif), netdev_name, port_no);
> >>
> >> -        if (!dpif_is_internal_port(netdev_get_type(netdev))) {
> >> +        if (!dpif_is_tap_port(netdev_get_type(netdev))) {
> >>
> >>              struct dpif_port dpif_port;
> >>
> >> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> >> index 87337e0..776d938 100644
> >> --- a/lib/netdev-linux.c
> >> +++ b/lib/netdev-linux.c
> >> @@ -3340,6 +3340,7 @@ const struct netdev_class netdev_tap_class = {
> >>
> >>  const struct netdev_class netdev_internal_class = {
> >>      NETDEV_LINUX_CLASS_COMMON,
> >> +    LINUX_FLOW_OFFLOAD_API,
> >>      .type = "internal",
> >>      .construct = netdev_linux_construct,
> >>      .get_stats = netdev_internal_get_stats,
> >> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> >> index 64a9a9e..7c2ebe0 100644
> >> --- a/lib/netdev-tc-offloads.c
> >> +++ b/lib/netdev-tc-offloads.c
> >> @@ -185,11 +185,12 @@ del_ufid_tc_mapping(const ovs_u128 *ufid)
> >>  /* Wrapper function to delete filter and ufid tc mapping */
> >>  static int
> >>  del_filter_and_ufid_mapping(int ifindex, int prio, int handle,
> >> -                            uint32_t block_id, const ovs_u128 *ufid)
> >> +                            uint32_t block_id, const ovs_u128 *ufid,
> >> +                            enum tc_qdisc_hook hook)
> >>  {
> >>      int err;
> >>
> >> -    err = tc_del_filter(ifindex, prio, handle, block_id, TC_INGRESS);
> >> +    err = tc_del_filter(ifindex, prio, handle, block_id, hook);
> >>      del_ufid_tc_mapping(ufid);
> >>
> >>      return err;
> >> @@ -346,9 +347,14 @@ get_block_id_from_netdev(struct netdev *netdev)
> >>  int
> >>  netdev_tc_flow_flush(struct netdev *netdev)
> >>  {
> >> +    enum tc_qdisc_hook hook = TC_INGRESS;
> >>      int ifindex = netdev_get_ifindex(netdev);
> >>      uint32_t block_id = 0;
> >>
> >> +    if (is_internal_port(netdev_get_type(netdev))) {
> >> +        hook = TC_EGRESS;
> >> +    }
> >> +
> >>      if (ifindex < 0) {
> >>          VLOG_ERR_RL(&error_rl, "flow_flush: failed to get ifindex for %s: %s",
> >>                      netdev_get_name(netdev), ovs_strerror(-ifindex));
> >> @@ -357,17 +363,22 @@ netdev_tc_flow_flush(struct netdev *netdev)
> >>
> >>      block_id = get_block_id_from_netdev(netdev);
> >>
> >> -    return tc_flush(ifindex, block_id, TC_INGRESS);
> >> +    return tc_flush(ifindex, block_id, hook);
> >>  }
> >>
> >>  int
> >>  netdev_tc_flow_dump_create(struct netdev *netdev,
> >>                             struct netdev_flow_dump **dump_out)
> >>  {
> >> +    enum tc_qdisc_hook hook = TC_INGRESS;
> >>      struct netdev_flow_dump *dump;
> >>      uint32_t block_id = 0;
> >>      int ifindex;
> >>
> >> +    if (is_internal_port(netdev_get_type(netdev))) {
> >> +        hook = TC_EGRESS;
> >> +    }
> >> +
> >>      ifindex = netdev_get_ifindex(netdev);
> >>      if (ifindex < 0) {
> >>          VLOG_ERR_RL(&error_rl, "dump_create: failed to get ifindex for %s: %s",
> >> @@ -379,7 +390,7 @@ netdev_tc_flow_dump_create(struct netdev *netdev,
> >>      dump = xzalloc(sizeof *dump);
> >>      dump->nl_dump = xzalloc(sizeof *dump->nl_dump);
> >>      dump->netdev = netdev_ref(netdev);
> >> -    tc_dump_flower_start(ifindex, dump->nl_dump, block_id, TC_INGRESS);
> >> +    tc_dump_flower_start(ifindex, dump->nl_dump, block_id, hook);
> >>
> >>      *dump_out = dump;
> >>
> >> @@ -1085,6 +1096,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
> >>      struct flow *mask = &match->wc.masks;
> >>      const struct flow_tnl *tnl = &match->flow.tunnel;
> >>      const struct flow_tnl *tnl_mask = &mask->tunnel;
> >> +    enum tc_qdisc_hook hook = TC_INGRESS;
> >>      struct tc_action *action;
> >>      uint32_t block_id = 0;
> >>      struct nlattr *nla;
> >> @@ -1094,6 +1106,10 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
> >>      int ifindex;
> >>      int err;
> >>
> >> +    if (is_internal_port(netdev_get_type(netdev))) {
> >> +        hook = TC_EGRESS;
> >> +    }
> >> +
> >>      ifindex = netdev_get_ifindex(netdev);
> >>      if (ifindex < 0) {
> >>          VLOG_ERR_RL(&error_rl, "flow_put: failed to get ifindex for %s: %s",
> >> @@ -1342,7 +1358,8 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
> >>      handle = get_ufid_tc_mapping(ufid, &prio, NULL);
> >>      if (handle && prio) {
> >>          VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d", handle, prio);
> >> -        del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid);
> >> +        del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid,
> >> +                                    hook);
> >>      }
> >>
> >>      if (!prio) {
> >> @@ -1356,8 +1373,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
> >>      flower.act_cookie.data = ufid;
> >>      flower.act_cookie.len = sizeof *ufid;
> >>
> >> -    err = tc_replace_flower(ifindex, prio, handle, &flower, block_id,
> >> -                            TC_INGRESS);
> >> +    err = tc_replace_flower(ifindex, prio, handle, &flower, block_id, hook);
> >>      if (!err) {
> >>          add_ufid_tc_mapping(ufid, flower.prio, flower.handle, netdev, ifindex);
> >>      }
> >> @@ -1375,6 +1391,7 @@ netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
> >>                     struct ofpbuf *buf)
> >>  {
> >>      static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> >> +    enum tc_qdisc_hook hook = TC_INGRESS;
> >>      struct netdev *dev;
> >>      struct tc_flower flower;
> >>      uint32_t block_id = 0;
> >> @@ -1389,6 +1406,10 @@ netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
> >>          return ENOENT;
> >>      }
> >>
> >> +    if (is_internal_port(netdev_get_type(netdev))) {
> >> +        hook = TC_EGRESS;
> >> +    }
> >> +
> >>      ifindex = netdev_get_ifindex(dev);
> >>      if (ifindex < 0) {
> >>          VLOG_ERR_RL(&error_rl, "flow_get: failed to get ifindex for %s: %s",
> >> @@ -1400,7 +1421,7 @@ netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
> >>      block_id = get_block_id_from_netdev(dev);
> >>      VLOG_DBG_RL(&rl, "flow get (dev %s prio %d handle %d block_id %d)",
> >>                  netdev_get_name(dev), prio, handle, block_id);
> >> -    err = tc_get_flower(ifindex, prio, handle, &flower, block_id, TC_INGRESS);
> >> +    err = tc_get_flower(ifindex, prio, handle, &flower, block_id, hook);
> >>      netdev_close(dev);
> >>      if (err) {
> >>          VLOG_ERR_RL(&error_rl, "flow get failed (dev %s prio %d handle %d): %s",
> >> @@ -1422,6 +1443,7 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
> >>                     const ovs_u128 *ufid,
> >>                     struct dpif_flow_stats *stats)
> >>  {
> >> +    enum tc_qdisc_hook hook = TC_INGRESS;
> >>      struct tc_flower flower;
> >>      uint32_t block_id = 0;
> >>      struct netdev *dev;
> >> @@ -1435,6 +1457,10 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
> >>          return ENOENT;
> >>      }
> >>
> >> +    if (is_internal_port(netdev_get_type(netdev))) {
> >> +        hook = TC_EGRESS;
> >> +    }
> >> +
> >>      ifindex = netdev_get_ifindex(dev);
> >>      if (ifindex < 0) {
> >>          VLOG_ERR_RL(&error_rl, "flow_del: failed to get ifindex for %s: %s",
> >> @@ -1447,15 +1473,15 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
> >>
> >>      if (stats) {
> >>          memset(stats, 0, sizeof *stats);
> >> -        if (!tc_get_flower(ifindex, prio, handle, &flower, block_id,
> >> -                           TC_INGRESS)) {
> >> +        if (!tc_get_flower(ifindex, prio, handle, &flower, block_id, hook)) {
> >>              stats->n_packets = get_32aligned_u64(&flower.stats.n_packets);
> >>              stats->n_bytes = get_32aligned_u64(&flower.stats.n_bytes);
> >>              stats->used = flower.lastused;
> >>          }
> >>      }
> >>
> >> -    error = del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid);
> >> +    error = del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid,
> >> +                                        hook);
> >>
> >>      netdev_close(dev);
> >>
> >> @@ -1527,10 +1553,15 @@ netdev_tc_init_flow_api(struct netdev *netdev)
> >>  {
> >>      static struct ovsthread_once multi_mask_once = OVSTHREAD_ONCE_INITIALIZER;
> >>      static struct ovsthread_once block_once = OVSTHREAD_ONCE_INITIALIZER;
> >> +    enum tc_qdisc_hook hook = TC_INGRESS;
> >>      uint32_t block_id = 0;
> >>      int ifindex;
> >>      int error;
> >>
> >> +    if (is_internal_port(netdev_get_type(netdev))) {
> >> +        hook = TC_EGRESS;
> >> +    }
> >> +
>
> a small refactor suggestion could be a small function for getting
> qdisc hook as we have multiple functions declaring hook as ingress and
> then change to egress if internal port.
>
> static get_qdisc_hook(netdev)
> {
>   return is_internal_port(netdev_get_type(netdev) ? TC_EGRESS : TC_INGRESS;
> }
>
> then all functions will just have this line
>
> enum tc_qdisc_hook hook = get_qdisc_hook(netdev);
>

Hi Roi,
Thanks for the reviews.
Yes, I agree that this is cleaner.
Let me respin.

>
> >>      ifindex = netdev_get_ifindex(netdev);
> >>      if (ifindex < 0) {
> >>          VLOG_ERR_RL(&error_rl, "init: failed to get ifindex for %s: %s",
> >> @@ -1552,7 +1583,7 @@ netdev_tc_init_flow_api(struct netdev *netdev)
> >>      }
> >>
> >>      block_id = get_block_id_from_netdev(netdev);
> >> -    error = tc_add_del_qdisc(ifindex, true, block_id, TC_INGRESS);
> >> +    error = tc_add_del_qdisc(ifindex, true, block_id, hook);
> >>
> >>      if (error && error != EEXIST) {
> >>          VLOG_ERR("failed adding ingress qdisc required for offloading: %s",
> >>
> >
> > Reviewed-by: Roi Dayan <roid@mellanox.com>
> >
diff mbox series

Patch

diff --git a/lib/dpif.c b/lib/dpif.c
index 457c9bf..063ba20 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -101,12 +101,9 @@  static bool should_log_flow_message(const struct vlog_module *module,
 struct seq *tnl_conf_seq;
 
 static bool
-dpif_is_internal_port(const char *type)
+dpif_is_tap_port(const char *type)
 {
-    /* For userspace datapath, tap devices are the equivalent
-     * of internal devices in the kernel datapath, so both
-     * these types are 'internal' devices. */
-    return !strcmp(type, "internal") || !strcmp(type, "tap");
+    return !strcmp(type, "tap");
 }
 
 static void
@@ -359,7 +356,7 @@  do_open(const char *name, const char *type, bool create, struct dpif **dpifp)
             struct netdev *netdev;
             int err;
 
-            if (dpif_is_internal_port(dpif_port.type)) {
+            if (dpif_is_tap_port(dpif_port.type)) {
                 continue;
             }
 
@@ -434,7 +431,7 @@  dpif_remove_netdev_ports(struct dpif *dpif) {
         struct dpif_port dpif_port;
 
         DPIF_PORT_FOR_EACH (&dpif_port, &port_dump, dpif) {
-            if (!dpif_is_internal_port(dpif_port.type)) {
+            if (!dpif_is_tap_port(dpif_port.type)) {
                 netdev_ports_remove(dpif_port.port_no, dpif->dpif_class);
             }
         }
@@ -582,7 +579,7 @@  dpif_port_add(struct dpif *dpif, struct netdev *netdev, odp_port_t *port_nop)
         VLOG_DBG_RL(&dpmsg_rl, "%s: added %s as port %"PRIu32,
                     dpif_name(dpif), netdev_name, port_no);
 
-        if (!dpif_is_internal_port(netdev_get_type(netdev))) {
+        if (!dpif_is_tap_port(netdev_get_type(netdev))) {
 
             struct dpif_port dpif_port;
 
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index 87337e0..776d938 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -3340,6 +3340,7 @@  const struct netdev_class netdev_tap_class = {
 
 const struct netdev_class netdev_internal_class = {
     NETDEV_LINUX_CLASS_COMMON,
+    LINUX_FLOW_OFFLOAD_API,
     .type = "internal",
     .construct = netdev_linux_construct,
     .get_stats = netdev_internal_get_stats,
diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
index 64a9a9e..7c2ebe0 100644
--- a/lib/netdev-tc-offloads.c
+++ b/lib/netdev-tc-offloads.c
@@ -185,11 +185,12 @@  del_ufid_tc_mapping(const ovs_u128 *ufid)
 /* Wrapper function to delete filter and ufid tc mapping */
 static int
 del_filter_and_ufid_mapping(int ifindex, int prio, int handle,
-                            uint32_t block_id, const ovs_u128 *ufid)
+                            uint32_t block_id, const ovs_u128 *ufid,
+                            enum tc_qdisc_hook hook)
 {
     int err;
 
-    err = tc_del_filter(ifindex, prio, handle, block_id, TC_INGRESS);
+    err = tc_del_filter(ifindex, prio, handle, block_id, hook);
     del_ufid_tc_mapping(ufid);
 
     return err;
@@ -346,9 +347,14 @@  get_block_id_from_netdev(struct netdev *netdev)
 int
 netdev_tc_flow_flush(struct netdev *netdev)
 {
+    enum tc_qdisc_hook hook = TC_INGRESS;
     int ifindex = netdev_get_ifindex(netdev);
     uint32_t block_id = 0;
 
+    if (is_internal_port(netdev_get_type(netdev))) {
+        hook = TC_EGRESS;
+    }
+
     if (ifindex < 0) {
         VLOG_ERR_RL(&error_rl, "flow_flush: failed to get ifindex for %s: %s",
                     netdev_get_name(netdev), ovs_strerror(-ifindex));
@@ -357,17 +363,22 @@  netdev_tc_flow_flush(struct netdev *netdev)
 
     block_id = get_block_id_from_netdev(netdev);
 
-    return tc_flush(ifindex, block_id, TC_INGRESS);
+    return tc_flush(ifindex, block_id, hook);
 }
 
 int
 netdev_tc_flow_dump_create(struct netdev *netdev,
                            struct netdev_flow_dump **dump_out)
 {
+    enum tc_qdisc_hook hook = TC_INGRESS;
     struct netdev_flow_dump *dump;
     uint32_t block_id = 0;
     int ifindex;
 
+    if (is_internal_port(netdev_get_type(netdev))) {
+        hook = TC_EGRESS;
+    }
+
     ifindex = netdev_get_ifindex(netdev);
     if (ifindex < 0) {
         VLOG_ERR_RL(&error_rl, "dump_create: failed to get ifindex for %s: %s",
@@ -379,7 +390,7 @@  netdev_tc_flow_dump_create(struct netdev *netdev,
     dump = xzalloc(sizeof *dump);
     dump->nl_dump = xzalloc(sizeof *dump->nl_dump);
     dump->netdev = netdev_ref(netdev);
-    tc_dump_flower_start(ifindex, dump->nl_dump, block_id, TC_INGRESS);
+    tc_dump_flower_start(ifindex, dump->nl_dump, block_id, hook);
 
     *dump_out = dump;
 
@@ -1085,6 +1096,7 @@  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
     struct flow *mask = &match->wc.masks;
     const struct flow_tnl *tnl = &match->flow.tunnel;
     const struct flow_tnl *tnl_mask = &mask->tunnel;
+    enum tc_qdisc_hook hook = TC_INGRESS;
     struct tc_action *action;
     uint32_t block_id = 0;
     struct nlattr *nla;
@@ -1094,6 +1106,10 @@  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
     int ifindex;
     int err;
 
+    if (is_internal_port(netdev_get_type(netdev))) {
+        hook = TC_EGRESS;
+    }
+
     ifindex = netdev_get_ifindex(netdev);
     if (ifindex < 0) {
         VLOG_ERR_RL(&error_rl, "flow_put: failed to get ifindex for %s: %s",
@@ -1342,7 +1358,8 @@  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
     handle = get_ufid_tc_mapping(ufid, &prio, NULL);
     if (handle && prio) {
         VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d", handle, prio);
-        del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid);
+        del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid,
+                                    hook);
     }
 
     if (!prio) {
@@ -1356,8 +1373,7 @@  netdev_tc_flow_put(struct netdev *netdev, struct match *match,
     flower.act_cookie.data = ufid;
     flower.act_cookie.len = sizeof *ufid;
 
-    err = tc_replace_flower(ifindex, prio, handle, &flower, block_id,
-                            TC_INGRESS);
+    err = tc_replace_flower(ifindex, prio, handle, &flower, block_id, hook);
     if (!err) {
         add_ufid_tc_mapping(ufid, flower.prio, flower.handle, netdev, ifindex);
     }
@@ -1375,6 +1391,7 @@  netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
                    struct ofpbuf *buf)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
+    enum tc_qdisc_hook hook = TC_INGRESS;
     struct netdev *dev;
     struct tc_flower flower;
     uint32_t block_id = 0;
@@ -1389,6 +1406,10 @@  netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
         return ENOENT;
     }
 
+    if (is_internal_port(netdev_get_type(netdev))) {
+        hook = TC_EGRESS;
+    }
+
     ifindex = netdev_get_ifindex(dev);
     if (ifindex < 0) {
         VLOG_ERR_RL(&error_rl, "flow_get: failed to get ifindex for %s: %s",
@@ -1400,7 +1421,7 @@  netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
     block_id = get_block_id_from_netdev(dev);
     VLOG_DBG_RL(&rl, "flow get (dev %s prio %d handle %d block_id %d)",
                 netdev_get_name(dev), prio, handle, block_id);
-    err = tc_get_flower(ifindex, prio, handle, &flower, block_id, TC_INGRESS);
+    err = tc_get_flower(ifindex, prio, handle, &flower, block_id, hook);
     netdev_close(dev);
     if (err) {
         VLOG_ERR_RL(&error_rl, "flow get failed (dev %s prio %d handle %d): %s",
@@ -1422,6 +1443,7 @@  netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
                    const ovs_u128 *ufid,
                    struct dpif_flow_stats *stats)
 {
+    enum tc_qdisc_hook hook = TC_INGRESS;
     struct tc_flower flower;
     uint32_t block_id = 0;
     struct netdev *dev;
@@ -1435,6 +1457,10 @@  netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
         return ENOENT;
     }
 
+    if (is_internal_port(netdev_get_type(netdev))) {
+        hook = TC_EGRESS;
+    }
+
     ifindex = netdev_get_ifindex(dev);
     if (ifindex < 0) {
         VLOG_ERR_RL(&error_rl, "flow_del: failed to get ifindex for %s: %s",
@@ -1447,15 +1473,15 @@  netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
 
     if (stats) {
         memset(stats, 0, sizeof *stats);
-        if (!tc_get_flower(ifindex, prio, handle, &flower, block_id,
-                           TC_INGRESS)) {
+        if (!tc_get_flower(ifindex, prio, handle, &flower, block_id, hook)) {
             stats->n_packets = get_32aligned_u64(&flower.stats.n_packets);
             stats->n_bytes = get_32aligned_u64(&flower.stats.n_bytes);
             stats->used = flower.lastused;
         }
     }
 
-    error = del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid);
+    error = del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid,
+                                        hook);
 
     netdev_close(dev);
 
@@ -1527,10 +1553,15 @@  netdev_tc_init_flow_api(struct netdev *netdev)
 {
     static struct ovsthread_once multi_mask_once = OVSTHREAD_ONCE_INITIALIZER;
     static struct ovsthread_once block_once = OVSTHREAD_ONCE_INITIALIZER;
+    enum tc_qdisc_hook hook = TC_INGRESS;
     uint32_t block_id = 0;
     int ifindex;
     int error;
 
+    if (is_internal_port(netdev_get_type(netdev))) {
+        hook = TC_EGRESS;
+    }
+
     ifindex = netdev_get_ifindex(netdev);
     if (ifindex < 0) {
         VLOG_ERR_RL(&error_rl, "init: failed to get ifindex for %s: %s",
@@ -1552,7 +1583,7 @@  netdev_tc_init_flow_api(struct netdev *netdev)
     }
 
     block_id = get_block_id_from_netdev(netdev);
-    error = tc_add_del_qdisc(ifindex, true, block_id, TC_INGRESS);
+    error = tc_add_del_qdisc(ifindex, true, block_id, hook);
 
     if (error && error != EEXIST) {
         VLOG_ERR("failed adding ingress qdisc required for offloading: %s",