On Mon, Apr 8, 2019 at 11:49 AM Roi Dayan <[email protected]> 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 <[email protected]>
> >> ---
> >> 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 <[email protected]>
> >
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
