Hello Simon,

This patch has me intrigued. By the looks of it, it bears uncanny
resemblance to patch [1] by another author. Is your patch based
on patch [1]? If yes, could you please comment on the following:

1) Your patch does not seem to reference the original author.
   Why is it so? Is there a problem, colleagues?

2) Your patch does not seem to address review feedback [2].
   There's a problem that has been indicated by Eli,
   regarding flow flush. Doesn't it still stand?

Interested to hear your input on this. Thank you.

[1] https://mail.openvswitch.org/pipermail/ovs-dev/2023-February/402152.html

[2] https://mail.openvswitch.org/pipermail/ovs-dev/2023-February/402172.html

On Wed, 26 Apr 2023, Simon Horman wrote:

From: Peng Zhang <peng.zhang at corigine.com>

Manage "transfer" flows via the corresponding mechanism.
Doing so requires that the traffic source be specified
explicitly, via the corresponding pattern item.

Signed-off-by: Peng Zhang <peng.zhang at corigine.com>
Signed-off-by: Jin Liu <jin.liu at corigine.com>
Co-authored-by: Jin Liu <jin.liu at corigine.com>
Signed-off-by: Simon Horman <simon.horman at corigine.com>
---
lib/netdev-dpdk.c         | 97 +++++++++++++++++++++++++++++++++------
lib/netdev-dpdk.h         |  3 ++
lib/netdev-offload-dpdk.c | 16 ++++---
3 files changed, 96 insertions(+), 20 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index fff57f78279a..278d6afc08af 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -437,6 +437,7 @@ enum dpdk_hw_ol_features {
struct netdev_dpdk {
    PADDED_MEMBERS_CACHELINE_MARKER(CACHE_LINE_SIZE, cacheline0,
        dpdk_port_t port_id;
+        dpdk_port_t flow_transfer_proxy_port_id;

        /* If true, device was attached by rte_eth_dev_attach(). */
        bool attached;
@@ -1155,6 +1156,24 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
    uint32_t rx_chksm_offload_capa = RTE_ETH_RX_OFFLOAD_UDP_CKSUM |
                                     RTE_ETH_RX_OFFLOAD_TCP_CKSUM |
                                     RTE_ETH_RX_OFFLOAD_IPV4_CKSUM;
+    int ret;
+
+    /* Managing "transfer" flows requires that the user communicate them
+     * via a port which has the privilege to control the embedded switch.
+     * For some vendors, all ports in a given switching domain have
+     * this privilege. For other vendors, it's only one port.
+     *
+     * Get the proxy port ID and remember it for later use.
+     */
+    ret = rte_flow_pick_transfer_proxy(dev->port_id,
+                                       &dev->flow_transfer_proxy_port_id,
+                                       NULL);
+    if (ret != 0) {
+        /* The PMD does not indicate the proxy port.
+         * Assume the proxy is unneeded.
+         */
+        dev->flow_transfer_proxy_port_id = dev->port_id;
+    }

    rte_eth_dev_info_get(dev->port_id, &info);

@@ -3767,8 +3786,10 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc 
OVS_UNUSED,
                   const char *argv[], void *aux OVS_UNUSED)
{
    struct ds used_interfaces = DS_EMPTY_INITIALIZER;
+    struct rte_flow_error ferror;
    struct rte_eth_dev_info dev_info;
    dpdk_port_t sibling_port_id;
+    struct netdev_dpdk *dev;
    dpdk_port_t port_id;
    bool used = false;
    char *response;
@@ -3786,8 +3807,6 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc 
OVS_UNUSED,
                  argv[1]);

    RTE_ETH_FOREACH_DEV_SIBLING (sibling_port_id, port_id) {
-        struct netdev_dpdk *dev;
-
        LIST_FOR_EACH (dev, list_node, &dpdk_list) {
            if (dev->port_id != sibling_port_id) {
                continue;
@@ -3807,6 +3826,22 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc 
OVS_UNUSED,
    }
    ds_destroy(&used_interfaces);

+    /* The device being detached may happen to be a flow proxy port
+     * for another device (still attached). Update the flow proxy port id.
+     */
+    LIST_FOR_EACH (dev, list_node, &dpdk_list) {
+        if (dev->port_id != port_id &&
+            dev->flow_transfer_proxy_port_id == port_id) {
+            if (rte_flow_flush(dev->flow_transfer_proxy_port_id, &ferror)) {
+                response = xasprintf("Flush port failed, Device '%s' can not"
+                                     "be detached (flow proxy)", argv[1]);
+                goto error;
+            }
+            break;
+        }
+    }
+
+
    rte_eth_dev_info_get(port_id, &dev_info);
    rte_eth_dev_close(port_id);
    if (rte_dev_remove(dev_info.device) < 0) {
@@ -3817,6 +3852,16 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc 
OVS_UNUSED,
    response = xasprintf("All devices shared with device '%s' "
                         "have been detached", argv[1]);

+    /* The device being detached may happen to be a flow proxy port.
+     * After the flow proxy port was detached, the related ports
+     * will reconfigure the device and update the proxy_port_id.
+     */
+    LIST_FOR_EACH (dev, list_node, &dpdk_list) {
+         if (dev->flow_transfer_proxy_port_id == port_id) {
+                netdev_request_reconfigure(&dev->up);
+        }
+    }
+
    ovs_mutex_unlock(&dpdk_mutex);
    unixctl_command_reply(conn, response);
    free(response);
@@ -5192,6 +5237,23 @@ out:
    return ret;
}

+int
+netdev_dpdk_get_prox_port_id(struct netdev *netdev)
+{
+    struct netdev_dpdk *dev;
+    int ret;
+
+    if (!is_dpdk_class(netdev->netdev_class)) {
+        return -1;
+    }
+
+    dev = netdev_dpdk_cast(netdev);
+    ovs_mutex_lock(&dev->mutex);
+    ret = dev->flow_transfer_proxy_port_id;
+    ovs_mutex_unlock(&dev->mutex);
+    return ret;
+}
+
bool
netdev_dpdk_flow_api_supported(struct netdev *netdev)
{
@@ -5222,13 +5284,15 @@ out:

int
netdev_dpdk_rte_flow_destroy(struct netdev *netdev,
+                             bool transfer,
                             struct rte_flow *rte_flow,
                             struct rte_flow_error *error)
{
    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
    int ret;

-    ret = rte_flow_destroy(dev->port_id, rte_flow, error);
+    ret = rte_flow_destroy(transfer ? dev->flow_transfer_proxy_port_id :
+                           dev->port_id, rte_flow, error);
    return ret;
}

@@ -5242,7 +5306,9 @@ netdev_dpdk_rte_flow_create(struct netdev *netdev,
    struct rte_flow *flow;
    struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);

-    flow = rte_flow_create(dev->port_id, attr, items, actions, error);
+    flow = rte_flow_create(attr->transfer ?
+                           dev->flow_transfer_proxy_port_id : dev->port_id,
+                           attr, items, actions, error);
    return flow;
}

@@ -5270,7 +5336,8 @@ netdev_dpdk_rte_flow_query_count(struct netdev *netdev,
    }

    dev = netdev_dpdk_cast(netdev);
-    ret = rte_flow_query(dev->port_id, rte_flow, actions, query, error);
+    ret = rte_flow_query(dev->flow_transfer_proxy_port_id, rte_flow,
+                         actions, query, error);
    return ret;
}

@@ -5292,8 +5359,8 @@ netdev_dpdk_rte_flow_tunnel_decap_set(struct netdev 
*netdev,

    dev = netdev_dpdk_cast(netdev);
    ovs_mutex_lock(&dev->mutex);
-    ret = rte_flow_tunnel_decap_set(dev->port_id, tunnel, actions,
-                                    num_of_actions, error);
+    ret = rte_flow_tunnel_decap_set(dev->flow_transfer_proxy_port_id, tunnel,
+                                    actions, num_of_actions, error);
    ovs_mutex_unlock(&dev->mutex);
    return ret;
}
@@ -5314,8 +5381,8 @@ netdev_dpdk_rte_flow_tunnel_match(struct netdev *netdev,

    dev = netdev_dpdk_cast(netdev);
    ovs_mutex_lock(&dev->mutex);
-    ret = rte_flow_tunnel_match(dev->port_id, tunnel, items, num_of_items,
-                                error);
+    ret = rte_flow_tunnel_match(dev->flow_transfer_proxy_port_id, tunnel,
+                                items, num_of_items, error);
    ovs_mutex_unlock(&dev->mutex);
    return ret;
}
@@ -5336,7 +5403,8 @@ netdev_dpdk_rte_flow_get_restore_info(struct netdev 
*netdev,

    dev = netdev_dpdk_cast(netdev);
    ovs_mutex_lock(&dev->mutex);
-    ret = rte_flow_get_restore_info(dev->port_id, m, info, error);
+    ret = rte_flow_get_restore_info(dev->flow_transfer_proxy_port_id, m, info,
+                                    error);
    ovs_mutex_unlock(&dev->mutex);
    return ret;
}
@@ -5357,8 +5425,9 @@ netdev_dpdk_rte_flow_tunnel_action_decap_release(

    dev = netdev_dpdk_cast(netdev);
    ovs_mutex_lock(&dev->mutex);
-    ret = rte_flow_tunnel_action_decap_release(dev->port_id, actions,
-                                               num_of_actions, error);
+    ret = rte_flow_tunnel_action_decap_release(
+                                            dev->flow_transfer_proxy_port_id,
+                                            actions, num_of_actions, error);
    ovs_mutex_unlock(&dev->mutex);
    return ret;
}
@@ -5378,8 +5447,8 @@ netdev_dpdk_rte_flow_tunnel_item_release(struct netdev 
*netdev,

    dev = netdev_dpdk_cast(netdev);
    ovs_mutex_lock(&dev->mutex);
-    ret = rte_flow_tunnel_item_release(dev->port_id, items, num_of_items,
-                                       error);
+    ret = rte_flow_tunnel_item_release(dev->flow_transfer_proxy_port_id, items,
+                                       num_of_items, error);
    ovs_mutex_unlock(&dev->mutex);
    return ret;
}
diff --git a/lib/netdev-dpdk.h b/lib/netdev-dpdk.h
index 5cd95d00f5a5..5ba59a82e8a7 100644
--- a/lib/netdev-dpdk.h
+++ b/lib/netdev-dpdk.h
@@ -36,6 +36,7 @@ bool netdev_dpdk_flow_api_supported(struct netdev *);

int
netdev_dpdk_rte_flow_destroy(struct netdev *netdev,
+                             bool transfer,
                             struct rte_flow *rte_flow,
                             struct rte_flow_error *error);
struct rte_flow *
@@ -51,6 +52,8 @@ netdev_dpdk_rte_flow_query_count(struct netdev *netdev,
                                 struct rte_flow_error *error);
int
netdev_dpdk_get_port_id(struct netdev *netdev);
+int
+netdev_dpdk_get_prox_port_id(struct netdev *netdev);

#ifdef ALLOW_EXPERIMENTAL_API

diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index 38f00fd309e6..9b594ac1ef90 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -920,6 +920,7 @@ netdev_offload_dpdk_flow_create(struct netdev *netdev,
            extra_str = ds_cstr(&s_extra);
            VLOG_DBG_RL(&rl, "%s: rte_flow 0x%"PRIxPTR" %s  flow create %d %s",
                        netdev_get_name(netdev), (intptr_t) flow, extra_str,
+                        attr->transfer ? netdev_dpdk_get_prox_port_id(netdev) :
                        netdev_dpdk_get_port_id(netdev), ds_cstr(&s));
        }
    } else {
@@ -935,6 +936,7 @@ netdev_offload_dpdk_flow_create(struct netdev *netdev,
            extra_str = ds_cstr(&s_extra);
            VLOG_RL(&rl, level, "%s: Failed flow: %s  flow create %d %s",
                    netdev_get_name(netdev), extra_str,
+                    attr->transfer ? netdev_dpdk_get_prox_port_id(netdev) :
                    netdev_dpdk_get_port_id(netdev), ds_cstr(&s));
        }
    }
@@ -1114,13 +1116,13 @@ vport_to_rte_tunnel(struct netdev *vport,
        tunnel->tp_dst = tnl_cfg->dst_port;
        if (!VLOG_DROP_DBG(&rl)) {
            ds_put_format(s_tnl, "flow tunnel create %d type vxlan; ",
-                          netdev_dpdk_get_port_id(netdev));
+                          netdev_dpdk_get_prox_port_id(netdev));
        }
    } else if (!strcmp(netdev_get_type(vport), "gre")) {
        tunnel->type = RTE_FLOW_ITEM_TYPE_GRE;
        if (!VLOG_DROP_DBG(&rl)) {
            ds_put_format(s_tnl, "flow tunnel create %d type gre; ",
-                          netdev_dpdk_get_port_id(netdev));
+                          netdev_dpdk_get_prox_port_id(netdev));
        }
    } else {
        VLOG_DBG_RL(&rl, "vport type '%s' is not supported",
@@ -2242,7 +2244,7 @@ netdev_offload_dpdk_actions(struct netdev *netdev,
                            struct nlattr *nl_actions,
                            size_t actions_len)
{
-    const struct rte_flow_attr flow_attr = { .ingress = 1, .transfer = 1 };
+    const struct rte_flow_attr flow_attr = { .transfer = 1 };
    struct flow_actions actions = {
        .actions = NULL,
        .cnt = 0,
@@ -2319,6 +2321,7 @@ netdev_offload_dpdk_flow_destroy(struct 
ufid_to_rte_flow_data *rte_flow_data)
    struct netdev *physdev;
    struct netdev *netdev;
    ovs_u128 *ufid;
+    bool transfer;
    int ret;

    ovs_mutex_lock(&rte_flow_data->lock);
@@ -2330,12 +2333,13 @@ netdev_offload_dpdk_flow_destroy(struct 
ufid_to_rte_flow_data *rte_flow_data)

    rte_flow_data->dead = true;

+    transfer = rte_flow_data->actions_offloaded;
    rte_flow = rte_flow_data->rte_flow;
    physdev = rte_flow_data->physdev;
    netdev = rte_flow_data->netdev;
    ufid = &rte_flow_data->ufid;

-    ret = netdev_dpdk_rte_flow_destroy(physdev, rte_flow, &error);
+    ret = netdev_dpdk_rte_flow_destroy(physdev, transfer, rte_flow, &error);

    if (ret == 0) {
        struct netdev_offload_dpdk_data *data;
@@ -2350,12 +2354,12 @@ netdev_offload_dpdk_flow_destroy(struct 
ufid_to_rte_flow_data *rte_flow_data)
                    " flow destroy %d ufid " UUID_FMT,
                    netdev_get_name(netdev), netdev_get_name(physdev),
                    (intptr_t) rte_flow,
-                    netdev_dpdk_get_port_id(physdev),
+                    netdev_dpdk_get_prox_port_id(physdev),
                    UUID_ARGS((struct uuid *) ufid));
    } else {
        VLOG_ERR("Failed flow: %s/%s: flow destroy %d ufid " UUID_FMT,
                 netdev_get_name(netdev), netdev_get_name(physdev),
-                 netdev_dpdk_get_port_id(physdev),
+                 netdev_dpdk_get_prox_port_id(physdev),
                 UUID_ARGS((struct uuid *) ufid));
    }

--
2.30.2

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to